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