Re: libahci driver and power switching HDD on newer kernels

[Niklas Cassel <cassel@kernel.org>]

On Thu, Oct 03, 2024 at 07:47:41AM +0900, Damien Le Moal wrote:
> 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@kernel.org>
> > 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@kernel.org>
> > ---
> >  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...

Hello Damien,

Hibernation is done by three consecutive PM events, in the following order:
1) PM_EVENT_FREEZE
2) PM_EVENT_THAW
3) PM_EVENT_HIBERNATE

Hibernate before commit aa3998dbeb3a ("ata: libata-scsi: Disable scsi
device manage_system_start_stop"):
-On event PM_EVENT_FREEZE:
 ata_eh_handle_port_suspend() would call ata_eh_freeze_port() and then
 call ap->ops->port_suspend().
-On event PM_EVENT_THAW:
 ata_port_resume() would trigger EH with ATA_EH_RESET, which triggers a
 all to ata_eh_reset(), which will thaw the port, and then later
 ata_eh_handle_port_resume() would call ap->ops->port_resume().
-On event PM_EVENT_HIBERNATE:
 same as PM_EVENT_FREEZE.

After commit aa3998dbeb3a ("ata: libata-scsi: Disable scsi device
manage_system_start_stop"), on event PM_EVENT_FREEZE, and on event
PM_EVENT_HIBERNATE, ata_eh_handle_port_suspend() would call
ata_eh_freeze_port() and then call ap->ops->port_suspend(),
but it would also call ata_dev_power_set_standby() to spin down the disk.

So what my propsed patch does is simply to restore
ata_eh_handle_port_suspend() to the behavior before commit aa3998dbeb3a
for event PM_EVENT_FREEZE, but for event PM_EVENT_HIBERNATE,
ata_eh_handle_port_suspend() will continue to spin down the disk.


> 
> 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 ?

I do not think that is a good option, as:
https://docs.kernel.org/driver-api/pm/devices.html#entering-hibernation
clearly states that:
"The ->freeze methods should quiesce the device so that it doesn’t
generate IRQs or DMA."

That is what we do when calling ata_eh_freeze_port().

I think my propsed patch is the simplest thing that we can do to
restore the behavior of ata_eh_handle_port_suspend() for event
PM_EVENT_FREEZE, as it was before commit aa3998dbeb3a.

Thus, I will send out this proposed patch as a real patch shortly.


Kind regards,
Niklas