On 10/8/24 02:15, Niklas Cassel wrote:
> A user reported that commit aa3998dbeb3a ("ata: libata-scsi: Disable scsi
> device manage_system_start_stop") introduced a spin down + immediate spin
> up of the disk both when entering and when resuming from hibernation.
> This behavior was not there before, and causes an increased latency both
> when when entering and when resuming from hibernation.
>
> Hibernation is done by three consecutive PM events, in the following order:
> 1) PM_EVENT_FREEZE
> 2) PM_EVENT_THAW
> 3) PM_EVENT_HIBERNATE
>
> Commit aa3998dbeb3a ("ata: libata-scsi: Disable scsi device
> manage_system_start_stop") modified ata_eh_handle_port_suspend() to call
> ata_dev_power_set_standby() (which spins down the disk), for both event
> PM_EVENT_FREEZE and event PM_EVENT_HIBERNATE.
>
> Documentation/driver-api/pm/devices.rst, section "Entering Hibernation",
> explicitly mentions that PM_EVENT_FREEZE does not have to be put the device
> in a low-power state, and actually recommends not doing so. Thus, let's not
> spin down the disk on PM_EVENT_FREEZE. (The disk will instead be spun down
> during the subsequent PM_EVENT_HIBERNATE event.)
>
> This way, PM_EVENT_FREEZE will behave as it did before commit aa3998dbeb3a
> ("ata: libata-scsi: Disable scsi device manage_system_start_stop"), while
> PM_EVENT_HIBERNATE will continue to spin down the disk.
>
> This will avoid the superfluous spin down + spin up when entering and
> resuming from hibernation, while still making sure that the disk is spun
> down before actually entering hibernation.
>
> Fixes: aa3998dbeb3a ("ata: libata-scsi: Disable scsi device manage_system_start_stop")
You forgot to add Cc: stable@xxxxxxxxxxxxxxx
With that added, looks good.
Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
> drivers/ata/libata-eh.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 3f0144e7dc80..fa41ea57a978 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -4099,10 +4099,20 @@ static void ata_eh_handle_port_suspend(struct ata_port *ap)
>
> WARN_ON(ap->pflags & ATA_PFLAG_SUSPENDED);
>
> - /* Set all devices attached to the port in standby mode */
> - ata_for_each_link(link, ap, HOST_FIRST) {
> - ata_for_each_dev(dev, link, ENABLED)
> - ata_dev_power_set_standby(dev);
> + /*
> + * We will reach this point for all of the PM events:
> + * PM_EVENT_SUSPEND (if runtime pm, PM_EVENT_AUTO will also be set)
> + * PM_EVENT_FREEZE, and PM_EVENT_HIBERNATE.
> + *
> + * We do not want to perform disk spin down for PM_EVENT_FREEZE.
> + * (Spin down will be performed by the subsequent PM_EVENT_HIBERNATE.)
> + */
> + if (!(ap->pm_mesg.event & PM_EVENT_FREEZE)) {
> + /* Set all devices attached to the port in standby mode */
> + ata_for_each_link(link, ap, HOST_FIRST) {
> + ata_for_each_dev(dev, link, ENABLED)
> + ata_dev_power_set_standby(dev);
> + }
> }
>
> /*
--
Damien Le Moal
Western Digital Research
