[PATCH v2 1/2] ata: Kconfig: Update SATA_LPM_POLICY default to "3"

[PATCH v2 1/2] ata: Kconfig: Update SATA_LPM_POLICY default to "3"

[Mario Limonciello]

SATA_LPM_POLICY is a "new" configuration item, but it was renamed from
SATA_LPM_MOBILE_POLICY in commit 4dd4d3deb502 ("ata: ahci: Rename
CONFIG_SATA_LPM_MOBILE_POLICY configuration item")

Since the configuration item was renamed it was mentioned that people
might invisibly lose their chosen defaults for it.  This means it's a
good time to evaluate whether those defaults make sense still.

Historically this was set to "0" to prevent problems with power management
on very old drives.  However it's been observed that almost all modern
Linux distributions either set the policy to "3" in the kernel
configuration or update it to this via userspace policy changes. Update
the policy default in the kernel to "3" to match that behavior as well.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Rebase on 5.18-rc1
 drivers/ata/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e5641e6c52ee..3ffe14057ed2 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -118,7 +118,7 @@ config SATA_AHCI
 config SATA_LPM_POLICY
 	int "Default SATA Link Power Management policy for low power chipsets"
 	range 0 4
-	default 0
+	default 3
 	depends on SATA_AHCI
 	help
 	  Select the Default SATA Link Power Management (LPM) policy to use
-- 
2.34.1

[PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back

[Mario Limonciello]

CONFIG_SATA_LPM_MOBILE_POLICY was renamed to CONFIG_SATA_LPM_POLICY in
commit 4dd4d3deb502 ("ata: ahci: Rename CONFIG_SATA_LPM_MOBILE_POLICY
configuration item").

This caused some pain as users would invisibly lose configuration policy
defaults when they built the new kernel.  To help alleviate that, switch
back to the old name (even if it's wrong).

Suggested-by: Christoph Hellwig <hch@infradead.org>
Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * New patch
 drivers/ata/Kconfig | 6 ++++--
 drivers/ata/ahci.c  | 2 +-
 drivers/ata/ahci.h  | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 3ffe14057ed2..c3194b80c655 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -115,14 +115,16 @@ config SATA_AHCI
 
 	  If unsure, say N.
 
-config SATA_LPM_POLICY
+config SATA_MOBILE_LPM_POLICY
 	int "Default SATA Link Power Management policy for low power chipsets"
 	range 0 4
 	default 3
 	depends on SATA_AHCI
 	help
 	  Select the Default SATA Link Power Management (LPM) policy to use
-	  for chipsets / "South Bridges" designated as supporting low power.
+	  for chipsets / "South Bridges" supporting low-power modes. Such
+	  chipsets are typically found on most laptops but desktops and
+	  servers now also widely use chipsets with low power modes support.
 
 	  The value set has the following meanings:
 		0 => Keep firmware settings
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 84456c05e845..397dfd27c90d 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1595,7 +1595,7 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
 static void ahci_update_initial_lpm_policy(struct ata_port *ap,
 					   struct ahci_host_priv *hpriv)
 {
-	int policy = CONFIG_SATA_LPM_POLICY;
+	int policy = CONFIG_SATA_MOBILE_LPM_POLICY;
 
 
 	/* Ignore processing for chipsets that don't use policy */
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 6ead58c1b6e5..ad11a4c52fbe 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -236,7 +236,7 @@ enum {
 	AHCI_HFLAG_NO_WRITE_TO_RO	= (1 << 24), /* don't write to read
 							only registers */
 	AHCI_HFLAG_USE_LPM_POLICY	= (1 << 25), /* chipset that should use
-							SATA_LPM_POLICY
+							SATA_MOBILE_LPM_POLICY
 							as default lpm_policy */
 	AHCI_HFLAG_SUSPEND_PHYS		= (1 << 26), /* handle PHYs during
 							suspend/resume */
-- 
2.34.1

Re: [PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back

[Christoph Hellwig]

On Mon, Apr 04, 2022 at 03:02:02PM -0500, Mario Limonciello wrote:
> CONFIG_SATA_LPM_MOBILE_POLICY was renamed to CONFIG_SATA_LPM_POLICY in
> commit 4dd4d3deb502 ("ata: ahci: Rename CONFIG_SATA_LPM_MOBILE_POLICY
> configuration item").
> 
> This caused some pain as users would invisibly lose configuration policy
> defaults when they built the new kernel.  To help alleviate that, switch
> back to the old name (even if it's wrong).

Note that for this to be useful it would have to go into 5.18.

Re: [PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back

[Damien Le Moal]

On 4/5/22 13:33, Christoph Hellwig wrote:
> On Mon, Apr 04, 2022 at 03:02:02PM -0500, Mario Limonciello wrote:
>> CONFIG_SATA_LPM_MOBILE_POLICY was renamed to CONFIG_SATA_LPM_POLICY in
>> commit 4dd4d3deb502 ("ata: ahci: Rename CONFIG_SATA_LPM_MOBILE_POLICY
>> configuration item").
>>
>> This caused some pain as users would invisibly lose configuration policy
>> defaults when they built the new kernel.  To help alleviate that, switch
>> back to the old name (even if it's wrong).
> 
> Note that for this to be useful it would have to go into 5.18.

Yep, that's the plan. I will send it for rc2.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back

[Damien Le Moal]

On 4/5/22 05:02, Mario Limonciello wrote:
> CONFIG_SATA_LPM_MOBILE_POLICY was renamed to CONFIG_SATA_LPM_POLICY in
> commit 4dd4d3deb502 ("ata: ahci: Rename CONFIG_SATA_LPM_MOBILE_POLICY
> configuration item").
> 
> This caused some pain as users would invisibly lose configuration policy
> defaults when they built the new kernel.  To help alleviate that, switch
> back to the old name (even if it's wrong).
> 
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

I applied this manually as the "default 3" is from the first patch, 
which is for 5.19. This one is queued in for-5.18-fixes now. Thanks.

> ---
> v1->v2:
>   * New patch
>   drivers/ata/Kconfig | 6 ++++--
>   drivers/ata/ahci.c  | 2 +-
>   drivers/ata/ahci.h  | 2 +-
>   3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 3ffe14057ed2..c3194b80c655 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -115,14 +115,16 @@ config SATA_AHCI
>   
>   	  If unsure, say N.
>   
> -config SATA_LPM_POLICY
> +config SATA_MOBILE_LPM_POLICY
>   	int "Default SATA Link Power Management policy for low power chipsets"
>   	range 0 4
>   	default 3
>   	depends on SATA_AHCI
>   	help
>   	  Select the Default SATA Link Power Management (LPM) policy to use
> -	  for chipsets / "South Bridges" designated as supporting low power.
> +	  for chipsets / "South Bridges" supporting low-power modes. Such
> +	  chipsets are typically found on most laptops but desktops and
> +	  servers now also widely use chipsets with low power modes support.
>   
>   	  The value set has the following meanings:
>   		0 => Keep firmware settings
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 84456c05e845..397dfd27c90d 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1595,7 +1595,7 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
>   static void ahci_update_initial_lpm_policy(struct ata_port *ap,
>   					   struct ahci_host_priv *hpriv)
>   {
> -	int policy = CONFIG_SATA_LPM_POLICY;
> +	int policy = CONFIG_SATA_MOBILE_LPM_POLICY;
>   
>   
>   	/* Ignore processing for chipsets that don't use policy */
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 6ead58c1b6e5..ad11a4c52fbe 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -236,7 +236,7 @@ enum {
>   	AHCI_HFLAG_NO_WRITE_TO_RO	= (1 << 24), /* don't write to read
>   							only registers */
>   	AHCI_HFLAG_USE_LPM_POLICY	= (1 << 25), /* chipset that should use
> -							SATA_LPM_POLICY
> +							SATA_MOBILE_LPM_POLICY
>   							as default lpm_policy */
>   	AHCI_HFLAG_SUSPEND_PHYS		= (1 << 26), /* handle PHYs during
>   							suspend/resume */


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Sent: Tuesday, April 5, 2022 21:11
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>;
> hdegoede@redhat.com; Christoph Hellwig <hch@infradead.org>
> Subject: Re: [PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY
> configuration item back
> 
> On 4/5/22 05:02, Mario Limonciello wrote:
> > CONFIG_SATA_LPM_MOBILE_POLICY was renamed to
> CONFIG_SATA_LPM_POLICY in
> > commit 4dd4d3deb502 ("ata: ahci: Rename
> CONFIG_SATA_LPM_MOBILE_POLICY
> > configuration item").
> >
> > This caused some pain as users would invisibly lose configuration policy
> > defaults when they built the new kernel.  To help alleviate that, switch
> > back to the old name (even if it's wrong).
> >
> > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> I applied this manually as the "default 3" is from the first patch,
> which is for 5.19. This one is queued in for-5.18-fixes now. Thanks.

Ah I didn't realize you were not going to put default 3 in 5.18, otherwise I would
have re-ordered them.  Sorry.

Which branch did you push what you've applied so far to?
I checked libata/for-next and libata/for-5.19 and didn't see anything yet.

As most of my patches recently touch the same code want to me make sure
any more follow ups work from the right base.
 
> 
> > ---
> > v1->v2:
> >   * New patch
> >   drivers/ata/Kconfig | 6 ++++--
> >   drivers/ata/ahci.c  | 2 +-
> >   drivers/ata/ahci.h  | 2 +-
> >   3 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > index 3ffe14057ed2..c3194b80c655 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -115,14 +115,16 @@ config SATA_AHCI
> >
> >   	  If unsure, say N.
> >
> > -config SATA_LPM_POLICY
> > +config SATA_MOBILE_LPM_POLICY
> >   	int "Default SATA Link Power Management policy for low power
> chipsets"
> >   	range 0 4
> >   	default 3
> >   	depends on SATA_AHCI
> >   	help
> >   	  Select the Default SATA Link Power Management (LPM) policy to
> use
> > -	  for chipsets / "South Bridges" designated as supporting low power.
> > +	  for chipsets / "South Bridges" supporting low-power modes. Such
> > +	  chipsets are typically found on most laptops but desktops and
> > +	  servers now also widely use chipsets with low power modes
> support.
> >
> >   	  The value set has the following meanings:
> >   		0 => Keep firmware settings
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index 84456c05e845..397dfd27c90d 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -1595,7 +1595,7 @@ static int ahci_init_msi(struct pci_dev *pdev,
> unsigned int n_ports,
> >   static void ahci_update_initial_lpm_policy(struct ata_port *ap,
> >   					   struct ahci_host_priv *hpriv)
> >   {
> > -	int policy = CONFIG_SATA_LPM_POLICY;
> > +	int policy = CONFIG_SATA_MOBILE_LPM_POLICY;
> >
> >
> >   	/* Ignore processing for chipsets that don't use policy */
> > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> > index 6ead58c1b6e5..ad11a4c52fbe 100644
> > --- a/drivers/ata/ahci.h
> > +++ b/drivers/ata/ahci.h
> > @@ -236,7 +236,7 @@ enum {
> >   	AHCI_HFLAG_NO_WRITE_TO_RO	= (1 << 24), /* don't write to
> read
> >   							only registers */
> >   	AHCI_HFLAG_USE_LPM_POLICY	= (1 << 25), /* chipset that
> should use
> > -							SATA_LPM_POLICY
> > +
> 	SATA_MOBILE_LPM_POLICY
> >   							as default lpm_policy
> */
> >   	AHCI_HFLAG_SUSPEND_PHYS		= (1 << 26), /* handle PHYs
> during
> >   							suspend/resume */
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Re: [PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back

[Damien Le Moal]

On 4/7/22 01:59, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Sent: Tuesday, April 5, 2022 21:11
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
>> ide@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>;
>> hdegoede@redhat.com; Christoph Hellwig <hch@infradead.org>
>> Subject: Re: [PATCH v2 2/2] ata: ahci: Rename CONFIG_SATA_LPM_POLICY
>> configuration item back
>>
>> On 4/5/22 05:02, Mario Limonciello wrote:
>>> CONFIG_SATA_LPM_MOBILE_POLICY was renamed to
>> CONFIG_SATA_LPM_POLICY in
>>> commit 4dd4d3deb502 ("ata: ahci: Rename
>> CONFIG_SATA_LPM_MOBILE_POLICY
>>> configuration item").
>>>
>>> This caused some pain as users would invisibly lose configuration policy
>>> defaults when they built the new kernel.  To help alleviate that, switch
>>> back to the old name (even if it's wrong).
>>>
>>> Suggested-by: Christoph Hellwig <hch@infradead.org>
>>> Suggested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>> I applied this manually as the "default 3" is from the first patch,
>> which is for 5.19. This one is queued in for-5.18-fixes now. Thanks.
> 
> Ah I didn't realize you were not going to put default 3 in 5.18, otherwise I would
> have re-ordered them.  Sorry.
> 
> Which branch did you push what you've applied so far to?
> I checked libata/for-next and libata/for-5.19 and didn't see anything yet.

I pushed the patch to for-5.18-fixes and will send a PR to Linus tomorrow.
The for-5.19 and for-next branches always are in sync and currently based
on 5.18-rc1. I will rebase them on rc2 next week to have the name revert
in and avoid conflicts.

> 
> As most of my patches recently touch the same code want to me make sure
> any more follow ups work from the right base.

Use 5.19 + the name revert patch for your work. That will correspond to
for-5.19 next week.


-- 
Damien Le Moal
Western Digital Research