[PATCH 0/5] drop low power policy board type

[PATCH 0/5] drop low power policy board type

[Niklas Cassel]

The series is based on top of:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next


Hello all,

This revives a patch sent out almost two years ago from Mario Limonciello:
https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u

The reason why we did not merge it back then, is because LPM and hotplug
events are mutually exclusive.

The difference with this series compared to what was sent out back then:
I've added a patch that checks if the port is external, i.e. either
hotplug capable or eSATA. For external ports, we never enable LPM, as
that will break hotplug.

For ports that do not advertise themselves as external (typically laptops),
we set the LPM policy as requested.

This matches how Microsoft Windows does things.
Thanks to Werner Fischer for suggesting something like this at last year's
ALPSS conference.

There might of course be some platform firmware that e.g. incorrectly marks
its port as internal, even though it is external, but if we find any such
platforms we will need to deal with them using quirks.


Also note that we even if the user requested a certain policy, there is
no guarantee that he will get all the features for that policy, see:
https://github.com/torvalds/linux/blob/master/drivers/ata/libata-sata.c#L403-L414

However, I'd rather we not try to map all the combinations of
partial/slumber/devsleep in to a single policy represented by a single
integer, thus I do not try to "change" the requested policy.
The user will get all the features that are included in the requested
policy AND supported by the HBA.

Another difference (compared to an earlier version of Mario's series)
is that we do not try to change the default CONFIG_SATA_MOBILE_LPM_POLICY
value from 0 to 3, it will continue to be 0.
If you really don't want LPM even if your HBA supports it, and your port
is internal, one option is to leave the Kconfig set to the default value.

Note: in current mainline, there is an issue related to Intel VMD
which causes the link to not come up when VMD is enabled.
(The link does comes up when VMD is disabled in BIOS.)
In order to not get a bunch of bug reports related to LPM, we probably
want to wait with this series until we have fixed the VMD bug.


Kind regards,
Niklas


Mario Limonciello (1):
  ata: ahci: Drop low power policy board type

Niklas Cassel (4):
  ata: ahci: move marking of external port earlier
  ata: ahci: a hotplug capable port is an external port
  ata: ahci: drop hpriv param from ahci_update_initial_lpm_policy()
  ata: ahci: do not enable LPM on external ports

 drivers/ata/Kconfig   |   5 +-
 drivers/ata/ahci.c    | 134 +++++++++++++++++++++++-------------------
 drivers/ata/ahci.h    |   9 +--
 drivers/ata/libahci.c |   7 ---
 4 files changed, 77 insertions(+), 78 deletions(-)

-- 
2.43.0

[PATCH 1/5] ata: ahci: move marking of external port earlier

[Niklas Cassel]

Move the marking of an external port earlier in the call chain.
This is needed for further cleanups.
No functional change intended.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c    | 14 ++++++++++++++
 drivers/ata/libahci.c |  7 -------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index da2e74fce2d9..157ab88bdf75 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1642,6 +1642,17 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
 	return pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
 }
 
+static void ahci_mark_external_port(struct ata_port *ap)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 tmp;
+
+	tmp = readl(port_mmio + PORT_CMD);
+	if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
+		ap->pflags |= ATA_PFLAG_EXTERNAL;
+}
+
 static void ahci_update_initial_lpm_policy(struct ata_port *ap,
 					   struct ahci_host_priv *hpriv)
 {
@@ -1934,6 +1945,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		if (ap->flags & ATA_FLAG_EM)
 			ap->em_message_type = hpriv->em_msg_type;
 
+		/* mark esata ports */
+		ahci_mark_external_port(ap);
+
 		ahci_update_initial_lpm_policy(ap, hpriv);
 
 		/* disabled/not-implemented port */
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 1a63200ea437..fca376f03c9e 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1280,10 +1280,8 @@ static void ahci_port_init(struct device *dev, struct ata_port *ap,
 			   int port_no, void __iomem *mmio,
 			   void __iomem *port_mmio)
 {
-	struct ahci_host_priv *hpriv = ap->host->private_data;
 	const char *emsg = NULL;
 	int rc;
-	u32 tmp;
 
 	/* make sure port is not active */
 	rc = ahci_deinit_port(ap, &emsg);
@@ -1291,11 +1289,6 @@ static void ahci_port_init(struct device *dev, struct ata_port *ap,
 		dev_warn(dev, "%s (%d)\n", emsg, rc);
 
 	ahci_port_clear_pending_irq(ap);
-
-	/* mark esata ports */
-	tmp = readl(port_mmio + PORT_CMD);
-	if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
-		ap->pflags |= ATA_PFLAG_EXTERNAL;
 }
 
 void ahci_init_controller(struct ata_host *host)
-- 
2.43.0

Re: [PATCH 1/5] ata: ahci: move marking of external port earlier

[Damien Le Moal]

On 2/2/24 01:14, Niklas Cassel wrote:
> Move the marking of an external port earlier in the call chain.
> This is needed for further cleanups.
> No functional change intended.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c    | 14 ++++++++++++++
>  drivers/ata/libahci.c |  7 -------
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index da2e74fce2d9..157ab88bdf75 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1642,6 +1642,17 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,
>  	return pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
>  }
>  
> +static void ahci_mark_external_port(struct ata_port *ap)
> +{
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	void __iomem *port_mmio = ahci_port_base(ap);
> +	u32 tmp;
> +
> +	tmp = readl(port_mmio + PORT_CMD);
> +	if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
> +		ap->pflags |= ATA_PFLAG_EXTERNAL;
> +}
> +
>  static void ahci_update_initial_lpm_policy(struct ata_port *ap,
>  					   struct ahci_host_priv *hpriv)
>  {
> @@ -1934,6 +1945,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		if (ap->flags & ATA_FLAG_EM)
>  			ap->em_message_type = hpriv->em_msg_type;
>  
> +		/* mark esata ports */
> +		ahci_mark_external_port(ap);

Nit: the comment is rather useless as the function name is clear. It is also
rather incorrect since this is for both esata as well as hot-plug capable ports
(later in the series). So maybe drop it ?

Otherwise, looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> +
>  		ahci_update_initial_lpm_policy(ap, hpriv);
>  
>  		/* disabled/not-implemented port */
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 1a63200ea437..fca376f03c9e 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1280,10 +1280,8 @@ static void ahci_port_init(struct device *dev, struct ata_port *ap,
>  			   int port_no, void __iomem *mmio,
>  			   void __iomem *port_mmio)
>  {
> -	struct ahci_host_priv *hpriv = ap->host->private_data;
>  	const char *emsg = NULL;
>  	int rc;
> -	u32 tmp;
>  
>  	/* make sure port is not active */
>  	rc = ahci_deinit_port(ap, &emsg);
> @@ -1291,11 +1289,6 @@ static void ahci_port_init(struct device *dev, struct ata_port *ap,
>  		dev_warn(dev, "%s (%d)\n", emsg, rc);
>  
>  	ahci_port_clear_pending_irq(ap);
> -
> -	/* mark esata ports */
> -	tmp = readl(port_mmio + PORT_CMD);
> -	if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
> -		ap->pflags |= ATA_PFLAG_EXTERNAL;
>  }
>  
>  void ahci_init_controller(struct ata_host *host)

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/5] ata: ahci: a hotplug capable port is an external port

[Niklas Cassel]

A hotplug capable port is an external port, so mark it as such.

We even say this ourselves in libata-scsi.c:
/* set scsi removable (RMB) bit per ata bit, or if the
 * AHCI port says it's external (Hotplug-capable, eSATA).
 */

This also matches the terminology used in AHCI 1.3.1
(the keyword to search for is "externally accessible").

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 157ab88bdf75..8c8403bae36f 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1649,7 +1649,8 @@ static void ahci_mark_external_port(struct ata_port *ap)
 	u32 tmp;
 
 	tmp = readl(port_mmio + PORT_CMD);
-	if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
+	if (((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS)) ||
+	    (tmp & PORT_CMD_HPCP))
 		ap->pflags |= ATA_PFLAG_EXTERNAL;
 }
 
@@ -1945,7 +1946,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		if (ap->flags & ATA_FLAG_EM)
 			ap->em_message_type = hpriv->em_msg_type;
 
-		/* mark esata ports */
+		/* mark external ports (hotplug-capable, eSATA) */
 		ahci_mark_external_port(ap);
 
 		ahci_update_initial_lpm_policy(ap, hpriv);
-- 
2.43.0

Re: [PATCH 2/5] ata: ahci: a hotplug capable port is an external port

[Damien Le Moal]

On 2/2/24 01:14, Niklas Cassel wrote:
> A hotplug capable port is an external port, so mark it as such.
> 
> We even say this ourselves in libata-scsi.c:
> /* set scsi removable (RMB) bit per ata bit, or if the
>  * AHCI port says it's external (Hotplug-capable, eSATA).
>  */
> 
> This also matches the terminology used in AHCI 1.3.1
> (the keyword to search for is "externally accessible").
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 157ab88bdf75..8c8403bae36f 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1649,7 +1649,8 @@ static void ahci_mark_external_port(struct ata_port *ap)
>  	u32 tmp;
>  
>  	tmp = readl(port_mmio + PORT_CMD);
> -	if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
> +	if (((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS)) ||
> +	    (tmp & PORT_CMD_HPCP))
>  		ap->pflags |= ATA_PFLAG_EXTERNAL;
>  }
>  
> @@ -1945,7 +1946,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		if (ap->flags & ATA_FLAG_EM)
>  			ap->em_message_type = hpriv->em_msg_type;
>  
> -		/* mark esata ports */
> +		/* mark external ports (hotplug-capable, eSATA) */

OK. So you fix the comment here... Maybe move that comment inside
ahci_mark_external_port() to make it clear which port are considered "external"
and remove the comment here ?

Otherwise, looks OK.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

>  		ahci_mark_external_port(ap);
>  
>  		ahci_update_initial_lpm_policy(ap, hpriv);

-- 
Damien Le Moal
Western Digital Research

[PATCH 3/5] ata: ahci: drop hpriv param from ahci_update_initial_lpm_policy()

[Niklas Cassel]

There is no need for ahci_update_initial_lpm_policy() to take hpriv as a
parameter, it can easily be derived from the ata_port.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 8c8403bae36f..19b605c98d42 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1654,9 +1654,9 @@ static void ahci_mark_external_port(struct ata_port *ap)
 		ap->pflags |= ATA_PFLAG_EXTERNAL;
 }
 
-static void ahci_update_initial_lpm_policy(struct ata_port *ap,
-					   struct ahci_host_priv *hpriv)
+static void ahci_update_initial_lpm_policy(struct ata_port *ap)
 {
+	struct ahci_host_priv *hpriv = ap->host->private_data;
 	int policy = CONFIG_SATA_MOBILE_LPM_POLICY;
 
 
@@ -1949,7 +1949,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		/* mark external ports (hotplug-capable, eSATA) */
 		ahci_mark_external_port(ap);
 
-		ahci_update_initial_lpm_policy(ap, hpriv);
+		ahci_update_initial_lpm_policy(ap);
 
 		/* disabled/not-implemented port */
 		if (!(hpriv->port_map & (1 << i)))
-- 
2.43.0

Re: [PATCH 3/5] ata: ahci: drop hpriv param from ahci_update_initial_lpm_policy()

[Damien Le Moal]

On 2/2/24 01:15, Niklas Cassel wrote:
> There is no need for ahci_update_initial_lpm_policy() to take hpriv as a
> parameter, it can easily be derived from the ata_port.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 4/5] ata: ahci: do not enable LPM on external ports

[Niklas Cassel]

The SATA specification contains a known incompatibility between LPM and
hot-plug events, thus do not enable LPM if the port advertises itself as
being an external SATA port (i.e. either hot-plug capable or eSATA).

This matches the power policy used in Microsoft Windows, which disables
LPM for external SATA ports, such that hot-plug events can be received.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 19b605c98d42..d50d1ae44e7f 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1664,6 +1664,14 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
 	if (!(hpriv->flags & AHCI_HFLAG_USE_LPM_POLICY))
 		return;
 
+	/*
+	 * The SATA specification contains a known incompatibility between LPM
+	 * and hot-plug events, thus do not enable LPM if the port advertises
+	 * itself as an external port (i.e. either hot-plug capable or eSATA).
+	 */
+	if (ap->pflags & ATA_PFLAG_EXTERNAL)
+		return;
+
 	/* user modified policy via module param */
 	if (mobile_lpm_policy != -1) {
 		policy = mobile_lpm_policy;
-- 
2.43.0

Re: [PATCH 4/5] ata: ahci: do not enable LPM on external ports

[Damien Le Moal]

On 2/2/24 01:15, Niklas Cassel wrote:
> The SATA specification contains a known incompatibility between LPM and
> hot-plug events, thus do not enable LPM if the port advertises itself as
> being an external SATA port (i.e. either hot-plug capable or eSATA).
> 
> This matches the power policy used in Microsoft Windows, which disables
> LPM for external SATA ports, such that hot-plug events can be received.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 19b605c98d42..d50d1ae44e7f 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1664,6 +1664,14 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>  	if (!(hpriv->flags & AHCI_HFLAG_USE_LPM_POLICY))
>  		return;
>  
> +	/*
> +	 * The SATA specification contains a known incompatibility between LPM
> +	 * and hot-plug events, thus do not enable LPM if the port advertises
> +	 * itself as an external port (i.e. either hot-plug capable or eSATA).
> +	 */

s/SATA/Serial ATA Revision 3.5a ?
s/contains/warns about ?

I see section 5.3.10 "Potential external SATA incompatibility issues" but it
does not mention LPM. Where is this stated in SATA-IO ?

> +	if (ap->pflags & ATA_PFLAG_EXTERNAL)
> +		return;
> +
>  	/* user modified policy via module param */
>  	if (mobile_lpm_policy != -1) {
>  		policy = mobile_lpm_policy;

-- 
Damien Le Moal
Western Digital Research

[PATCH 5/5] ata: ahci: Drop low power policy board type

[Niklas Cassel]

From: Mario Limonciello <mario.limonciello@amd.com>

The low power policy board type was introduced to allow systems
to get into deep states reliably.  Before it was introduced `min_power`
was causing problems for a number of drives.  New power policies
`min_power_with_partial` and `med_power_with_dipm` have been introduced
which provide a more stable baseline for systems.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Acked-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
[cassel: rebase patch and fix trivial conflicts]
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/Kconfig |   5 +-
 drivers/ata/ahci.c  | 109 +++++++++++++++++++-------------------------
 drivers/ata/ahci.h  |   9 ++--
 3 files changed, 53 insertions(+), 70 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 42b51c9812a0..928ec93c6b45 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -116,15 +116,14 @@ config SATA_AHCI
 	  If unsure, say N.
 
 config SATA_MOBILE_LPM_POLICY
-	int "Default SATA Link Power Management policy for low power chipsets"
+	int "Default SATA Link Power Management policy"
 	range 0 4
 	default 0
 	depends on SATA_AHCI
 	help
 	  Select the Default SATA Link Power Management (LPM) policy to use
 	  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 supporting low power modes.
+	  chipsets are ubiquitous across laptops, desktops and servers.
 
 	  The value set has the following meanings:
 		0 => Keep firmware settings
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index d50d1ae44e7f..97b22e2fc428 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -50,7 +50,6 @@ enum board_ids {
 	board_ahci,
 	board_ahci_43bit_dma,
 	board_ahci_ign_iferr,
-	board_ahci_low_power,
 	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
@@ -143,13 +142,6 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_low_power] = {
-		AHCI_HFLAGS	(AHCI_HFLAG_USE_LPM_POLICY),
-		.flags		= AHCI_FLAG_COMMON,
-		.pio_mask	= ATA_PIO4,
-		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_ops,
-	},
 	[board_ahci_no_debounce_delay] = {
 		.flags		= AHCI_FLAG_COMMON,
 		.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
@@ -283,13 +275,13 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ 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_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292a), board_ahci_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292b), board_ahci_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292c), board_ahci_low_power }, /* ICH9M */
-	{ PCI_VDEVICE(INTEL, 0x292f), board_ahci_low_power }, /* ICH9M */
+	{ 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, 0x294d), board_ahci }, /* ICH9 */
-	{ PCI_VDEVICE(INTEL, 0x294e), board_ahci_low_power }, /* ICH9M */
+	{ PCI_VDEVICE(INTEL, 0x294e), board_ahci }, /* ICH9M */
 	{ PCI_VDEVICE(INTEL, 0x502a), board_ahci }, /* Tolapai */
 	{ PCI_VDEVICE(INTEL, 0x502b), board_ahci }, /* Tolapai */
 	{ PCI_VDEVICE(INTEL, 0x3a05), board_ahci }, /* ICH10 */
@@ -299,9 +291,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ 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_low_power }, /* PCH M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x3b29), board_ahci }, /* PCH M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
-	{ PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_low_power }, /* PCH M RAID */
+	{ PCI_VDEVICE(INTEL, 0x3b2c), board_ahci }, /* PCH M RAID */
 	{ PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
 	{ PCI_VDEVICE(INTEL, 0x19b0), board_ahci_pcs7 }, /* DNV AHCI */
 	{ PCI_VDEVICE(INTEL, 0x19b1), board_ahci_pcs7 }, /* DNV AHCI */
@@ -324,9 +316,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x19cE), board_ahci_pcs7 }, /* DNV AHCI */
 	{ PCI_VDEVICE(INTEL, 0x19cF), board_ahci_pcs7 }, /* DNV AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1c02), board_ahci }, /* CPT AHCI */
-	{ PCI_VDEVICE(INTEL, 0x1c03), board_ahci_low_power }, /* CPT M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x1c03), board_ahci }, /* CPT M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1c04), board_ahci }, /* CPT RAID */
-	{ PCI_VDEVICE(INTEL, 0x1c05), board_ahci_low_power }, /* CPT M RAID */
+	{ PCI_VDEVICE(INTEL, 0x1c05), board_ahci }, /* 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 */
@@ -334,29 +326,29 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x1d06), 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_low_power }, /* Panther M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x1e03), board_ahci }, /* 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_low_power }, /* Panther M RAID */
+	{ PCI_VDEVICE(INTEL, 0x1e07), board_ahci }, /* 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_low_power }, /* Lynx M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x8c03), board_ahci }, /* Lynx M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x8c04), board_ahci }, /* Lynx Point RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c05), board_ahci_low_power }, /* Lynx M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c05), board_ahci }, /* Lynx M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c06), board_ahci }, /* Lynx Point RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c07), board_ahci_low_power }, /* Lynx M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c07), board_ahci }, /* Lynx M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c0e), board_ahci }, /* Lynx Point RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c0f), board_ahci_low_power }, /* Lynx M RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c02), board_ahci_low_power }, /* Lynx LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9c03), board_ahci_low_power }, /* Lynx LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9c04), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c05), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c06), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c07), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c0e), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c0f), board_ahci_low_power }, /* Lynx LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9dd3), board_ahci_low_power }, /* Cannon Lake PCH-LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x8c0f), board_ahci }, /* Lynx M RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c02), board_ahci }, /* Lynx LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9c03), board_ahci }, /* Lynx LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9c04), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c05), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c06), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c07), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c0e), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c0f), board_ahci }, /* Lynx LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9dd3), board_ahci }, /* Cannon Lake PCH-LP AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1f22), board_ahci }, /* Avoton AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1f23), board_ahci }, /* Avoton AHCI */
 	{ PCI_VDEVICE(INTEL, 0x1f24), board_ahci }, /* Avoton RAID */
@@ -390,26 +382,26 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ 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_low_power }, /* Wildcat LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9c85), board_ahci_low_power }, /* Wildcat LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c87), board_ahci_low_power }, /* Wildcat LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9c8f), board_ahci_low_power }, /* Wildcat LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c83), board_ahci }, /* Wildcat LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9c85), board_ahci }, /* Wildcat LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c87), board_ahci }, /* Wildcat LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9c8f), board_ahci }, /* Wildcat LP RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c82), board_ahci }, /* 9 Series AHCI */
-	{ PCI_VDEVICE(INTEL, 0x8c83), board_ahci_low_power }, /* 9 Series M AHCI */
+	{ PCI_VDEVICE(INTEL, 0x8c83), board_ahci }, /* 9 Series M AHCI */
 	{ PCI_VDEVICE(INTEL, 0x8c84), board_ahci }, /* 9 Series RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c85), board_ahci_low_power }, /* 9 Series M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c85), board_ahci }, /* 9 Series M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c86), board_ahci }, /* 9 Series RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c87), board_ahci_low_power }, /* 9 Series M RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c87), board_ahci }, /* 9 Series M RAID */
 	{ PCI_VDEVICE(INTEL, 0x8c8e), board_ahci }, /* 9 Series RAID */
-	{ PCI_VDEVICE(INTEL, 0x8c8f), board_ahci_low_power }, /* 9 Series M RAID */
-	{ PCI_VDEVICE(INTEL, 0x9d03), board_ahci_low_power }, /* Sunrise LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x9d05), board_ahci_low_power }, /* Sunrise LP RAID */
-	{ PCI_VDEVICE(INTEL, 0x9d07), board_ahci_low_power }, /* Sunrise LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x8c8f), board_ahci }, /* 9 Series M RAID */
+	{ PCI_VDEVICE(INTEL, 0x9d03), board_ahci }, /* Sunrise LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x9d05), board_ahci }, /* Sunrise LP RAID */
+	{ PCI_VDEVICE(INTEL, 0x9d07), board_ahci }, /* Sunrise LP RAID */
 	{ PCI_VDEVICE(INTEL, 0xa102), board_ahci }, /* Sunrise Point-H AHCI */
-	{ PCI_VDEVICE(INTEL, 0xa103), board_ahci_low_power }, /* Sunrise M AHCI */
+	{ PCI_VDEVICE(INTEL, 0xa103), board_ahci }, /* 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_low_power }, /* Sunrise M RAID */
+	{ PCI_VDEVICE(INTEL, 0xa107), board_ahci }, /* Sunrise M RAID */
 	{ PCI_VDEVICE(INTEL, 0xa10f), board_ahci }, /* Sunrise Point-H RAID */
 	{ PCI_VDEVICE(INTEL, 0xa182), board_ahci }, /* Lewisburg AHCI*/
 	{ PCI_VDEVICE(INTEL, 0xa186), board_ahci }, /* Lewisburg RAID*/
@@ -422,16 +414,16 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0xa356), board_ahci }, /* Cannon Lake PCH-H RAID */
 	{ PCI_VDEVICE(INTEL, 0x06d7), board_ahci }, /* Comet Lake-H RAID */
 	{ PCI_VDEVICE(INTEL, 0xa386), board_ahci }, /* Comet Lake PCH-V RAID */
-	{ PCI_VDEVICE(INTEL, 0x0f22), board_ahci_low_power }, /* Bay Trail AHCI */
-	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_low_power }, /* Bay Trail AHCI */
-	{ PCI_VDEVICE(INTEL, 0x22a3), board_ahci_low_power }, /* Cherry Tr. AHCI */
-	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_low_power }, /* ApolloLake AHCI */
-	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci_low_power }, /* Ice Lake LP AHCI */
-	{ PCI_VDEVICE(INTEL, 0x02d3), board_ahci_low_power }, /* Comet Lake PCH-U AHCI */
-	{ PCI_VDEVICE(INTEL, 0x02d7), board_ahci_low_power }, /* Comet Lake PCH 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 Tr. AHCI */
+	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci }, /* ApolloLake AHCI */
+	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci }, /* Ice Lake LP AHCI */
+	{ PCI_VDEVICE(INTEL, 0x02d3), board_ahci }, /* Comet Lake PCH-U AHCI */
+	{ PCI_VDEVICE(INTEL, 0x02d7), board_ahci }, /* Comet Lake PCH RAID */
 	/* Elkhart Lake IDs 0x4b60 & 0x4b62 https://sata-io.org/product/8803 not tested yet */
-	{ PCI_VDEVICE(INTEL, 0x4b63), board_ahci_low_power }, /* Elkhart Lake AHCI */
-	{ PCI_VDEVICE(INTEL, 0x7ae2), board_ahci_low_power }, /* Alder Lake-P AHCI */
+	{ PCI_VDEVICE(INTEL, 0x4b63), board_ahci }, /* Elkhart Lake AHCI */
+	{ PCI_VDEVICE(INTEL, 0x7ae2), board_ahci }, /* Alder Lake-P 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,
@@ -459,7 +451,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
 	{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /* AMD Hudson-2 (AHCI mode) */
 	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
-	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_low_power }, /* AMD Green Sardine */
+	{ PCI_VDEVICE(AMD, 0x7901), board_ahci }, /* AMD Green Sardine */
 	/* AMD is using RAID class only for ahci controllers */
 	{ PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
 	  PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
@@ -1659,11 +1651,6 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
 	struct ahci_host_priv *hpriv = ap->host->private_data;
 	int policy = CONFIG_SATA_MOBILE_LPM_POLICY;
 
-
-	/* Ignore processing for chipsets that don't use policy */
-	if (!(hpriv->flags & AHCI_HFLAG_USE_LPM_POLICY))
-		return;
-
 	/*
 	 * The SATA specification contains a known incompatibility between LPM
 	 * and hot-plug events, thus do not enable LPM if the port advertises
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index df8f8a1a3a34..4a0a602c6b16 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -241,13 +241,10 @@ enum {
 	AHCI_HFLAG_YES_ALPM		= BIT(23), /* force ALPM cap on */
 	AHCI_HFLAG_NO_WRITE_TO_RO	= BIT(24), /* don't write to read
 						      only registers */
-	AHCI_HFLAG_USE_LPM_POLICY	= BIT(25), /* chipset that should use
-						      SATA_MOBILE_LPM_POLICY
-						      as default lpm_policy */
-	AHCI_HFLAG_SUSPEND_PHYS		= BIT(26), /* handle PHYs during
+	AHCI_HFLAG_SUSPEND_PHYS		= BIT(25), /* handle PHYs during
 						      suspend/resume */
-	AHCI_HFLAG_NO_SXS		= BIT(28), /* SXS not supported */
-	AHCI_HFLAG_43BIT_ONLY		= BIT(29), /* 43bit DMA addr limit */
+	AHCI_HFLAG_NO_SXS		= BIT(26), /* SXS not supported */
+	AHCI_HFLAG_43BIT_ONLY		= BIT(27), /* 43bit DMA addr limit */
 
 	/* ap->flags bits */
 
-- 
2.43.0

Re: [PATCH 5/5] ata: ahci: Drop low power policy board type

[Damien Le Moal]

On 2/2/24 01:15, Niklas Cassel wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> The low power policy board type was introduced to allow systems
> to get into deep states reliably.  Before it was introduced `min_power`
> was causing problems for a number of drives.  New power policies
> `min_power_with_partial` and `med_power_with_dipm` have been introduced
> which provide a more stable baseline for systems.
> 
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Acked-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> [cassel: rebase patch and fix trivial conflicts]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

Let's get this in linux-next ASAP so that it gets plenty of testing.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/5] drop low power policy board type

[Damien Le Moal]

On 2/2/24 01:14, Niklas Cassel wrote:
> The series is based on top of:
> https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next
> 
> 
> Hello all,
> 
> This revives a patch sent out almost two years ago from Mario Limonciello:
> https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u

You forgot to CC Mario.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/5] drop low power policy board type

[Werner Fischer]

(adding Mario Limonciello in CC)

On Thu, 2024-02-01 at 17:14 +0100, Niklas Cassel wrote:

> This revives a patch sent out almost two years ago from Mario
> Limonciello:
> https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u
> 
> The reason why we did not merge it back then, is because LPM and
> hotplug events are mutually exclusive.
>
> I've added a patch that checks if the port is external, i.e. either
> hotplug capable or eSATA. For external ports, we never enable LPM, as
> that will break hotplug.
> 
> For ports that do not advertise themselves as external (typically
> laptops), we set the LPM policy as requested.
>
> This matches how Microsoft Windows does things.
> Thanks to Werner Fischer for suggesting something like this at last
> year's ALPSS conference.
Thank you for the discussions last year at the ALPSS and thanks a lot
for implementing this now.

> There might of course be some platform firmware that e.g. incorrectly
> marks its port as internal, even though it is external, but if we
> find any such platforms we will need to deal with them using quirks.
I plan some testing within the upcoming two weeks.
I'll do testing with the Elkhart-Lake based system (0x4b63 SATA
controller) to verify whether LPM is activated.
And I'll reach out to colleagues to test with hot-pluggable servers.
Especially I'll try for them to get a system with an AMD 0x7901 SATA
controller like Supermicro H12SSL-NT, as the default
"board_ahci_mobile" for this SATA controller - see commit 1527f69204fe
("ata: ahci: Add Green Sardine vendor ID as board_ahci_mobile") - lead
to Hot-Plug not working with default Ubuntu or Proxmox Kernels [1].

As especially for AMD systems identical SATA controllers (like 0x7901
in this case) can be used in both mobile systems and servers, Niklas'
new patch series could could bring lasting improvement here.

[1] https://www.thomas-krenn.com/en/wiki/AMD_EPYC_Server_with_Ubuntu_-_Enable_SATA_Hot-Swap

--
Best regards,
Werner Fischer

Re: [PATCH 0/5] drop low power policy board type

[Mario Limonciello]

On 2/2/2024 07:13, Werner Fischer wrote:
> (adding Mario Limonciello in CC)
> 
> On Thu, 2024-02-01 at 17:14 +0100, Niklas Cassel wrote:
> 
>> This revives a patch sent out almost two years ago from Mario
>> Limonciello:
>> https://lore.kernel.org/linux-ide/20220524170508.563-1-mario.limonciello@amd.com/T/#u
>>
>> The reason why we did not merge it back then, is because LPM and
>> hotplug events are mutually exclusive.
>>
>> I've added a patch that checks if the port is external, i.e. either
>> hotplug capable or eSATA. For external ports, we never enable LPM, as
>> that will break hotplug.
>>
>> For ports that do not advertise themselves as external (typically
>> laptops), we set the LPM policy as requested.
>>
>> This matches how Microsoft Windows does things.
>> Thanks to Werner Fischer for suggesting something like this at last
>> year's ALPSS conference.
> Thank you for the discussions last year at the ALPSS and thanks a lot
> for implementing this now.
> 
>> There might of course be some platform firmware that e.g. incorrectly
>> marks its port as internal, even though it is external, but if we
>> find any such platforms we will need to deal with them using quirks.
> I plan some testing within the upcoming two weeks.
> I'll do testing with the Elkhart-Lake based system (0x4b63 SATA
> controller) to verify whether LPM is activated.
> And I'll reach out to colleagues to test with hot-pluggable servers.
> Especially I'll try for them to get a system with an AMD 0x7901 SATA
> controller like Supermicro H12SSL-NT, as the default
> "board_ahci_mobile" for this SATA controller - see commit 1527f69204fe
> ("ata: ahci: Add Green Sardine vendor ID as board_ahci_mobile") - lead
> to Hot-Plug not working with default Ubuntu or Proxmox Kernels [1].
> 
> As especially for AMD systems identical SATA controllers (like 0x7901
> in this case) can be used in both mobile systems and servers, Niklas'
> new patch series could could bring lasting improvement here.
> 
> [1] https://www.thomas-krenn.com/en/wiki/AMD_EPYC_Server_with_Ubuntu_-_Enable_SATA_Hot-Swap
> 
> --
> Best regards,
> Werner Fischer

Thanks for looping me in and also reviving the patch.  The series looks 
good to me.  I do think that after this baked a bit and everyone is 
happy we should also revisit changing the upstream default to match the 
distros again too.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>