Re: Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Feb 14, 2018 at 05:45:33PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 14-02-18 16:48, Greg KH wrote:
> > On Wed, Feb 14, 2018 at 04:03:17PM +0100, Hans de Goede wrote:
> > > Hi All,
> > > 
> > > On 14-02-18 15:25, gregkh@linuxfoundation.org wrote:
> > > > 
> > > > This is a note to let you know that I've just added the patch titled
> > > > 
> > > >       ahci: Allow setting a default LPM policy for mobile chipsets
> > > > 
> > > > to the 4.14-stable tree which can be found at:
> > > >       http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > > > 
> > > > The filename of the patch is:
> > > >        ahci-allow-setting-a-default-lpm-policy-for-mobile-chipsets.patch
> > > > and it can be found in the queue-4.14 subdirectory.
> > > > 
> > > > If you, or anyone else, feels it should not be added to the stable tree,
> > > > please let <stable@vger.kernel.org> know about it.
> > > 
> > > I wonder how this ended up on the patches-for-stable list? Torvald's
> > > master commits have neither a Cc: stable or a Fixes tag.
> > > 
> > > By itself this series is harmless, until someone actually sets
> > > the new Kconfig option to something other then 0.
> > 
> > See my response to the 4.15.y patch for "how" this came to be merged.
> > 
> > > > +config SATA_MOBILE_LPM_POLICY
> > > > +	int "Default SATA Link Power Management policy for mobile chipsets"
> > > > +	range 0 4
> > > > +	default 0
> > > > +	depends on SATA_AHCI
> > > > +	help
> > > > +	  Select the Default SATA Link Power Management (LPM) policy to use
> > > > +	  for mobile / laptop variants of chipsets / "South Bridges".
> > > > +
> > > > +	  The value set has the following meanings:
> > > > +		0 => Keep firmware settings
> > > > +		1 => Maximum performance
> > > > +		2 => Medium power
> > > > +		3 => Medium power with Device Initiated PM enabled
> > > > +		4 => Minimum power
> > > > +
> > > > +	  Note "Minimum power" is known to cause issues, including disk
> > > > +	  corruption, with some disks and should not be used.
> > > > +
> > > 
> > > AFAIK 4.14 and older do not have med_power_with_dipm, so setting this
> > > to 3 will lead to a setting of min_power. Which leads me to my worry
> > > about this series, as said in itself it is harmless, but as the help
> > > text says setting it to 4 (*) is dangerous. Actually this week I've
> > > received my first bug report that even med_power_with_dipm is causing
> > > issues (disconnects) with some devices. I'm working with the reporter
> > > an a blacklist entry for the specific SSD in question, but given that
> > > we're still figuring this out for master I wonder how wise it is to
> > > add these to stable, esp. since stable lacks med_power_with_dipm.
> > > 
> > > At a minimum we should fixup the help-text for 4.14 and older
> > > (4.15 does have med_power_with_dipm).
> > 
> > What would the text be for 4.14.y and older?
> 
> Drop the:
> 		3 => Medium power with Device Initiated PM enabled
> 
> Line and:
> 		4 => Minimum power
> Becomes:
> 		3 => Minimum power
> 
> Also "range 0 4" should become "range 0 3"
> 
> > I'll be glad to fix that
> > up.  Or I can drop the whole thing and fit in the device id update "by
> > hand", if you think this shouldn't go to the stable trees.
> 
> I would prefer for this to not go to the stable trees. The problem is
> that if people enable this and set it to "Minimum power" combined
> with say a Crucial_CT525MX300 SSD then this is know to cause data-
> corruption, which is not just a regression but one of the worst
> kind of regressions. Note people can already get the same result
> (shoot themselves in the foot) by using powertop --auto-tune,
> or TLP, or setting this manually through sysfs. I really wonder
> if the stable series is a good place to give people one more
> way to shoot themselves in the foot though and I'm worried that
> some distro kernel maintainer will see this new option in a stable
> update and sets it to Minimum power, which would be quite bad.
> 
> As said before I've just received my first bug-report of this
> causing issues even with the more conservative "med_power_with_dipm"
> option, which is known to e.g. not cause issues on that same
> Crucial_CT525MX300 SSD. So I will likely be submitting some
> ATA_HORKAGE_NOLPM quirk patches to tj soon. TL;DR: I really
> believe this is too adventurous for the stable kernels.

Ok, I've now dropped this patch entirely from all of the stable trees,
sorry for the noise.  The fix-up to get the new device ids added was
really trivial, I should have just done that first :(

greg k-h