* Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree
@ 2018-02-14 14:25 gregkh
2018-02-14 15:03 ` Hans de Goede
0 siblings, 1 reply; 5+ messages in thread
From: gregkh @ 2018-02-14 14:25 UTC (permalink / raw)
To: hdegoede, gregkh, tj; +Cc: stable, stable-commits
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.
>From ebb82e3c79d2a956366d0848304a53648bd6350b Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 11 Dec 2017 17:52:16 +0100
Subject: ahci: Allow setting a default LPM policy for mobile chipsets
From: Hans de Goede <hdegoede@redhat.com>
commit ebb82e3c79d2a956366d0848304a53648bd6350b upstream.
On many laptops setting a different LPM policy then unknown /
max_performance can lead to power-savings of 1.0 - 1.5 Watts (when idle).
Modern ultrabooks idle around 6W (at 50% screen brightness), 1.0 - 1.5W
is a significant chunk of this.
There are some performance / latency costs to enabling LPM by default,
so it is desirable to make it possible to set a different LPM policy
for mobile / laptop variants of chipsets / "South Bridges" vs their
desktop / server counterparts. 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.
This commits adds a new ahci.mobile_lpm_policy kernel cmdline option,
which defaults to a new SATA_MOBILE_LPM_POLICY Kconfig option so that
Linux distributions can choose to set a LPM policy for mobile chipsets
by default.
The reason to have both a kernel cmdline option and a Kconfig default
value for it, is to allow easy overriding of the default to allow
trouble-shooting without needing to rebuild the kernel.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/ata/Kconfig | 19 ++++++++++
drivers/ata/ahci.c | 97 ++++++++++++++++++++++++++++++----------------------
drivers/ata/ahci.h | 3 +
3 files changed, 78 insertions(+), 41 deletions(-)
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -92,6 +92,25 @@ config SATA_AHCI
If unsure, say N.
+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.
+
config SATA_AHCI_PLATFORM
tristate "Platform AHCI SATA support"
help
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -64,6 +64,7 @@ enum board_ids {
/* board IDs by feature in alphabetical order */
board_ahci,
board_ahci_ign_iferr,
+ board_ahci_mobile,
board_ahci_nomsi,
board_ahci_noncq,
board_ahci_nosntf,
@@ -139,6 +140,13 @@ static const struct ata_port_info ahci_p
.udma_mask = ATA_UDMA6,
.port_ops = &ahci_ops,
},
+ [board_ahci_mobile] = {
+ AHCI_HFLAGS (AHCI_HFLAG_IS_MOBILE),
+ .flags = AHCI_FLAG_COMMON,
+ .pio_mask = ATA_PIO4,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &ahci_ops,
+ },
[board_ahci_nomsi] = {
AHCI_HFLAGS (AHCI_HFLAG_NO_MSI),
.flags = AHCI_FLAG_COMMON,
@@ -251,13 +259,13 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(INTEL, 0x2924), board_ahci }, /* ICH9 */
{ PCI_VDEVICE(INTEL, 0x2925), board_ahci }, /* ICH9 */
{ PCI_VDEVICE(INTEL, 0x2927), board_ahci }, /* ICH9 */
- { PCI_VDEVICE(INTEL, 0x2929), board_ahci }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292a), board_ahci }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292b), board_ahci }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292c), board_ahci }, /* ICH9M */
- { PCI_VDEVICE(INTEL, 0x292f), board_ahci }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x2929), board_ahci_mobile }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292a), board_ahci_mobile }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292b), board_ahci_mobile }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292c), board_ahci_mobile }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x292f), board_ahci_mobile }, /* ICH9M */
{ PCI_VDEVICE(INTEL, 0x294d), board_ahci }, /* ICH9 */
- { PCI_VDEVICE(INTEL, 0x294e), board_ahci }, /* ICH9M */
+ { PCI_VDEVICE(INTEL, 0x294e), board_ahci_mobile }, /* ICH9M */
{ PCI_VDEVICE(INTEL, 0x502a), board_ahci }, /* Tolapai */
{ PCI_VDEVICE(INTEL, 0x502b), board_ahci }, /* Tolapai */
{ PCI_VDEVICE(INTEL, 0x3a05), board_ahci }, /* ICH10 */
@@ -267,9 +275,9 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(INTEL, 0x3b23), board_ahci }, /* PCH AHCI */
{ PCI_VDEVICE(INTEL, 0x3b24), board_ahci }, /* PCH RAID */
{ PCI_VDEVICE(INTEL, 0x3b25), board_ahci }, /* PCH RAID */
- { PCI_VDEVICE(INTEL, 0x3b29), board_ahci }, /* PCH M AHCI */
+ { PCI_VDEVICE(INTEL, 0x3b29), board_ahci_mobile }, /* PCH M AHCI */
{ PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
- { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci }, /* PCH M RAID */
+ { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_mobile }, /* PCH M RAID */
{ PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
{ PCI_VDEVICE(INTEL, 0x19b0), board_ahci }, /* DNV AHCI */
{ PCI_VDEVICE(INTEL, 0x19b1), board_ahci }, /* DNV AHCI */
@@ -292,9 +300,9 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(INTEL, 0x19cE), board_ahci }, /* DNV AHCI */
{ PCI_VDEVICE(INTEL, 0x19cF), board_ahci }, /* DNV AHCI */
{ PCI_VDEVICE(INTEL, 0x1c02), board_ahci }, /* CPT AHCI */
- { PCI_VDEVICE(INTEL, 0x1c03), board_ahci }, /* CPT M AHCI */
+ { PCI_VDEVICE(INTEL, 0x1c03), board_ahci_mobile }, /* CPT M AHCI */
{ PCI_VDEVICE(INTEL, 0x1c04), board_ahci }, /* CPT RAID */
- { PCI_VDEVICE(INTEL, 0x1c05), board_ahci }, /* CPT M RAID */
+ { PCI_VDEVICE(INTEL, 0x1c05), board_ahci_mobile }, /* CPT M RAID */
{ PCI_VDEVICE(INTEL, 0x1c06), board_ahci }, /* CPT RAID */
{ PCI_VDEVICE(INTEL, 0x1c07), board_ahci }, /* CPT RAID */
{ PCI_VDEVICE(INTEL, 0x1d02), board_ahci }, /* PBG AHCI */
@@ -303,28 +311,28 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(INTEL, 0x2826), board_ahci }, /* PBG RAID */
{ PCI_VDEVICE(INTEL, 0x2323), board_ahci }, /* DH89xxCC AHCI */
{ PCI_VDEVICE(INTEL, 0x1e02), board_ahci }, /* Panther Point AHCI */
- { PCI_VDEVICE(INTEL, 0x1e03), board_ahci }, /* Panther Point M AHCI */
+ { PCI_VDEVICE(INTEL, 0x1e03), board_ahci_mobile }, /* Panther M AHCI */
{ PCI_VDEVICE(INTEL, 0x1e04), board_ahci }, /* Panther Point RAID */
{ PCI_VDEVICE(INTEL, 0x1e05), board_ahci }, /* Panther Point RAID */
{ PCI_VDEVICE(INTEL, 0x1e06), board_ahci }, /* Panther Point RAID */
- { PCI_VDEVICE(INTEL, 0x1e07), board_ahci }, /* Panther Point M RAID */
+ { PCI_VDEVICE(INTEL, 0x1e07), board_ahci_mobile }, /* Panther M RAID */
{ PCI_VDEVICE(INTEL, 0x1e0e), board_ahci }, /* Panther Point RAID */
{ PCI_VDEVICE(INTEL, 0x8c02), board_ahci }, /* Lynx Point AHCI */
- { PCI_VDEVICE(INTEL, 0x8c03), board_ahci }, /* Lynx Point M AHCI */
+ { PCI_VDEVICE(INTEL, 0x8c03), board_ahci_mobile }, /* Lynx M AHCI */
{ PCI_VDEVICE(INTEL, 0x8c04), board_ahci }, /* Lynx Point RAID */
- { PCI_VDEVICE(INTEL, 0x8c05), board_ahci }, /* Lynx Point M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c05), board_ahci_mobile }, /* Lynx M RAID */
{ PCI_VDEVICE(INTEL, 0x8c06), board_ahci }, /* Lynx Point RAID */
- { PCI_VDEVICE(INTEL, 0x8c07), board_ahci }, /* Lynx Point M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c07), board_ahci_mobile }, /* Lynx M RAID */
{ PCI_VDEVICE(INTEL, 0x8c0e), board_ahci }, /* Lynx Point RAID */
- { PCI_VDEVICE(INTEL, 0x8c0f), board_ahci }, /* Lynx Point M RAID */
- { PCI_VDEVICE(INTEL, 0x9c02), board_ahci }, /* Lynx Point-LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9c03), board_ahci }, /* Lynx Point-LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9c04), board_ahci }, /* Lynx Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c05), board_ahci }, /* Lynx Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c06), board_ahci }, /* Lynx Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c07), board_ahci }, /* Lynx Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c0e), board_ahci }, /* Lynx Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c0f), board_ahci }, /* Lynx Point-LP RAID */
+ { PCI_VDEVICE(INTEL, 0x8c0f), board_ahci_mobile }, /* Lynx M RAID */
+ { PCI_VDEVICE(INTEL, 0x9c02), board_ahci_mobile }, /* Lynx LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9c03), board_ahci_mobile }, /* Lynx LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9c04), board_ahci_mobile }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c05), board_ahci_mobile }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c06), board_ahci_mobile }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c07), board_ahci_mobile }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c0e), board_ahci_mobile }, /* Lynx LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c0f), board_ahci_mobile }, /* Lynx LP RAID */
{ PCI_VDEVICE(INTEL, 0x1f22), board_ahci }, /* Avoton AHCI */
{ PCI_VDEVICE(INTEL, 0x1f23), board_ahci }, /* Avoton AHCI */
{ PCI_VDEVICE(INTEL, 0x1f24), board_ahci }, /* Avoton RAID */
@@ -352,26 +360,26 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(INTEL, 0x8d66), board_ahci }, /* Wellsburg RAID */
{ PCI_VDEVICE(INTEL, 0x8d6e), board_ahci }, /* Wellsburg RAID */
{ PCI_VDEVICE(INTEL, 0x23a3), board_ahci }, /* Coleto Creek AHCI */
- { PCI_VDEVICE(INTEL, 0x9c83), board_ahci }, /* Wildcat Point-LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9c85), board_ahci }, /* Wildcat Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c87), board_ahci }, /* Wildcat Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9c8f), board_ahci }, /* Wildcat Point-LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c83), board_ahci_mobile }, /* Wildcat LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9c85), board_ahci_mobile }, /* Wildcat LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c87), board_ahci_mobile }, /* Wildcat LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9c8f), board_ahci_mobile }, /* Wildcat LP RAID */
{ PCI_VDEVICE(INTEL, 0x8c82), board_ahci }, /* 9 Series AHCI */
- { PCI_VDEVICE(INTEL, 0x8c83), board_ahci }, /* 9 Series M AHCI */
+ { PCI_VDEVICE(INTEL, 0x8c83), board_ahci_mobile }, /* 9 Series M AHCI */
{ PCI_VDEVICE(INTEL, 0x8c84), board_ahci }, /* 9 Series RAID */
- { PCI_VDEVICE(INTEL, 0x8c85), board_ahci }, /* 9 Series M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c85), board_ahci_mobile }, /* 9 Series M RAID */
{ PCI_VDEVICE(INTEL, 0x8c86), board_ahci }, /* 9 Series RAID */
- { PCI_VDEVICE(INTEL, 0x8c87), board_ahci }, /* 9 Series M RAID */
+ { PCI_VDEVICE(INTEL, 0x8c87), board_ahci_mobile }, /* 9 Series M RAID */
{ PCI_VDEVICE(INTEL, 0x8c8e), board_ahci }, /* 9 Series RAID */
- { PCI_VDEVICE(INTEL, 0x8c8f), board_ahci }, /* 9 Series M RAID */
- { PCI_VDEVICE(INTEL, 0x9d03), board_ahci }, /* Sunrise Point-LP AHCI */
- { PCI_VDEVICE(INTEL, 0x9d05), board_ahci }, /* Sunrise Point-LP RAID */
- { PCI_VDEVICE(INTEL, 0x9d07), board_ahci }, /* Sunrise Point-LP RAID */
+ { PCI_VDEVICE(INTEL, 0x8c8f), board_ahci_mobile }, /* 9 Series M RAID */
+ { PCI_VDEVICE(INTEL, 0x9d03), board_ahci_mobile }, /* Sunrise LP AHCI */
+ { PCI_VDEVICE(INTEL, 0x9d05), board_ahci_mobile }, /* Sunrise LP RAID */
+ { PCI_VDEVICE(INTEL, 0x9d07), board_ahci_mobile }, /* Sunrise LP RAID */
{ PCI_VDEVICE(INTEL, 0xa102), board_ahci }, /* Sunrise Point-H AHCI */
- { PCI_VDEVICE(INTEL, 0xa103), board_ahci }, /* Sunrise Point-H M AHCI */
+ { PCI_VDEVICE(INTEL, 0xa103), board_ahci_mobile }, /* Sunrise M AHCI */
{ PCI_VDEVICE(INTEL, 0xa105), board_ahci }, /* Sunrise Point-H RAID */
{ PCI_VDEVICE(INTEL, 0xa106), board_ahci }, /* Sunrise Point-H RAID */
- { PCI_VDEVICE(INTEL, 0xa107), board_ahci }, /* Sunrise Point-H M RAID */
+ { PCI_VDEVICE(INTEL, 0xa107), board_ahci_mobile }, /* Sunrise M RAID */
{ PCI_VDEVICE(INTEL, 0xa10f), board_ahci }, /* Sunrise Point-H RAID */
{ PCI_VDEVICE(INTEL, 0x2822), board_ahci }, /* Lewisburg RAID*/
{ PCI_VDEVICE(INTEL, 0x2823), board_ahci }, /* Lewisburg AHCI*/
@@ -385,10 +393,10 @@ static const struct pci_device_id ahci_p
{ PCI_VDEVICE(INTEL, 0xa206), board_ahci }, /* Lewisburg RAID*/
{ PCI_VDEVICE(INTEL, 0xa252), board_ahci }, /* Lewisburg RAID*/
{ PCI_VDEVICE(INTEL, 0xa256), board_ahci }, /* Lewisburg RAID*/
- { PCI_VDEVICE(INTEL, 0x0f22), board_ahci }, /* Bay Trail AHCI */
- { PCI_VDEVICE(INTEL, 0x0f23), board_ahci }, /* Bay Trail AHCI */
- { PCI_VDEVICE(INTEL, 0x22a3), board_ahci }, /* Cherry Trail AHCI */
- { PCI_VDEVICE(INTEL, 0x5ae3), board_ahci }, /* Apollo Lake AHCI */
+ { PCI_VDEVICE(INTEL, 0x0f22), board_ahci_mobile }, /* Bay Trail AHCI */
+ { PCI_VDEVICE(INTEL, 0x0f23), board_ahci_mobile }, /* Bay Trail AHCI */
+ { PCI_VDEVICE(INTEL, 0x22a3), board_ahci_mobile }, /* Cherry Tr. AHCI */
+ { PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_mobile }, /* ApolloLake AHCI */
/* JMicron 360/1/3/5/6, match class to avoid IDE function */
{ PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
@@ -596,6 +604,9 @@ static int marvell_enable = 1;
module_param(marvell_enable, int, 0644);
MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
+static int mobile_lpm_policy = CONFIG_SATA_MOBILE_LPM_POLICY;
+module_param(mobile_lpm_policy, int, 0644);
+MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
static void ahci_pci_save_initial_config(struct pci_dev *pdev,
struct ahci_host_priv *hpriv)
@@ -1727,6 +1738,10 @@ static int ahci_init_one(struct pci_dev
if (ap->flags & ATA_FLAG_EM)
ap->em_message_type = hpriv->em_msg_type;
+ if ((hpriv->flags & AHCI_HFLAG_IS_MOBILE) &&
+ mobile_lpm_policy >= ATA_LPM_UNKNOWN &&
+ mobile_lpm_policy <= ATA_LPM_MIN_POWER)
+ ap->target_lpm_policy = mobile_lpm_policy;
/* disabled/not-implemented port */
if (!(hpriv->port_map & (1 << i)))
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -251,6 +251,9 @@ enum {
AHCI_HFLAG_YES_ALPM = (1 << 23), /* force ALPM cap on */
AHCI_HFLAG_NO_WRITE_TO_RO = (1 << 24), /* don't write to read
only registers */
+ AHCI_HFLAG_IS_MOBILE = (1 << 25), /* mobile chipset, use
+ SATA_MOBILE_LPM_POLICY
+ as default lpm_policy */
/* ap->flags bits */
Patches currently in stable-queue which might be from hdegoede@redhat.com are
queue-4.14/ahci-add-pci-ids-for-intel-bay-trail-cherry-trail-and-apollo-lake-ahci.patch
queue-4.14/ahci-allow-setting-a-default-lpm-policy-for-mobile-chipsets.patch
queue-4.14/ahci-annotate-pci-ids-for-mobile-intel-chipsets-as-such.patch
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree
2018-02-14 14:25 Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree gregkh
@ 2018-02-14 15:03 ` Hans de Goede
2018-02-14 15:48 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2018-02-14 15:03 UTC (permalink / raw)
To: gregkh, tj; +Cc: stable, stable-commits
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.
Also the Kconfig help text is wrong:
> +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).
Regards,
Hans
*) Or with 4.14 or older where there is no med_power_with_dipm to 3
> config SATA_AHCI_PLATFORM
> tristate "Platform AHCI SATA support"
> help
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -64,6 +64,7 @@ enum board_ids {
> /* board IDs by feature in alphabetical order */
> board_ahci,
> board_ahci_ign_iferr,
> + board_ahci_mobile,
> board_ahci_nomsi,
> board_ahci_noncq,
> board_ahci_nosntf,
> @@ -139,6 +140,13 @@ static const struct ata_port_info ahci_p
> .udma_mask = ATA_UDMA6,
> .port_ops = &ahci_ops,
> },
> + [board_ahci_mobile] = {
> + AHCI_HFLAGS (AHCI_HFLAG_IS_MOBILE),
> + .flags = AHCI_FLAG_COMMON,
> + .pio_mask = ATA_PIO4,
> + .udma_mask = ATA_UDMA6,
> + .port_ops = &ahci_ops,
> + },
> [board_ahci_nomsi] = {
> AHCI_HFLAGS (AHCI_HFLAG_NO_MSI),
> .flags = AHCI_FLAG_COMMON,
> @@ -251,13 +259,13 @@ static const struct pci_device_id ahci_p
> { PCI_VDEVICE(INTEL, 0x2924), board_ahci }, /* ICH9 */
> { PCI_VDEVICE(INTEL, 0x2925), board_ahci }, /* ICH9 */
> { PCI_VDEVICE(INTEL, 0x2927), board_ahci }, /* ICH9 */
> - { PCI_VDEVICE(INTEL, 0x2929), board_ahci }, /* ICH9M */
> - { PCI_VDEVICE(INTEL, 0x292a), board_ahci }, /* ICH9M */
> - { PCI_VDEVICE(INTEL, 0x292b), board_ahci }, /* ICH9M */
> - { PCI_VDEVICE(INTEL, 0x292c), board_ahci }, /* ICH9M */
> - { PCI_VDEVICE(INTEL, 0x292f), board_ahci }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x2929), board_ahci_mobile }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292a), board_ahci_mobile }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292b), board_ahci_mobile }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292c), board_ahci_mobile }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292f), board_ahci_mobile }, /* ICH9M */
> { PCI_VDEVICE(INTEL, 0x294d), board_ahci }, /* ICH9 */
> - { PCI_VDEVICE(INTEL, 0x294e), board_ahci }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x294e), board_ahci_mobile }, /* ICH9M */
> { PCI_VDEVICE(INTEL, 0x502a), board_ahci }, /* Tolapai */
> { PCI_VDEVICE(INTEL, 0x502b), board_ahci }, /* Tolapai */
> { PCI_VDEVICE(INTEL, 0x3a05), board_ahci }, /* ICH10 */
> @@ -267,9 +275,9 @@ static const struct pci_device_id ahci_p
> { PCI_VDEVICE(INTEL, 0x3b23), board_ahci }, /* PCH AHCI */
> { PCI_VDEVICE(INTEL, 0x3b24), board_ahci }, /* PCH RAID */
> { PCI_VDEVICE(INTEL, 0x3b25), board_ahci }, /* PCH RAID */
> - { PCI_VDEVICE(INTEL, 0x3b29), board_ahci }, /* PCH M AHCI */
> + { PCI_VDEVICE(INTEL, 0x3b29), board_ahci_mobile }, /* PCH M AHCI */
> { PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
> - { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci }, /* PCH M RAID */
> + { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_mobile }, /* PCH M RAID */
> { PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
> { PCI_VDEVICE(INTEL, 0x19b0), board_ahci }, /* DNV AHCI */
> { PCI_VDEVICE(INTEL, 0x19b1), board_ahci }, /* DNV AHCI */
> @@ -292,9 +300,9 @@ static const struct pci_device_id ahci_p
> { PCI_VDEVICE(INTEL, 0x19cE), board_ahci }, /* DNV AHCI */
> { PCI_VDEVICE(INTEL, 0x19cF), board_ahci }, /* DNV AHCI */
> { PCI_VDEVICE(INTEL, 0x1c02), board_ahci }, /* CPT AHCI */
> - { PCI_VDEVICE(INTEL, 0x1c03), board_ahci }, /* CPT M AHCI */
> + { PCI_VDEVICE(INTEL, 0x1c03), board_ahci_mobile }, /* CPT M AHCI */
> { PCI_VDEVICE(INTEL, 0x1c04), board_ahci }, /* CPT RAID */
> - { PCI_VDEVICE(INTEL, 0x1c05), board_ahci }, /* CPT M RAID */
> + { PCI_VDEVICE(INTEL, 0x1c05), board_ahci_mobile }, /* CPT M RAID */
> { PCI_VDEVICE(INTEL, 0x1c06), board_ahci }, /* CPT RAID */
> { PCI_VDEVICE(INTEL, 0x1c07), board_ahci }, /* CPT RAID */
> { PCI_VDEVICE(INTEL, 0x1d02), board_ahci }, /* PBG AHCI */
> @@ -303,28 +311,28 @@ static const struct pci_device_id ahci_p
> { PCI_VDEVICE(INTEL, 0x2826), board_ahci }, /* PBG RAID */
> { PCI_VDEVICE(INTEL, 0x2323), board_ahci }, /* DH89xxCC AHCI */
> { PCI_VDEVICE(INTEL, 0x1e02), board_ahci }, /* Panther Point AHCI */
> - { PCI_VDEVICE(INTEL, 0x1e03), board_ahci }, /* Panther Point M AHCI */
> + { PCI_VDEVICE(INTEL, 0x1e03), board_ahci_mobile }, /* Panther M AHCI */
> { PCI_VDEVICE(INTEL, 0x1e04), board_ahci }, /* Panther Point RAID */
> { PCI_VDEVICE(INTEL, 0x1e05), board_ahci }, /* Panther Point RAID */
> { PCI_VDEVICE(INTEL, 0x1e06), board_ahci }, /* Panther Point RAID */
> - { PCI_VDEVICE(INTEL, 0x1e07), board_ahci }, /* Panther Point M RAID */
> + { PCI_VDEVICE(INTEL, 0x1e07), board_ahci_mobile }, /* Panther M RAID */
> { PCI_VDEVICE(INTEL, 0x1e0e), board_ahci }, /* Panther Point RAID */
> { PCI_VDEVICE(INTEL, 0x8c02), board_ahci }, /* Lynx Point AHCI */
> - { PCI_VDEVICE(INTEL, 0x8c03), board_ahci }, /* Lynx Point M AHCI */
> + { PCI_VDEVICE(INTEL, 0x8c03), board_ahci_mobile }, /* Lynx M AHCI */
> { PCI_VDEVICE(INTEL, 0x8c04), board_ahci }, /* Lynx Point RAID */
> - { PCI_VDEVICE(INTEL, 0x8c05), board_ahci }, /* Lynx Point M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c05), board_ahci_mobile }, /* Lynx M RAID */
> { PCI_VDEVICE(INTEL, 0x8c06), board_ahci }, /* Lynx Point RAID */
> - { PCI_VDEVICE(INTEL, 0x8c07), board_ahci }, /* Lynx Point M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c07), board_ahci_mobile }, /* Lynx M RAID */
> { PCI_VDEVICE(INTEL, 0x8c0e), board_ahci }, /* Lynx Point RAID */
> - { PCI_VDEVICE(INTEL, 0x8c0f), board_ahci }, /* Lynx Point M RAID */
> - { PCI_VDEVICE(INTEL, 0x9c02), board_ahci }, /* Lynx Point-LP AHCI */
> - { PCI_VDEVICE(INTEL, 0x9c03), board_ahci }, /* Lynx Point-LP AHCI */
> - { PCI_VDEVICE(INTEL, 0x9c04), board_ahci }, /* Lynx Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9c05), board_ahci }, /* Lynx Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9c06), board_ahci }, /* Lynx Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9c07), board_ahci }, /* Lynx Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9c0e), board_ahci }, /* Lynx Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9c0f), board_ahci }, /* Lynx Point-LP RAID */
> + { PCI_VDEVICE(INTEL, 0x8c0f), board_ahci_mobile }, /* Lynx M RAID */
> + { PCI_VDEVICE(INTEL, 0x9c02), board_ahci_mobile }, /* Lynx LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9c03), board_ahci_mobile }, /* Lynx LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9c04), board_ahci_mobile }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c05), board_ahci_mobile }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c06), board_ahci_mobile }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c07), board_ahci_mobile }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c0e), board_ahci_mobile }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c0f), board_ahci_mobile }, /* Lynx LP RAID */
> { PCI_VDEVICE(INTEL, 0x1f22), board_ahci }, /* Avoton AHCI */
> { PCI_VDEVICE(INTEL, 0x1f23), board_ahci }, /* Avoton AHCI */
> { PCI_VDEVICE(INTEL, 0x1f24), board_ahci }, /* Avoton RAID */
> @@ -352,26 +360,26 @@ static const struct pci_device_id ahci_p
> { PCI_VDEVICE(INTEL, 0x8d66), board_ahci }, /* Wellsburg RAID */
> { PCI_VDEVICE(INTEL, 0x8d6e), board_ahci }, /* Wellsburg RAID */
> { PCI_VDEVICE(INTEL, 0x23a3), board_ahci }, /* Coleto Creek AHCI */
> - { PCI_VDEVICE(INTEL, 0x9c83), board_ahci }, /* Wildcat Point-LP AHCI */
> - { PCI_VDEVICE(INTEL, 0x9c85), board_ahci }, /* Wildcat Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9c87), board_ahci }, /* Wildcat Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9c8f), board_ahci }, /* Wildcat Point-LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c83), board_ahci_mobile }, /* Wildcat LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9c85), board_ahci_mobile }, /* Wildcat LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c87), board_ahci_mobile }, /* Wildcat LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c8f), board_ahci_mobile }, /* Wildcat LP RAID */
> { PCI_VDEVICE(INTEL, 0x8c82), board_ahci }, /* 9 Series AHCI */
> - { PCI_VDEVICE(INTEL, 0x8c83), board_ahci }, /* 9 Series M AHCI */
> + { PCI_VDEVICE(INTEL, 0x8c83), board_ahci_mobile }, /* 9 Series M AHCI */
> { PCI_VDEVICE(INTEL, 0x8c84), board_ahci }, /* 9 Series RAID */
> - { PCI_VDEVICE(INTEL, 0x8c85), board_ahci }, /* 9 Series M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c85), board_ahci_mobile }, /* 9 Series M RAID */
> { PCI_VDEVICE(INTEL, 0x8c86), board_ahci }, /* 9 Series RAID */
> - { PCI_VDEVICE(INTEL, 0x8c87), board_ahci }, /* 9 Series M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c87), board_ahci_mobile }, /* 9 Series M RAID */
> { PCI_VDEVICE(INTEL, 0x8c8e), board_ahci }, /* 9 Series RAID */
> - { PCI_VDEVICE(INTEL, 0x8c8f), board_ahci }, /* 9 Series M RAID */
> - { PCI_VDEVICE(INTEL, 0x9d03), board_ahci }, /* Sunrise Point-LP AHCI */
> - { PCI_VDEVICE(INTEL, 0x9d05), board_ahci }, /* Sunrise Point-LP RAID */
> - { PCI_VDEVICE(INTEL, 0x9d07), board_ahci }, /* Sunrise Point-LP RAID */
> + { PCI_VDEVICE(INTEL, 0x8c8f), board_ahci_mobile }, /* 9 Series M RAID */
> + { PCI_VDEVICE(INTEL, 0x9d03), board_ahci_mobile }, /* Sunrise LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9d05), board_ahci_mobile }, /* Sunrise LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9d07), board_ahci_mobile }, /* Sunrise LP RAID */
> { PCI_VDEVICE(INTEL, 0xa102), board_ahci }, /* Sunrise Point-H AHCI */
> - { PCI_VDEVICE(INTEL, 0xa103), board_ahci }, /* Sunrise Point-H M AHCI */
> + { PCI_VDEVICE(INTEL, 0xa103), board_ahci_mobile }, /* Sunrise M AHCI */
> { PCI_VDEVICE(INTEL, 0xa105), board_ahci }, /* Sunrise Point-H RAID */
> { PCI_VDEVICE(INTEL, 0xa106), board_ahci }, /* Sunrise Point-H RAID */
> - { PCI_VDEVICE(INTEL, 0xa107), board_ahci }, /* Sunrise Point-H M RAID */
> + { PCI_VDEVICE(INTEL, 0xa107), board_ahci_mobile }, /* Sunrise M RAID */
> { PCI_VDEVICE(INTEL, 0xa10f), board_ahci }, /* Sunrise Point-H RAID */
> { PCI_VDEVICE(INTEL, 0x2822), board_ahci }, /* Lewisburg RAID*/
> { PCI_VDEVICE(INTEL, 0x2823), board_ahci }, /* Lewisburg AHCI*/
> @@ -385,10 +393,10 @@ static const struct pci_device_id ahci_p
> { PCI_VDEVICE(INTEL, 0xa206), board_ahci }, /* Lewisburg RAID*/
> { PCI_VDEVICE(INTEL, 0xa252), board_ahci }, /* Lewisburg RAID*/
> { PCI_VDEVICE(INTEL, 0xa256), board_ahci }, /* Lewisburg RAID*/
> - { PCI_VDEVICE(INTEL, 0x0f22), board_ahci }, /* Bay Trail AHCI */
> - { PCI_VDEVICE(INTEL, 0x0f23), board_ahci }, /* Bay Trail AHCI */
> - { PCI_VDEVICE(INTEL, 0x22a3), board_ahci }, /* Cherry Trail AHCI */
> - { PCI_VDEVICE(INTEL, 0x5ae3), board_ahci }, /* Apollo Lake AHCI */
> + { PCI_VDEVICE(INTEL, 0x0f22), board_ahci_mobile }, /* Bay Trail AHCI */
> + { PCI_VDEVICE(INTEL, 0x0f23), board_ahci_mobile }, /* Bay Trail AHCI */
> + { PCI_VDEVICE(INTEL, 0x22a3), board_ahci_mobile }, /* Cherry Tr. AHCI */
> + { PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_mobile }, /* ApolloLake AHCI */
>
> /* JMicron 360/1/3/5/6, match class to avoid IDE function */
> { PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> @@ -596,6 +604,9 @@ static int marvell_enable = 1;
> module_param(marvell_enable, int, 0644);
> MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
>
> +static int mobile_lpm_policy = CONFIG_SATA_MOBILE_LPM_POLICY;
> +module_param(mobile_lpm_policy, int, 0644);
> +MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
>
> static void ahci_pci_save_initial_config(struct pci_dev *pdev,
> struct ahci_host_priv *hpriv)
> @@ -1727,6 +1738,10 @@ static int ahci_init_one(struct pci_dev
> if (ap->flags & ATA_FLAG_EM)
> ap->em_message_type = hpriv->em_msg_type;
>
> + if ((hpriv->flags & AHCI_HFLAG_IS_MOBILE) &&
> + mobile_lpm_policy >= ATA_LPM_UNKNOWN &&
> + mobile_lpm_policy <= ATA_LPM_MIN_POWER)
> + ap->target_lpm_policy = mobile_lpm_policy;
>
> /* disabled/not-implemented port */
> if (!(hpriv->port_map & (1 << i)))
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -251,6 +251,9 @@ enum {
> AHCI_HFLAG_YES_ALPM = (1 << 23), /* force ALPM cap on */
> AHCI_HFLAG_NO_WRITE_TO_RO = (1 << 24), /* don't write to read
> only registers */
> + AHCI_HFLAG_IS_MOBILE = (1 << 25), /* mobile chipset, use
> + SATA_MOBILE_LPM_POLICY
> + as default lpm_policy */
>
> /* ap->flags bits */
>
>
>
> Patches currently in stable-queue which might be from hdegoede@redhat.com are
>
> queue-4.14/ahci-add-pci-ids-for-intel-bay-trail-cherry-trail-and-apollo-lake-ahci.patch
> queue-4.14/ahci-allow-setting-a-default-lpm-policy-for-mobile-chipsets.patch
> queue-4.14/ahci-annotate-pci-ids-for-mobile-intel-chipsets-as-such.patch
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree
2018-02-14 15:03 ` Hans de Goede
@ 2018-02-14 15:48 ` Greg KH
2018-02-14 16:45 ` Hans de Goede
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2018-02-14 15:48 UTC (permalink / raw)
To: Hans de Goede; +Cc: tj, stable, stable-commits
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? 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.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree
2018-02-14 15:48 ` Greg KH
@ 2018-02-14 16:45 ` Hans de Goede
2018-02-14 18:23 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2018-02-14 16:45 UTC (permalink / raw)
To: Greg KH; +Cc: tj, stable, stable-commits
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree
2018-02-14 16:45 ` Hans de Goede
@ 2018-02-14 18:23 ` Greg KH
0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2018-02-14 18:23 UTC (permalink / raw)
To: Hans de Goede; +Cc: tj, stable, stable-commits
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-14 18:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-14 14:25 Patch "ahci: Allow setting a default LPM policy for mobile chipsets" has been added to the 4.14-stable tree gregkh
2018-02-14 15:03 ` Hans de Goede
2018-02-14 15:48 ` Greg KH
2018-02-14 16:45 ` Hans de Goede
2018-02-14 18:23 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox