Re: [PATCH 0/2] ata: libahci: devslp fixes

[Gwendal Grignou]

On Fri, Mar 8, 2019 at 11:30 AM Rajat Jain <rajatja@xxxxxxxxxx> wrote:
>
> On Fri, Mar 8, 2019 at 12:57 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On 08-03-19 01:04, Srinivas Pandruvada wrote:
> > > On Thu, 2019-03-07 at 15:07 -0800, Rajat Jain wrote:
> > >> Hello,
> > >>
> > >> On Thu, Mar 7, 2019 at 12:37 PM Hans de Goede <hdegoede@xxxxxxxxxx>
> > >> wrote:
> > >>>
> > >>> Hi,
> > >>>
> > >>> On 07-03-19 21:27, Gwendal Grignou wrote:
> > >>>> Srinivas,
> > >>>>
> > >>>> I am looking at problem on a laptop machine that suspends to
> > >>>> S01x, but
> > >>>> link_management is set to max_performance, because the machine is
> > >>>> connected to a charger.
> > >>>
> > >>> What is setting it to max_performance when charging?  I assume
> > >>> chrome-os is
> > >>> running something in userspace to do this (like TLP, but I guess
> > >>> you are not
> > >>> using TLP) ?
> > >>
> > >> Yes, we have a udev script that does this.
> > >>
> > >>>
> > >>> Have you run benchmarks with max_performance vs the default?
> > >>> I seriously doubt there will be a significant difference, esp.
> > >>> with a chrome-os style workload.
> > >>>
> > >>>> Given DVLSP must be set before the laptop suspends ["""One of the
> > >>>> requirement for modern x86 system to enter lowest power
> > >>>> mode  (SLP_S0)
> > >>>> is SATA IP block to be off."""], the machine never reaches S01x.
> > >>>> Does it make sense to change the target_lpm_policy at suspend
> > >>>> (ata_port_suspend()) to min_power and bring it back to the
> > >>>> original
> > >>>> value on resume?
> > >>>
> > >>> If userspace messes with the setting, then userspace should also
> > >>> put it back before suspending...
> > >>>
> > >>> The upstream kernel's default behavior is to have the target level
> > >>> set
> > >>> to a fixed level independent of the charging state. Could it be
> > >>> this
> > >>> fixed level is actually max-performance ? If that is the default
> > >>> the
> > >>> kernel comes up with, that would indicate a kernel bug.
> > >>
> > >> Side note: max-performance indeed can be the default forced by the
> > >> kernel for some (broken) SATA devices:
> > >>
> > >>          if (dev->horkage & ATA_HORKAGE_NOLPM) {
> > >>                  ata_dev_warn(dev, "LPM support broken, forcing
> > >> max_power\n");
> > >>                  dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER;
> > >>          }
> > >>
> > >> So definitely these systems won't be able to go into S0ix today.
> > >>
> > >> But Idrivers/ata/libata-core.c think the main idea that we are asking is:
> > >>
> > >> 1) Yes, we acknowledge that the userspace has set it max-performance.
> > >>
> > >> 2) However, given that the kernel already knows that:
> > >>         - while in suspend, there is no real value in retaining the
> > >> max-performance.
> > >>         - On the contrary, we know system will fail to go into lower
> > >> power mode because of max-suspend.
> > >>
> > >> 3) Does it not make sense to use this knowledge and switch to
> > >> min_power when we are actually going to suspend (even if user
> > >> specified max-performance), and restore max-performance on resume?
> > >
> > > It is all about regressions. Hence we added multiple conditions for
> > > setting default to min power.
> > > It may cause issues for some SATAs, which may not recover once enters
> > > slumber or DEVSLP. There is also case where user having issues with
> > > default LPM policy hence he changed policy to max performance. We can't
> > > detect that.
> > > So it will be much safer if user space change policy to default before
> > > calling suspend.
>
> Now I understand, and agree.
>
> >
> > Right, or simply do not mess with the setting in the first place!
Reading the kernel code further, I understand the intent to no setting
link power from user space anymore.
Before 4.16, the link was set to max_performance by the kernel; we
have script to set min_power, but that was done indiscriminately, and
breaks devices that need max_performance.
I added one similar to intel-sata-powermgmt  (at
https://github.com/rickysarraf/laptop-mode-tools/blob/lmt-upstream/usr/share/laptop-mode-tools/modules/intel-sata-powermgmt),
but that was a mistake for kernel 4.19.

For future devices, we will  make sure to not use devices with horkage
ATA_HORKAGE_NOLPM or that require max_performance (see
http://preston4tw.blogspot.com/2015/02/transcend-mts400-ssd-power-saving.html).
Thanks for your help,

Gwendal.


> >
> > I noticed you did not answer this part of my original reply:
> >
> > "Have you run benchmarks with max_performance vs the default?
> > I seriously doubt there will be a significant difference, esp.
> > with a chrome-os style workload."
> >
> > I seriously doubt the max-performance setting has a user
> > noticeable impact on performance. So I repeat has someone
> > actually measured this with real-world chrome-os workloads ?
>
> The reason we're setting it to max-performance is not really
> performance - but as a work around to a broken SATA device (Transcend
> SSD) that can't handle DEVSLP in active state (similar to what
> Srinivas said). Putting it to min_power while suspended was OK because
> it'd be in reset anyway at that time. Thanks for your explanations and
> help.
>
> Rajat
>
>
> >
> > Regards,
> >
> > Hans
> >