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@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx> 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