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

[Hans de Goede <hdegoede@redhat.com>]

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.

Regards,

Hans