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

[Damien Le Moal]

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@xxxxxxxxx>

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