Re: libahci driver and power switching HDD on newer kernels

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/3/24 06:20, Niklas Cassel wrote:
> On Tue, Sep 24, 2024 at 12:42:10PM +0200, W wrote:
>>>
>>> Given that you had 6.4.12 working OK, it is likely some commit that introduced a
>>> regression. If you can git bisect it, we will have a better idea how to remove
>>> the regression.
>>
>> Please take a look at bugzilla report:
>> https://bugzilla.kernel.org/show_bug.cgi?id=219296 - there are the details.
>>
>> I'm wondering what is the better way for communication - here on mailing
>> list or put the comments in bugzilla ticket?
>> Probably here will be better idea...
>>
>> W
>>
> 
> Hello W,
> 
> Could you please try the following patch,
> and see if it helps:
> 
> 
> From dba01b7d68fffc26f3abf3252296082311a767a0 Mon Sep 17 00:00:00 2001
> From: Niklas Cassel <cassel@xxxxxxxxxx>
> Date: Wed, 2 Oct 2024 21:40:41 +0200
> Subject: [PATCH] ata: libata: do not spin down disk on PM event freeze
> 
> Currently, ata_eh_handle_port_suspend() will return early if
> ATA_PFLAG_PM_PENDING is not set, or if the PM event has flag
> PM_EVENT_RESUME set.
> 
> This means that the following PM callbacks:
> .suspend = ata_port_pm_suspend,
> .freeze = ata_port_pm_freeze,
> .poweroff = ata_port_pm_poweroff,
> .runtime_suspend = ata_port_runtime_suspend,
> will actually make ata_eh_handle_port_suspend() perform some work.
> 
> ata_eh_handle_port_suspend() will spin down the disks (by calling
> ata_dev_power_set_standby()), regardless of the PM event.
> 
> Documentation/driver-api/pm/devices.rst, section "Entering Hibernation",
> explicitly mentions that .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 for the .freeze() callback. (The disk will instead be
> spun down during the succeeding .poweroff() callback.)
> 
> Fixes: aa3998dbeb3a ("ata: libata-scsi: Disable scsi device manage_system_start_stop")
> 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..45a0d9af2d54 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 succeeding PM_EVENT_HIBERNATE.)
> +        */
> +       if (!(ap->pm_mesg.event & PM_EVENT_FREEZE)) {

This feels odd: not doing anything to the drive for PM_EVENT_FREEZE will still
endup freezing the port, which will later cause a reset. And we still endup
calling the port suspend op and ata_acpi_set_state(), which seems to be doing
nothing for freeze...

So I wonder if a simpler approach would not be to simply remove the
ata_port_pm_freeze() method entirely and do nothing for freeze events ?

> +               /* 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




[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux