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 chipsetsto the 4.14-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
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 usefor mobile / laptop variants of chipsets / "South Bridges".The value set has the following meanings:0 => Keep firmware settings1 => Maximum performance2 => Medium power3 => Medium power with Device Initiated PM enabled4 => Minimum powerNote "Minimum power" is known to cause issues, including diskcorruption, 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