Re: [PATCH v4 2/3] ata: stop disk on restart if ACPI power resources are found

[Damien Le Moal <dlemoal@kernel.org>]

On 2025/10/23 9:52, 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. ACPI power
> resources are usually off by default, thus making it unclear if the
> specific power resource will retain its state after a restart. If power
> resources are defined on ATA ports / devices in ACPI, we should stop the
> disk on SYSTEM_RESTART, to ensure the disk will not lose power while
> active.
> 
> Add a new function, ata_acpi_dev_manage_restart(), that will be used to
> determine if a disk should be stopped before restarting the system. If a
> usable ACPI power resource has been found, it is assumed that the disk
> will lose power after a restart and should be stopped to avoid a power
> failure.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  drivers/ata/libata-acpi.c | 28 ++++++++++++++++++++++++++++
>  drivers/ata/libata-scsi.c |  1 +
>  drivers/ata/libata.h      |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index f2140fc06ba0..f05c247d25bc 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -245,6 +245,34 @@ void ata_acpi_bind_dev(struct ata_device *dev)
>  				   ata_acpi_dev_uevent);
>  }
>  
> +/**
> + * ata_acpi_dev_manage_restart - if the disk should be stopped (spun down) on
> + * system restart.

Please align "system restart" to the "if" above.

> + * @dev: target ATA device
> + *
> + * RETURNS:
> + * 1 if the disk should be stopped, otherwise 0

s/1/true
s/0/false

And please terminate sentences with a period.

> + */
> +bool ata_acpi_dev_manage_restart(struct ata_device *dev)
> +{
> +	/* If the device is power manageable, we assume the disk loses power on
> +	 * reboot.
> +	 */

And then ? This seems incomplete...
Also please adhere to the normal multi-line comment style by starting the
multi-line comment block with "/*".

	/*
	 * If the device is power manageable, we assume the disk loses power on
	 * reboot.
	 */

> +
> +	/* If `ATA_FLAG_ACPI_SATA` is set, the acpi fwnode is attached to the
> +	 * `ata_device` instead of the `ata_port`.
> +	 */

No need for all the quote marks here.

> +	if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA) {
> +		if (!is_acpi_device_node(dev->tdev.fwnode))
> +			return 0;

			return false;

> +		return acpi_bus_power_manageable(ACPI_HANDLE(&dev->tdev));
> +	}
> +
> +	if (!is_acpi_device_node(dev->link->ap->tdev.fwnode))
> +		return 0;

		return false;

> +	return acpi_bus_power_manageable(ACPI_HANDLE(&dev->link->ap->tdev));
> +}

Overall, I think the following is cleaner and simpler as it doe not repeat code:

bool ata_acpi_dev_manage_restart(struct ata_device *dev)
{
	struct device *tdev;

	/*
	 * If ATA_FLAG_ACPI_SATA is set, the acpi fwnode is attached to the
	 * ATA device instead of the port.
	 */
	if (dev->link->ap->flags & ATA_FLAG_ACPI_SATA)
		tdev = &dev->tdev;
	else
		tdev = &dev->link->ap->tdev;

	if (!is_acpi_device_node(tdev->fwnode))
		return false;

	return acpi_bus_power_manageable(ACPI_HANDLE(tdev));
}

> +
>  /**
>   * ata_acpi_dissociate - dissociate ATA host from ACPI objects
>   * @host: target ATA host
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b43a3196e2be..026122bb6f2f 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1095,6 +1095,7 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
>  		 */
>  		sdev->manage_runtime_start_stop = 1;
>  		sdev->manage_shutdown = 1;
> +		sdev->manage_restart = ata_acpi_dev_manage_restart(dev);
>  		sdev->force_runtime_start_on_system_start = 1;
>  	}
>  
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index e5b977a8d3e1..af08bb9b40d0 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 bool ata_acpi_dev_manage_restart(struct ata_device *dev);
>  extern acpi_handle ata_dev_acpi_handle(struct ata_device *dev);
>  #else
>  static inline void ata_acpi_dissociate(struct ata_host *host) { }
> @@ -140,6 +141,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 bool ata_acpi_dev_manage_restart(struct ata_device *dev) { return 0; }
>  #endif
>  
>  /* libata-scsi.c */


-- 
Damien Le Moal
Western Digital Research