drop low power policy board type

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 <[email protected]>

---
 drivers/ata/ahci.c    | 14 ++++++++++++++
 drivers/ata/libahci.c |  7 -------
 2 files changed, 14 insertions(+), 7 deletions(-)

drivers/ata/ahci.c +11 -0

index da2e74fce2d9..157ab88bdf75 100644
@@ -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)
 {

drivers/ata/ahci.c +1 -0

+
 		ahci_update_initial_lpm_policy(ap, hpriv);
 
 		/* disabled/not-implemented port */

drivers/ata/libahci.c +0 -7

index 1a63200ea437..fca376f03c9e 100644
@@ -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)

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 <[email protected]>

---
 drivers/ata/ahci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/ata/ahci.c +2 -1

index 157ab88bdf75..8c8403bae36f 100644
@@ -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;
 }
 

drivers/ata/ahci.c +0 -0

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

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 <[email protected]>

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 <[email protected]>

---
 drivers/ata/ahci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/ata/ahci.c +0 -0

index 19b605c98d42..d50d1ae44e7f 100644

drivers/ata/ahci.c +3 -0

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

From: Mario Limonciello <[email protected]>

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 <[email protected]>

Acked-by Christoph Hellwig <[email protected]>

Signed-off-by Mario Limonciello <[email protected]>

[cassel: rebase patch and fix trivial conflicts]

Signed-off-by Niklas Cassel <[email protected]>

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/[email protected]/T/#u

This revives a patch sent out almost two years ago from Mario Limonciello:
https://lore.kernel.org/linux-ide/[email protected]/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.

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.

(adding Mario Limonciello in CC)

Quoted 11 lines · Niklas Cassel

This revives a patch sent out almost two years ago from Mario Limonciello:
https://lore.kernel.org/linux-ide/[email protected]/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.

Quoted 6 lines

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.

Quoted 3 lines

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