[v5] Support power resources defined in acpi on ata

[PATCH v5 3/3] ata: Use ACPI methods to power on disks

Posted by Markus Probst 3 months, 2 weeks ago

Some embedded devices have the ability to control whether power is
provided to the disks via the SATA power connector or not. If power
resources are defined on ATA ports / devices in ACPI, we should try to set
the power state to D0 before probing the disk to ensure that any power
supply or power gate that may exist is providing power to the disk.

An example for such devices would be newer synology NAS devices. Every
disk slot has its own SATA power connector. Whether the connector is
providing power is controlled via an gpio, which is *off by default*.
Also the disk loses power on reboots.

Add a new function, ata_acpi_port_power_on(), that will be used to power
on the SATA power connector if usable ACPI power resources on the
associated ATA port / device are found. It will be called right before
probing the port, therefore the disk will be powered on just in time.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/ata/libata-acpi.c | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-core.c |  2 ++
 drivers/ata/libata.h      |  2 ++
 3 files changed, 44 insertions(+)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 196ca1227d09..b0d70e075cd2 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -271,6 +271,46 @@ bool ata_acpi_dev_manage_restart(struct ata_device *dev)
 	return acpi_bus_power_manageable(ACPI_HANDLE(tdev));
 }
 
+/**
+ * ata_acpi_port_power_on - set the power state of the ata port to D0
+ * @ap: target ATA port
+ *
+ * This function is called at the beginning of ata_port_probe.
+ */
+void ata_acpi_port_power_on(struct ata_port *ap)
+{
+	acpi_handle handle;
+	int i;
+
+	/* If `ATA_FLAG_ACPI_SATA` is set, the acpi fwnode is attached to the
+	 * `ata_device` instead of the `ata_port`.
+	 */
+	if (ap->flags & ATA_FLAG_ACPI_SATA) {
+		for (i = 0; i < ATA_MAX_DEVICES; i++) {
+			struct ata_device *dev = &ap->link.device[i];
+
+			if (!is_acpi_device_node(dev->tdev.fwnode))
+				continue;
+			handle = ACPI_HANDLE(&dev->tdev);
+			if (!acpi_bus_power_manageable(handle))
+				continue;
+			if (acpi_bus_set_power(handle, ACPI_STATE_D0))
+				ata_dev_err(dev,
+					    "acpi: failed to power state to D0\n");
+		}
+		return;
+	}
+
+	if (!is_acpi_device_node(ap->tdev.fwnode))
+		return;
+	handle = ACPI_HANDLE(&ap->tdev);
+	if (!acpi_bus_power_manageable(handle))
+		return;
+
+	if (acpi_bus_set_power(handle, ACPI_STATE_D0))
+		ata_port_err(ap, "acpi: failed to set power state to D0\n");
+}
+
 /**
  * ata_acpi_dissociate - dissociate ATA host from ACPI objects
  * @host: target ATA host
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2a210719c4ce..a6813ced3ec2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5901,6 +5901,8 @@ void ata_port_probe(struct ata_port *ap)
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	unsigned long flags;
 
+	ata_acpi_port_power_on(ap);
+
 	/* kick EH for boot probing */
 	spin_lock_irqsave(ap->lock, flags);
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index af08bb9b40d0..0e7ecac73680 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -130,6 +130,7 @@ extern void ata_acpi_on_disable(struct ata_device *dev);
 extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
 extern void ata_acpi_bind_port(struct ata_port *ap);
 extern void ata_acpi_bind_dev(struct ata_device *dev);
+extern void ata_acpi_port_power_on(struct ata_port *ap);
 extern bool ata_acpi_dev_manage_restart(struct ata_device *dev);
 extern acpi_handle ata_dev_acpi_handle(struct ata_device *dev);
 #else
@@ -141,6 +142,7 @@ static inline void ata_acpi_set_state(struct ata_port *ap,
 				      pm_message_t state) { }
 static inline void ata_acpi_bind_port(struct ata_port *ap) {}
 static inline void ata_acpi_bind_dev(struct ata_device *dev) {}
+static inline void ata_acpi_port_power_on(struct ata_port *ap) {}
 static inline bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
 #endif
 
-- 
2.51.0

Re: [PATCH v5 3/3] ata: Use ACPI methods to power on disks

Posted by Damien Le Moal 3 months, 2 weeks ago

On 10/28/25 5:23 AM, Markus Probst wrote:
> Some embedded devices have the ability to control whether power is
> provided to the disks via the SATA power connector or not. If power
> resources are defined on ATA ports / devices in ACPI, we should try to set
> the power state to D0 before probing the disk to ensure that any power
> supply or power gate that may exist is providing power to the disk.
> 
> An example for such devices would be newer synology NAS devices. Every
> disk slot has its own SATA power connector. Whether the connector is
> providing power is controlled via an gpio, which is *off by default*.
> Also the disk loses power on reboots.
> 
> Add a new function, ata_acpi_port_power_on(), that will be used to power
> on the SATA power connector if usable ACPI power resources on the
> associated ATA port / device are found. It will be called right before
> probing the port, therefore the disk will be powered on just in time.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>

I think that this patch should be 2/3 and 2/3 should be 3/3. That makes more
sense to have the basic power control first and then solve the restart power
loss after that.

> ---
>  drivers/ata/libata-acpi.c | 40 +++++++++++++++++++++++++++++++++++++++
>  drivers/ata/libata-core.c |  2 ++
>  drivers/ata/libata.h      |  2 ++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 196ca1227d09..b0d70e075cd2 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -271,6 +271,46 @@ bool ata_acpi_dev_manage_restart(struct ata_device *dev)
>  	return acpi_bus_power_manageable(ACPI_HANDLE(tdev));
>  }
>  
> +/**
> + * ata_acpi_port_power_on - set the power state of the ata port to D0
> + * @ap: target ATA port
> + *
> + * This function is called at the beginning of ata_port_probe.

Please add "()" after ata_port_probe ("ata_port_probe()") to show it is a function.

> + */
> +void ata_acpi_port_power_on(struct ata_port *ap)
> +{
> +	acpi_handle handle;
> +	int i;
> +
> +	/* If `ATA_FLAG_ACPI_SATA` is set, the acpi fwnode is attached to the
> +	 * `ata_device` instead of the `ata_port`.
> +	 */

As already commented, please drop the quote marks inside the comment (not
needed at all) and format the multi line comment correctly:

	/*
	 * If ATA_FLAG_ACPI_SATA is set, the acpi fwnode is attached to the
	 * ATA device instead of the ATA port.
	 */

> +	if (ap->flags & ATA_FLAG_ACPI_SATA) {
> +		for (i = 0; i < ATA_MAX_DEVICES; i++) {
> +			struct ata_device *dev = &ap->link.device[i];
> +
> +			if (!is_acpi_device_node(dev->tdev.fwnode))
> +				continue;
> +			handle = ACPI_HANDLE(&dev->tdev);
> +			if (!acpi_bus_power_manageable(handle))
> +				continue;
> +			if (acpi_bus_set_power(handle, ACPI_STATE_D0))
> +				ata_dev_err(dev,
> +					    "acpi: failed to power state to D0\n");

"acpi: failed to set power state to D0\n" ? (set missing ?)

> +		}
> +		return;
> +	}
> +
> +	if (!is_acpi_device_node(ap->tdev.fwnode))
> +		return;
> +	handle = ACPI_HANDLE(&ap->tdev);
> +	if (!acpi_bus_power_manageable(handle))
> +		return;
> +
> +	if (acpi_bus_set_power(handle, ACPI_STATE_D0))
> +		ata_port_err(ap, "acpi: failed to set power state to D0\n");

Not missing here :)

> +}
> +
>  /**
>   * ata_acpi_dissociate - dissociate ATA host from ACPI objects
>   * @host: target ATA host
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 2a210719c4ce..a6813ced3ec2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5901,6 +5901,8 @@ void ata_port_probe(struct ata_port *ap)
>  	struct ata_eh_info *ehi = &ap->link.eh_info;
>  	unsigned long flags;
>  
> +	ata_acpi_port_power_on(ap);
> +
>  	/* kick EH for boot probing */
>  	spin_lock_irqsave(ap->lock, flags);
>  
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index af08bb9b40d0..0e7ecac73680 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -130,6 +130,7 @@ extern void ata_acpi_on_disable(struct ata_device *dev);
>  extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
>  extern void ata_acpi_bind_port(struct ata_port *ap);
>  extern void ata_acpi_bind_dev(struct ata_device *dev);
> +extern void ata_acpi_port_power_on(struct ata_port *ap);
>  extern bool ata_acpi_dev_manage_restart(struct ata_device *dev);
>  extern acpi_handle ata_dev_acpi_handle(struct ata_device *dev);
>  #else
> @@ -141,6 +142,7 @@ static inline void ata_acpi_set_state(struct ata_port *ap,
>  				      pm_message_t state) { }
>  static inline void ata_acpi_bind_port(struct ata_port *ap) {}
>  static inline void ata_acpi_bind_dev(struct ata_device *dev) {}
> +static inline void ata_acpi_port_power_on(struct ata_port *ap) {}
>  static inline bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
>  #endif
>  


-- 
Damien Le Moal
Western Digital Research

[PATCH v5 1/3] scsi: sd: Add manage_restart device attribute to scsi_disk
[PATCH v5 2/3] ata: stop disk on restart if ACPI power resources are found
[PATCH v5 3/3] ata: Use ACPI methods to power on disks