| Message ID | 20180328112104.14269-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Unstable] UBUNTU: [Config] CONFIG_SATA_MOBILE_LPM_POLICY=3 | expand |
On Wed, Mar 28, 2018 at 07:21:04PM +0800, Kai-Heng Feng wrote: > BugLink: https://bugs.launchpad.net/bugs/1759547 > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > debian.master/config/config.common.ubuntu | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/debian.master/config/config.common.ubuntu b/debian.master/config/config.common.ubuntu > index e09a65ef0a1f..98a828b82f30 100644 > --- a/debian.master/config/config.common.ubuntu > +++ b/debian.master/config/config.common.ubuntu > @@ -7691,7 +7691,7 @@ CONFIG_SATA_DWC=m > CONFIG_SATA_DWC_OLD_DMA=y > CONFIG_SATA_HIGHBANK=y > CONFIG_SATA_INIC162X=m > -CONFIG_SATA_MOBILE_LPM_POLICY=0 > +CONFIG_SATA_MOBILE_LPM_POLICY=3 From the commit message of the patch adding this: Also enabling LPM by default is not entirely without risk of regressions. At least min_power is known to cause issues with some disks, including some reports of data corruption. Is there any risk of these issues with the value you've chosen here?
> On Mar 29, 2018, at 1:32 AM, Seth Forshee <seth.forshee@canonical.com> > wrote: > > On Wed, Mar 28, 2018 at 07:21:04PM +0800, Kai-Heng Feng wrote: >> BugLink: https://bugs.launchpad.net/bugs/1759547 >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> debian.master/config/config.common.ubuntu | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/debian.master/config/config.common.ubuntu >> b/debian.master/config/config.common.ubuntu >> index e09a65ef0a1f..98a828b82f30 100644 >> --- a/debian.master/config/config.common.ubuntu >> +++ b/debian.master/config/config.common.ubuntu >> @@ -7691,7 +7691,7 @@ CONFIG_SATA_DWC=m >> CONFIG_SATA_DWC_OLD_DMA=y >> CONFIG_SATA_HIGHBANK=y >> CONFIG_SATA_INIC162X=m >> -CONFIG_SATA_MOBILE_LPM_POLICY=0 >> +CONFIG_SATA_MOBILE_LPM_POLICY=3 > > From the commit message of the patch adding this: > > Also enabling LPM by default is not entirely without risk of > regressions. At least min_power is known to cause issues with some > disks, including some reports of data corruption. > > Is there any risk of these issues with the value you've chosen here? Yes, users in [1] can't mount rootfs with min_power. I wrote a patch to quirk their devices to use med_power_with_dipm (i.e. CONFIG_SATA_MOBILE_LPM_POLICY=3), unfortunately their disk still have issues with this setting. So yes, some disks are buggy under both min_power and med_power_with_dipm. Since LPM can be set from userspace (e.g. laptop-mode-tools, TLP, powertop), user will likely to get hit by this bug, one way or another. In a discussion with Hans [2], he mentioned that Fedora will use CONFIG_SATA_MOBILE_LPM_POLICY=3 from v4.15+, I think we should do the same. Also, one of our customer does want us to use a saner default for laptops. [1] https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1726930 [2] https://github.com/linrunner/TLP/issues/84#issuecomment-365929563 Kai-Heng
On Thu, Mar 29, 2018 at 12:00:44PM +0800, Kai Heng Feng wrote: > > > On Mar 29, 2018, at 1:32 AM, Seth Forshee <seth.forshee@canonical.com> > > wrote: > > > > On Wed, Mar 28, 2018 at 07:21:04PM +0800, Kai-Heng Feng wrote: > > > BugLink: https://bugs.launchpad.net/bugs/1759547 > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > --- > > > debian.master/config/config.common.ubuntu | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/debian.master/config/config.common.ubuntu > > > b/debian.master/config/config.common.ubuntu > > > index e09a65ef0a1f..98a828b82f30 100644 > > > --- a/debian.master/config/config.common.ubuntu > > > +++ b/debian.master/config/config.common.ubuntu > > > @@ -7691,7 +7691,7 @@ CONFIG_SATA_DWC=m > > > CONFIG_SATA_DWC_OLD_DMA=y > > > CONFIG_SATA_HIGHBANK=y > > > CONFIG_SATA_INIC162X=m > > > -CONFIG_SATA_MOBILE_LPM_POLICY=0 > > > +CONFIG_SATA_MOBILE_LPM_POLICY=3 > > > > From the commit message of the patch adding this: > > > > Also enabling LPM by default is not entirely without risk of > > regressions. At least min_power is known to cause issues with some > > disks, including some reports of data corruption. > > > > Is there any risk of these issues with the value you've chosen here? > > Yes, users in [1] can't mount rootfs with min_power. > > I wrote a patch to quirk their devices to use med_power_with_dipm (i.e. > CONFIG_SATA_MOBILE_LPM_POLICY=3), > unfortunately their disk still have issues with this setting. > > So yes, some disks are buggy under both min_power and med_power_with_dipm. > > Since LPM can be set from userspace (e.g. laptop-mode-tools, TLP, powertop), > user will likely to get hit by this bug, one way or another. > > In a discussion with Hans [2], he mentioned that Fedora will use > CONFIG_SATA_MOBILE_LPM_POLICY=3 from v4.15+, > I think we should do the same. > > Also, one of our customer does want us to use a saner default for laptops. Thanks, applied to unstable/master. Did you also intend to request this for bionic?
Seth Forshee <seth.forshee@canonical.com> wrote: > On Thu, Mar 29, 2018 at 12:00:44PM +0800, Kai Heng Feng wrote: >>> On Mar 29, 2018, at 1:32 AM, Seth Forshee <seth.forshee@canonical.com> >>> wrote: >>> >>> On Wed, Mar 28, 2018 at 07:21:04PM +0800, Kai-Heng Feng wrote: >>>> BugLink: https://bugs.launchpad.net/bugs/1759547 >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> --- >>>> debian.master/config/config.common.ubuntu | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/debian.master/config/config.common.ubuntu >>>> b/debian.master/config/config.common.ubuntu >>>> index e09a65ef0a1f..98a828b82f30 100644 >>>> --- a/debian.master/config/config.common.ubuntu >>>> +++ b/debian.master/config/config.common.ubuntu >>>> @@ -7691,7 +7691,7 @@ CONFIG_SATA_DWC=m >>>> CONFIG_SATA_DWC_OLD_DMA=y >>>> CONFIG_SATA_HIGHBANK=y >>>> CONFIG_SATA_INIC162X=m >>>> -CONFIG_SATA_MOBILE_LPM_POLICY=0 >>>> +CONFIG_SATA_MOBILE_LPM_POLICY=3 >>> >>> From the commit message of the patch adding this: >>> >>> Also enabling LPM by default is not entirely without risk of >>> regressions. At least min_power is known to cause issues with some >>> disks, including some reports of data corruption. >>> >>> Is there any risk of these issues with the value you've chosen here? >> >> Yes, users in [1] can't mount rootfs with min_power. >> >> I wrote a patch to quirk their devices to use med_power_with_dipm (i.e. >> CONFIG_SATA_MOBILE_LPM_POLICY=3), >> unfortunately their disk still have issues with this setting. >> >> So yes, some disks are buggy under both min_power and med_power_with_dipm. >> >> Since LPM can be set from userspace (e.g. laptop-mode-tools, TLP, >> powertop), >> user will likely to get hit by this bug, one way or another. >> >> In a discussion with Hans [2], he mentioned that Fedora will use >> CONFIG_SATA_MOBILE_LPM_POLICY=3 from v4.15+, >> I think we should do the same. >> >> Also, one of our customer does want us to use a saner default for laptops. > > Thanks, applied to unstable/master. Did you also intend to request this > for bionic? I think it's too risky for an LTS release, do it in the next release should be more manageable.
diff --git a/debian.master/config/config.common.ubuntu b/debian.master/config/config.common.ubuntu index e09a65ef0a1f..98a828b82f30 100644 --- a/debian.master/config/config.common.ubuntu +++ b/debian.master/config/config.common.ubuntu @@ -7691,7 +7691,7 @@ CONFIG_SATA_DWC=m CONFIG_SATA_DWC_OLD_DMA=y CONFIG_SATA_HIGHBANK=y CONFIG_SATA_INIC162X=m -CONFIG_SATA_MOBILE_LPM_POLICY=0 +CONFIG_SATA_MOBILE_LPM_POLICY=3 CONFIG_SATA_MV=m CONFIG_SATA_NV=m CONFIG_SATA_PMP=y
BugLink: https://bugs.launchpad.net/bugs/1759547 Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- debian.master/config/config.common.ubuntu | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)