* [PATCH 0/2] ata: libahci: devslp fixes @ 2018-07-02 19:01 Srinivas Pandruvada 2018-07-02 19:01 ` [PATCH 1/2] ata: libahci: Correct setting of DEVSLP register Srinivas Pandruvada ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Srinivas Pandruvada @ 2018-07-02 19:01 UTC (permalink / raw) To: tj, hdegoede; +Cc: rjw, alan.cox, linux-ide, linux-kernel, Srinivas Pandruvada Some minor fixes to be able to correctly set devslp register to optimize power. Srinivas Pandruvada (2): ata: libahci: Correct setting of DEVSLP register ata: libahci: Allow reconfigure of DEVSLP register drivers/ata/libahci.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] ata: libahci: Correct setting of DEVSLP register 2018-07-02 19:01 [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada @ 2018-07-02 19:01 ` Srinivas Pandruvada [not found] ` <CAAtuYK9zXJHhJ8_aRqfqOPJJ9L29patDA9RuUs9r+GJVZMCy=Q@mail.gmail.com> 2018-07-02 19:01 ` [PATCH 2/2] ata: libahci: Allow reconfigure " Srinivas Pandruvada 2018-07-30 15:15 ` [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada 2 siblings, 1 reply; 15+ messages in thread From: Srinivas Pandruvada @ 2018-07-02 19:01 UTC (permalink / raw) To: tj, hdegoede; +Cc: rjw, alan.cox, linux-ide, linux-kernel, Srinivas Pandruvada We have seen that on some platforms, SATA device never show any DEVSLP residency. This prevent power gating of SATA IP, which prevent system to transition to low power mode in systems with SLP_S0 aka modern standby systems. The PHY logic is off only in DEVSLP not in slumber. Reference: https://www.intel.com/content/dam/www/public/us/en/documents/datasheets /332995-skylake-i-o-platform-datasheet-volume-1.pdf Section 28.7.6.1 Here driver is trying to do read-modify-write the devslp register. But not resetting the bits for which this driver will modify values (DITO, MDAT and DETO). So simply reset those bits before updating to new values. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/ata/libahci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 965842a08743..f6795d261869 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -2159,6 +2159,8 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep) deto = 20; } + /* Make dito, mdat, deto bits to 0s */ + devslp &= ~GENMASK_ULL(24, 2); devslp |= ((dito << PORT_DEVSLP_DITO_OFFSET) | (mdat << PORT_DEVSLP_MDAT_OFFSET) | (deto << PORT_DEVSLP_DETO_OFFSET) | -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
[parent not found: <CAAtuYK9zXJHhJ8_aRqfqOPJJ9L29patDA9RuUs9r+GJVZMCy=Q@mail.gmail.com>]
* Re: [PATCH 1/2] ata: libahci: Correct setting of DEVSLP register [not found] ` <CAAtuYK9zXJHhJ8_aRqfqOPJJ9L29patDA9RuUs9r+GJVZMCy=Q@mail.gmail.com> @ 2018-07-17 3:43 ` Srinivas Pandruvada 0 siblings, 0 replies; 15+ messages in thread From: Srinivas Pandruvada @ 2018-07-17 3:43 UTC (permalink / raw) To: AceLan Kao; +Cc: tj, hdegoede, rjw, alan.cox, linux-ide, linux-kernel On Tue, 2018-07-17 at 11:26 +0800, AceLan Kao wrote: > Tested-by: AceLan Kao <acelan.kao@canonical.com> Thanks Kao for the test. -Srinivas > > The patches help the power consumption a little bit on my test > system, > and no obvious issue I can observe. > > 2018-07-03 3:01 GMT+08:00 Srinivas Pandruvada <srinivas.pandruvada@li > nux.intel.com>: > > We have seen that on some platforms, SATA device never show any > > DEVSLP > > residency. This prevent power gating of SATA IP, which prevent > > system > > to transition to low power mode in systems with SLP_S0 aka modern > > standby systems. The PHY logic is off only in DEVSLP not in > > slumber. > > Reference: > > https://www.intel.com/content/dam/www/public/us/en/documents/datash > > eets > > /332995-skylake-i-o-platform-datasheet-volume-1.pdf > > Section 28.7.6.1 > > > > Here driver is trying to do read-modify-write the devslp register. > > But > > not resetting the bits for which this driver will modify values > > (DITO, > > MDAT and DETO). So simply reset those bits before updating to new > > values. > > > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel > > .com> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/ata/libahci.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > > index 965842a08743..f6795d261869 100644 > > --- a/drivers/ata/libahci.c > > +++ b/drivers/ata/libahci.c > > @@ -2159,6 +2159,8 @@ static void ahci_set_aggressive_devslp(struct > > ata_port *ap, bool sleep) > > deto = 20; > > } > > > > + /* Make dito, mdat, deto bits to 0s */ > > + devslp &= ~GENMASK_ULL(24, 2); > > devslp |= ((dito << PORT_DEVSLP_DITO_OFFSET) | > > (mdat << PORT_DEVSLP_MDAT_OFFSET) | > > (deto << PORT_DEVSLP_DETO_OFFSET) | > > -- > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] ata: libahci: Allow reconfigure of DEVSLP register 2018-07-02 19:01 [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada 2018-07-02 19:01 ` [PATCH 1/2] ata: libahci: Correct setting of DEVSLP register Srinivas Pandruvada @ 2018-07-02 19:01 ` Srinivas Pandruvada 2018-07-30 15:15 ` [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada 2 siblings, 0 replies; 15+ messages in thread From: Srinivas Pandruvada @ 2018-07-02 19:01 UTC (permalink / raw) To: tj, hdegoede; +Cc: rjw, alan.cox, linux-ide, linux-kernel, Srinivas Pandruvada There are two modes in which DEVSLP can be entered. The OS initiated or hardware autonomous. In hardware autonomous mode, BIOS configures the AHCI controller and the device to enable DEVSLP. But they may not be ideal for all cases. So in this case, OS should be able to reconfigure DEVSLP register. Currently if the DEVSLP is already enabled, we can't set again as it will simply return. There are some systems where the firmware is setting high DITO by default, in this case we can't modify here to correct settings. With the default in several seconds, we are not able to transition to DEVSLP. This change will allow reconfiguration of devslp register if DITO is different. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/ata/libahci.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index f6795d261869..432ac2f5de34 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -2104,7 +2104,7 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep) struct ahci_host_priv *hpriv = ap->host->private_data; void __iomem *port_mmio = ahci_port_base(ap); struct ata_device *dev = ap->link.device; - u32 devslp, dm, dito, mdat, deto; + u32 devslp, dm, dito, mdat, deto, dito_conf; int rc; unsigned int err_mask; @@ -2128,8 +2128,15 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep) return; } - /* device sleep was already enabled */ - if (devslp & PORT_DEVSLP_ADSE) + dm = (devslp & PORT_DEVSLP_DM_MASK) >> PORT_DEVSLP_DM_OFFSET; + dito = devslp_idle_timeout / (dm + 1); + if (dito > 0x3ff) + dito = 0x3ff; + + dito_conf = (devslp >> PORT_DEVSLP_DITO_OFFSET) & 0x3FF; + + /* device sleep was already enabled and same dito */ + if ((devslp & PORT_DEVSLP_ADSE) && (dito_conf == dito)) return; /* set DITO, MDAT, DETO and enable DevSlp, need to stop engine first */ @@ -2137,11 +2144,6 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep) if (rc) return; - dm = (devslp & PORT_DEVSLP_DM_MASK) >> PORT_DEVSLP_DM_OFFSET; - dito = devslp_idle_timeout / (dm + 1); - if (dito > 0x3ff) - dito = 0x3ff; - /* Use the nominal value 10 ms if the read MDAT is zero, * the nominal value of DETO is 20 ms. */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2018-07-02 19:01 [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada 2018-07-02 19:01 ` [PATCH 1/2] ata: libahci: Correct setting of DEVSLP register Srinivas Pandruvada 2018-07-02 19:01 ` [PATCH 2/2] ata: libahci: Allow reconfigure " Srinivas Pandruvada @ 2018-07-30 15:15 ` Srinivas Pandruvada 2018-07-30 15:22 ` Tejun Heo 2 siblings, 1 reply; 15+ messages in thread From: Srinivas Pandruvada @ 2018-07-30 15:15 UTC (permalink / raw) To: tj, hdegoede; +Cc: rjw, alan.cox, linux-ide, linux-kernel Hi Tejan, On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada wrote: > Some minor fixes to be able to correctly set devslp register > to optimize power. > > Srinivas Pandruvada (2): > ata: libahci: Correct setting of DEVSLP register > ata: libahci: Allow reconfigure of DEVSLP register > Are you applying this series? Thanks, Srinivas > drivers/ata/libahci.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2018-07-30 15:15 ` [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada @ 2018-07-30 15:22 ` Tejun Heo 2018-07-30 15:26 ` Hans de Goede 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2018-07-30 15:22 UTC (permalink / raw) To: Srinivas Pandruvada; +Cc: hdegoede, rjw, alan.cox, linux-ide, linux-kernel On Mon, Jul 30, 2018 at 08:15:47AM -0700, Srinivas Pandruvada wrote: > Hi Tejan, > > On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada wrote: > > Some minor fixes to be able to correctly set devslp register > > to optimize power. > > > > Srinivas Pandruvada (2): > > ata: libahci: Correct setting of DEVSLP register > > ata: libahci: Allow reconfigure of DEVSLP register > > > Are you applying this series? I was waiting for Hans's reviews. Hans, what do you think? Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2018-07-30 15:22 ` Tejun Heo @ 2018-07-30 15:26 ` Hans de Goede 2018-07-30 17:33 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: Hans de Goede @ 2018-07-30 15:26 UTC (permalink / raw) To: Tejun Heo, Srinivas Pandruvada; +Cc: rjw, alan.cox, linux-ide, linux-kernel Hi, On 30-07-18 17:22, Tejun Heo wrote: > On Mon, Jul 30, 2018 at 08:15:47AM -0700, Srinivas Pandruvada wrote: >> Hi Tejan, >> >> On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada wrote: >>> Some minor fixes to be able to correctly set devslp register >>> to optimize power. >>> >>> Srinivas Pandruvada (2): >>> ata: libahci: Correct setting of DEVSLP register >>> ata: libahci: Allow reconfigure of DEVSLP register >>> >> Are you applying this series? > > I was waiting for Hans's reviews. Hans, what do you think? Ah I missed that this was another series. With the caveat that I do not really know that much about devslp, both patches seem sensible to me, so both are: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2018-07-30 15:26 ` Hans de Goede @ 2018-07-30 17:33 ` Tejun Heo 2019-03-07 20:27 ` Gwendal Grignou 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2018-07-30 17:33 UTC (permalink / raw) To: Hans de Goede; +Cc: Srinivas Pandruvada, rjw, alan.cox, linux-ide, linux-kernel On Mon, Jul 30, 2018 at 05:26:45PM +0200, Hans de Goede wrote: > Hi, > > On 30-07-18 17:22, Tejun Heo wrote: > >On Mon, Jul 30, 2018 at 08:15:47AM -0700, Srinivas Pandruvada wrote: > >>Hi Tejan, > >> > >>On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada wrote: > >>>Some minor fixes to be able to correctly set devslp register > >>>to optimize power. > >>> > >>>Srinivas Pandruvada (2): > >>> ata: libahci: Correct setting of DEVSLP register > >>> ata: libahci: Allow reconfigure of DEVSLP register > >>> > >>Are you applying this series? > > > >I was waiting for Hans's reviews. Hans, what do you think? > > Ah I missed that this was another series. With the caveat that > I do not really know that much about devslp, both patches > seem sensible to me, so both are: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Applied 1-2 to libata/for-4.19. Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2018-07-30 17:33 ` Tejun Heo @ 2019-03-07 20:27 ` Gwendal Grignou 2019-03-07 20:37 ` Hans de Goede 0 siblings, 1 reply; 15+ messages in thread From: Gwendal Grignou @ 2019-03-07 20:27 UTC (permalink / raw) To: Srinivas Pandruvada, Tejun Heo Cc: Hans de Goede, Rafael J. Wysocki, alan.cox, IDE/ATA development list, Linux Kernel, rajatja 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. 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? Gwendal. On Mon, Jul 30, 2018 at 10:33 AM Tejun Heo <tj@kernel.org> wrote: > > On Mon, Jul 30, 2018 at 05:26:45PM +0200, Hans de Goede wrote: > > Hi, > > > > On 30-07-18 17:22, Tejun Heo wrote: > > >On Mon, Jul 30, 2018 at 08:15:47AM -0700, Srinivas Pandruvada wrote: > > >>Hi Tejan, > > >> > > >>On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada wrote: > > >>>Some minor fixes to be able to correctly set devslp register > > >>>to optimize power. > > >>> > > >>>Srinivas Pandruvada (2): > > >>> ata: libahci: Correct setting of DEVSLP register > > >>> ata: libahci: Allow reconfigure of DEVSLP register > > >>> > > >>Are you applying this series? > > > > > >I was waiting for Hans's reviews. Hans, what do you think? > > > > Ah I missed that this was another series. With the caveat that > > I do not really know that much about devslp, both patches > > seem sensible to me, so both are: > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Applied 1-2 to libata/for-4.19. > > Thanks. > > -- > tejun > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2019-03-07 20:27 ` Gwendal Grignou @ 2019-03-07 20:37 ` Hans de Goede 2019-03-07 23:07 ` Rajat Jain 0 siblings, 1 reply; 15+ messages in thread From: Hans de Goede @ 2019-03-07 20:37 UTC (permalink / raw) To: Gwendal Grignou, Srinivas Pandruvada, Tejun Heo Cc: Rafael J. Wysocki, alan.cox, IDE/ATA development list, Linux Kernel, rajatja 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) ? 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. Regards, Hans > > Gwendal. > > > On Mon, Jul 30, 2018 at 10:33 AM Tejun Heo <tj@kernel.org> wrote: >> >> On Mon, Jul 30, 2018 at 05:26:45PM +0200, Hans de Goede wrote: >>> Hi, >>> >>> On 30-07-18 17:22, Tejun Heo wrote: >>>> On Mon, Jul 30, 2018 at 08:15:47AM -0700, Srinivas Pandruvada wrote: >>>>> Hi Tejan, >>>>> >>>>> On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada wrote: >>>>>> Some minor fixes to be able to correctly set devslp register >>>>>> to optimize power. >>>>>> >>>>>> Srinivas Pandruvada (2): >>>>>> ata: libahci: Correct setting of DEVSLP register >>>>>> ata: libahci: Allow reconfigure of DEVSLP register >>>>>> >>>>> Are you applying this series? >>>> >>>> I was waiting for Hans's reviews. Hans, what do you think? >>> >>> Ah I missed that this was another series. With the caveat that >>> I do not really know that much about devslp, both patches >>> seem sensible to me, so both are: >>> >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> >> Applied 1-2 to libata/for-4.19. >> >> Thanks. >> >> -- >> tejun >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-ide" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2019-03-07 20:37 ` Hans de Goede @ 2019-03-07 23:07 ` Rajat Jain 2019-03-08 0:04 ` Srinivas Pandruvada 0 siblings, 1 reply; 15+ messages in thread From: Rajat Jain @ 2019-03-07 23:07 UTC (permalink / raw) To: Hans de Goede Cc: Gwendal Grignou, Srinivas Pandruvada, Tejun Heo, Rafael J. Wysocki, alan.cox, IDE/ATA development list, Linux Kernel, Rajat Jain Hello, On Thu, Mar 7, 2019 at 12:37 PM Hans de Goede <hdegoede@redhat.com> 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 I 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? Or may be there are issues that this causes, that we're not aware of? Can you please provide us some pointers? Thanks, Rajat > > Regards, > > Hans > > > > > > > Gwendal. > > > > > > On Mon, Jul 30, 2018 at 10:33 AM Tejun Heo <tj@kernel.org> wrote: > >> > >> On Mon, Jul 30, 2018 at 05:26:45PM +0200, Hans de Goede wrote: > >>> Hi, > >>> > >>> On 30-07-18 17:22, Tejun Heo wrote: > >>>> On Mon, Jul 30, 2018 at 08:15:47AM -0700, Srinivas Pandruvada wrote: > >>>>> Hi Tejan, > >>>>> > >>>>> On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada wrote: > >>>>>> Some minor fixes to be able to correctly set devslp register > >>>>>> to optimize power. > >>>>>> > >>>>>> Srinivas Pandruvada (2): > >>>>>> ata: libahci: Correct setting of DEVSLP register > >>>>>> ata: libahci: Allow reconfigure of DEVSLP register > >>>>>> > >>>>> Are you applying this series? > >>>> > >>>> I was waiting for Hans's reviews. Hans, what do you think? > >>> > >>> Ah I missed that this was another series. With the caveat that > >>> I do not really know that much about devslp, both patches > >>> seem sensible to me, so both are: > >>> > >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > >> > >> Applied 1-2 to libata/for-4.19. > >> > >> Thanks. > >> > >> -- > >> tejun > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-ide" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2019-03-07 23:07 ` Rajat Jain @ 2019-03-08 0:04 ` Srinivas Pandruvada 2019-03-08 8:57 ` Hans de Goede 0 siblings, 1 reply; 15+ messages in thread From: Srinivas Pandruvada @ 2019-03-08 0:04 UTC (permalink / raw) To: Rajat Jain, Hans de Goede Cc: Gwendal Grignou, Tejun Heo, Rafael J. Wysocki, alan.cox, IDE/ATA development list, Linux Kernel, Rajat Jain 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@redhat.com> > 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 I 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. Thanks, Srinivas > > Or may be there are issues that this causes, that we're not aware of? > Can you please provide us some pointers? > > Thanks, > > Rajat > > > > > Regards, > > > > Hans > > > > > > > > > > > > Gwendal. > > > > > > > > > On Mon, Jul 30, 2018 at 10:33 AM Tejun Heo <tj@kernel.org> wrote: > > > > > > > > On Mon, Jul 30, 2018 at 05:26:45PM +0200, Hans de Goede wrote: > > > > > Hi, > > > > > > > > > > On 30-07-18 17:22, Tejun Heo wrote: > > > > > > On Mon, Jul 30, 2018 at 08:15:47AM -0700, Srinivas > > > > > > Pandruvada wrote: > > > > > > > Hi Tejan, > > > > > > > > > > > > > > On Mon, 2018-07-02 at 12:01 -0700, Srinivas Pandruvada > > > > > > > wrote: > > > > > > > > Some minor fixes to be able to correctly set devslp > > > > > > > > register > > > > > > > > to optimize power. > > > > > > > > > > > > > > > > Srinivas Pandruvada (2): > > > > > > > > ata: libahci: Correct setting of DEVSLP register > > > > > > > > ata: libahci: Allow reconfigure of DEVSLP register > > > > > > > > > > > > > > > > > > > > > > Are you applying this series? > > > > > > > > > > > > I was waiting for Hans's reviews. Hans, what do you think? > > > > > > > > > > Ah I missed that this was another series. With the caveat > > > > > that > > > > > I do not really know that much about devslp, both patches > > > > > seem sensible to me, so both are: > > > > > > > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > > > > > > > Applied 1-2 to libata/for-4.19. > > > > > > > > Thanks. > > > > > > > > -- > > > > tejun > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe > > > > linux-ide" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at > > > > http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2019-03-08 0:04 ` Srinivas Pandruvada @ 2019-03-08 8:57 ` Hans de Goede 2019-03-08 19:29 ` Rajat Jain 0 siblings, 1 reply; 15+ messages in thread From: Hans de Goede @ 2019-03-08 8:57 UTC (permalink / raw) To: Srinivas Pandruvada, Rajat Jain Cc: Gwendal Grignou, Tejun Heo, Rafael J. Wysocki, alan.cox, IDE/ATA development list, Linux Kernel, Rajat Jain 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@redhat.com> >> 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 I 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. Right, or simply do not mess with the setting in the first place! 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 ? Regards, Hans ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2019-03-08 8:57 ` Hans de Goede @ 2019-03-08 19:29 ` Rajat Jain 2019-03-11 7:52 ` Gwendal Grignou 0 siblings, 1 reply; 15+ messages in thread From: Rajat Jain @ 2019-03-08 19:29 UTC (permalink / raw) To: Hans de Goede Cc: Srinivas Pandruvada, Gwendal Grignou, Tejun Heo, Rafael J. Wysocki, alan.cox, IDE/ATA development list, Linux Kernel, Rajat Jain On Fri, Mar 8, 2019 at 12:57 AM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> > >> 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 I 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! > > 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] ata: libahci: devslp fixes 2019-03-08 19:29 ` Rajat Jain @ 2019-03-11 7:52 ` Gwendal Grignou 0 siblings, 0 replies; 15+ messages in thread From: Gwendal Grignou @ 2019-03-11 7:52 UTC (permalink / raw) To: Rajat Jain Cc: Hans de Goede, Srinivas Pandruvada, Tejun Heo, Rafael J. Wysocki, alan.cox, IDE/ATA development list, Linux Kernel, Rajat Jain On Fri, Mar 8, 2019 at 11:30 AM Rajat Jain <rajatja@google.com> wrote: > > On Fri, Mar 8, 2019 at 12:57 AM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> > > >> 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-03-11 7:52 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-02 19:01 [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada 2018-07-02 19:01 ` [PATCH 1/2] ata: libahci: Correct setting of DEVSLP register Srinivas Pandruvada [not found] ` <CAAtuYK9zXJHhJ8_aRqfqOPJJ9L29patDA9RuUs9r+GJVZMCy=Q@mail.gmail.com> 2018-07-17 3:43 ` Srinivas Pandruvada 2018-07-02 19:01 ` [PATCH 2/2] ata: libahci: Allow reconfigure " Srinivas Pandruvada 2018-07-30 15:15 ` [PATCH 0/2] ata: libahci: devslp fixes Srinivas Pandruvada 2018-07-30 15:22 ` Tejun Heo 2018-07-30 15:26 ` Hans de Goede 2018-07-30 17:33 ` Tejun Heo 2019-03-07 20:27 ` Gwendal Grignou 2019-03-07 20:37 ` Hans de Goede 2019-03-07 23:07 ` Rajat Jain 2019-03-08 0:04 ` Srinivas Pandruvada 2019-03-08 8:57 ` Hans de Goede 2019-03-08 19:29 ` Rajat Jain 2019-03-11 7:52 ` Gwendal Grignou
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).