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

[PATCH v2 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, see:
https://studylib.net/doc/10034428/esata---microsoft-center

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.

Damien: considering that the Intel VMD + ahci_intel_pcs_quirk() bug turned
out to have nothing to do with LPM, it was simply the fact that the
ahci_intel_pcs_quirk() was only applied if there was an explicit entry in
ahci_pci_tbl. So since that bug is totally unrelated to LPM, I no longer
think that this series need to wait for a fix for that bug.


Link to v1:
https://lore.kernel.org/linux-ide/20240201161507.1147521-1-cassel@kernel.org/

Changes since v1:
-Picked up tags from Damien.
-Moved the comment in front of ahci_mark_external_port() to inside the
 function.
-Modified the comment in patch 4/5 to more clearly state hotplug removal
 events.
-Rewrote the commit message for patch 4/5 to be more detailed.


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    | 135 +++++++++++++++++++++++-------------------
 drivers/ata/ahci.h    |   9 +--
 drivers/ata/libahci.c |   7 ---
 4 files changed, 78 insertions(+), 78 deletions(-)

-- 
2.43.0

[PATCH v2 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.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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..aa58ce615e79 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1642,6 +1642,18 @@ 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;
+
+	/* mark esata ports */
+	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 +1946,8 @@ 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;
 
+		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

[PATCH v2 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").

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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 aa58ce615e79..4d3ec6d15ad1 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1648,9 +1648,10 @@ static void ahci_mark_external_port(struct ata_port *ap)
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 tmp;
 
-	/* mark esata ports */
+	/* mark external ports (hotplug-capable, eSATA) */
 	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;
 }
 
-- 
2.43.0

[PATCH v2 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.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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 4d3ec6d15ad1..346a0f9ef246 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1655,9 +1655,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)
 
 		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

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

[Niklas Cassel]

AHCI 1.3.3, 7.3.1.1 Software Flow for Hot Plug Removal Detection states:
"To reliably detect hot plug removals, software must disable interface
power management.

Software should perform the following initialization on a port after a
device is attached:
-Set PxSCTL.IPM to 3h to disable interface power management state
 transitions.
-Set PxCMD.ALPE to ‘0’ to disable aggressive power management.
-Ensure PxIE.PRCE is set to ‘1’ to enable interrupts on hot plug removals.
-Disable device initiated interface power management by issuing the
 appropriate SET FEATURES command."

Further, AHCI 1.3.3, 7.3 Native Hot Plug Support states:
"The HBA shall set the PxSERR.DIAG.X bit to ‘1’ when a COMINIT is received
from the device. Hot plug insertions are detected via the PxIS.PCS bit
that directly reflects the PxSERR.DIAG.X bit. The HBA shall set the
PxSERR.DIAG.N bit to ‘1’ when the HBA’s internal PhyRdy signal changes
state.

Hot plug removals are detected via the PxIS.PRCS bit that directly
reflects the PxSERR.DIAG.N bit. Note that PxSERR.DIAG.N is also set
to ‘1’ on insertions and during interface power management entry/exit."

ahci_set_lpm() already disables the PxIS.PRCS interrupt if setting a
LPM policy != ATA_LPM_MAX_POWER, so we cannot detect hot plug removals
when LPM policy != ATA_LPM_MAX_POWER.

We do have PxIS.PCS interrupt enabled even for LPM policy !=
ATA_LPM_MAX_POWER, so we should theoretically still be able to detect
hot plug insertions even when LPM is enabled.

However, in practise, for LPM policy ATA_LPM_MED_POWER_WITH_DIPM,
ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MIN_POWER, if there is
no link enabled, sata_link_scr_lpm() will set SControl.DET = 0x4,
which will transition the port to the "P:Offline" state.

The P:Offline mode is described in SATA Gold 3.5a:
4.1.1.103 Phy offline:
"In this mode the host Phy is forced off and the host Phy does not
recognize nor respond to COMINIT or COMWAKE. This mode is entered by
setting the DET field of the SControl register to 0100b. This is a
mechanism for the host to turn off its Phy."

So in the P:Offline state the PHY does not recognize the unsolicited
COMINIT which is sent on a hot plug insertion.

While we could change sata_link_scr_lpm() to never power off an external
port for LPM policy != ATA_LPM_MAX_POWER (in order be able to handle hot
plug insertions), we still would not be able to handle hot plug removals.

Thus, simply modify ahci_update_initial_lpm_policy() to not enable LPM if
the port advertises itself as an external port, as this function is
already being used to set/override the initial LPM policy.

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

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 346a0f9ef246..9d052ff2b86c 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1665,6 +1665,15 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
 	if (!(hpriv->flags & AHCI_HFLAG_USE_LPM_POLICY))
 		return;
 
+	/*
+	 * AHCI contains a known incompatibility between LPM and hot-plug
+	 * removal events, see 7.3.1 Hot Plug Removal Detection and Power
+	 * Management Interaction in AHCI 1.3.1. Therefore, do not enable
+	 * LPM if the port advertises itself as an external port.
+	 */
+	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

[PATCH v2 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.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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 9d052ff2b86c..ae0a592e2185 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 },
@@ -1660,11 +1652,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;
-
 	/*
 	 * AHCI contains a known incompatibility between LPM and hot-plug
 	 * removal events, see 7.3.1 Hot Plug Removal Detection and Power
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 v2 0/5] drop low power policy board type

[Mario Limonciello]

On 2/6/2024 15:13, 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
> 
> 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, see:
> https://studylib.net/doc/10034428/esata---microsoft-center
> 
> 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.
> 
> Damien: considering that the Intel VMD + ahci_intel_pcs_quirk() bug turned
> out to have nothing to do with LPM, it was simply the fact that the
> ahci_intel_pcs_quirk() was only applied if there was an explicit entry in
> ahci_pci_tbl. So since that bug is totally unrelated to LPM, I no longer
> think that this series need to wait for a fix for that bug.
> 
> 
> Link to v1:
> https://lore.kernel.org/linux-ide/20240201161507.1147521-1-cassel@kernel.org/
> 
> Changes since v1:
> -Picked up tags from Damien.
> -Moved the comment in front of ahci_mark_external_port() to inside the
>   function.
> -Modified the comment in patch 4/5 to more clearly state hotplug removal
>   events.
> -Rewrote the commit message for patch 4/5 to be more detailed.
> 
> 

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

> 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    | 135 +++++++++++++++++++++++-------------------
>   drivers/ata/ahci.h    |   9 +--
>   drivers/ata/libahci.c |   7 ---
>   4 files changed, 78 insertions(+), 78 deletions(-)
> 

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

[Jian-Hong Pan]

Niklas Cassel <cassel@kernel.org> 於 2024年2月7日 週三 上午5:14寫道：
>
> 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.
>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 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 9d052ff2b86c..ae0a592e2185 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 },
> @@ -1660,11 +1652,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;
> -
>         /*
>          * AHCI contains a known incompatibility between LPM and hot-plug
>          * removal events, see 7.3.1 Hot Plug Removal Detection and Power
> 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
>

Acked-by: Jian-Hong Pan <jhp@endlessos.org>

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

[Jian-Hong Pan]

Niklas Cassel <cassel@kernel.org> 於 2024年2月7日 週三 上午5:14寫道：
>
> 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.
>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 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 4d3ec6d15ad1..346a0f9ef246 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1655,9 +1655,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)
>
>                 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
>

Acked-by: Jian-Hong Pan <jhp@endlessos.org>

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

[Jian-Hong Pan]

Mario Limonciello <mario.limonciello@amd.com> 於 2024年2月7日 週三 上午5:54寫道：
>
> On 2/6/2024 15:13, 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
> >
> > 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, see:
> > https://studylib.net/doc/10034428/esata---microsoft-center
> >
> > 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.
> >
> > Damien: considering that the Intel VMD + ahci_intel_pcs_quirk() bug turned
> > out to have nothing to do with LPM, it was simply the fact that the
> > ahci_intel_pcs_quirk() was only applied if there was an explicit entry in
> > ahci_pci_tbl. So since that bug is totally unrelated to LPM, I no longer
> > think that this series need to wait for a fix for that bug.
> >
> >
> > Link to v1:
> > https://lore.kernel.org/linux-ide/20240201161507.1147521-1-cassel@kernel.org/
> >
> > Changes since v1:
> > -Picked up tags from Damien.
> > -Moved the comment in front of ahci_mark_external_port() to inside the
> >   function.
> > -Modified the comment in patch 4/5 to more clearly state hotplug removal
> >   events.
> > -Rewrote the commit message for patch 4/5 to be more detailed.
> >
> >
>
> For the series:
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>
> > 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    | 135 +++++++++++++++++++++++-------------------
> >   drivers/ata/ahci.h    |   9 +--
> >   drivers/ata/libahci.c |   7 ---
> >   4 files changed, 78 insertions(+), 78 deletions(-)
> >
>

I have tested this patch series on ASUS B1400CEAE with both enabled
and disabled VMD.
The SATA storage works and binds the LPM policy correctly.

Tested-by: Jian-Hong Pan <jhp@endlessos.org>

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

[Mika Westerberg]

Hi Niklas,

On Tue, Feb 06, 2024 at 10:13:41PM +0100, Niklas Cassel wrote:
> 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

Looks good to me. Thanks for looking into it!

For the series,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

[Damien Le Moal]

On 2/7/24 06:13, Niklas Cassel wrote:
> AHCI 1.3.3, 7.3.1.1 Software Flow for Hot Plug Removal Detection states:
> "To reliably detect hot plug removals, software must disable interface
> power management.
> 
> Software should perform the following initialization on a port after a
> device is attached:
> -Set PxSCTL.IPM to 3h to disable interface power management state
>  transitions.
> -Set PxCMD.ALPE to ‘0’ to disable aggressive power management.
> -Ensure PxIE.PRCE is set to ‘1’ to enable interrupts on hot plug removals.
> -Disable device initiated interface power management by issuing the
>  appropriate SET FEATURES command."
> 
> Further, AHCI 1.3.3, 7.3 Native Hot Plug Support states:
> "The HBA shall set the PxSERR.DIAG.X bit to ‘1’ when a COMINIT is received
> from the device. Hot plug insertions are detected via the PxIS.PCS bit
> that directly reflects the PxSERR.DIAG.X bit. The HBA shall set the
> PxSERR.DIAG.N bit to ‘1’ when the HBA’s internal PhyRdy signal changes
> state.
> 
> Hot plug removals are detected via the PxIS.PRCS bit that directly
> reflects the PxSERR.DIAG.N bit. Note that PxSERR.DIAG.N is also set
> to ‘1’ on insertions and during interface power management entry/exit."
> 
> ahci_set_lpm() already disables the PxIS.PRCS interrupt if setting a
> LPM policy != ATA_LPM_MAX_POWER, so we cannot detect hot plug removals
> when LPM policy != ATA_LPM_MAX_POWER.
> 
> We do have PxIS.PCS interrupt enabled even for LPM policy !=
> ATA_LPM_MAX_POWER, so we should theoretically still be able to detect
> hot plug insertions even when LPM is enabled.
> 
> However, in practise, for LPM policy ATA_LPM_MED_POWER_WITH_DIPM,
> ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MIN_POWER, if there is
> no link enabled, sata_link_scr_lpm() will set SControl.DET = 0x4,
> which will transition the port to the "P:Offline" state.
> 
> The P:Offline mode is described in SATA Gold 3.5a:
> 4.1.1.103 Phy offline:
> "In this mode the host Phy is forced off and the host Phy does not
> recognize nor respond to COMINIT or COMWAKE. This mode is entered by
> setting the DET field of the SControl register to 0100b. This is a
> mechanism for the host to turn off its Phy."
> 
> So in the P:Offline state the PHY does not recognize the unsolicited
> COMINIT which is sent on a hot plug insertion.
> 
> While we could change sata_link_scr_lpm() to never power off an external
> port for LPM policy != ATA_LPM_MAX_POWER (in order be able to handle hot
> plug insertions), we still would not be able to handle hot plug removals.
> 
> Thus, simply modify ahci_update_initial_lpm_policy() to not enable LPM if
> the port advertises itself as an external port, as this function is
> already being used to set/override the initial LPM policy.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

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

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2/7/24 06:13, 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
> 
> 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, see:
> https://studylib.net/doc/10034428/esata---microsoft-center
> 
> 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.
> 
> Damien: considering that the Intel VMD + ahci_intel_pcs_quirk() bug turned
> out to have nothing to do with LPM, it was simply the fact that the
> ahci_intel_pcs_quirk() was only applied if there was an explicit entry in
> ahci_pci_tbl. So since that bug is totally unrelated to LPM, I no longer
> think that this series need to wait for a fix for that bug.
> 
> 
> Link to v1:
> https://lore.kernel.org/linux-ide/20240201161507.1147521-1-cassel@kernel.org/
> 
> Changes since v1:
> -Picked up tags from Damien.
> -Moved the comment in front of ahci_mark_external_port() to inside the
>  function.
> -Modified the comment in patch 4/5 to more clearly state hotplug removal
>  events.
> -Rewrote the commit message for patch 4/5 to be more detailed.

I tested the series on an AMD machine which has AMD AHCI (FCH SATA Controller
[AHCI mode] (rev 51), PCI IDs: 1022:7901) and also a Marvell 88SE9128 PCIe SATA
adapter (PCI ID 1b4b:9128).

SATA Hotplug for the Marvell adapter works out of the box using 6.8-rc3 but
*does not work* at all for the AMD AHCI (not even link down events are seen
when a drive is pulled from a front loading slot).

Applying the series on top of 6.8-rc3, hotplug with the Marvell adapter
continues to work just fine and the AMD AHCI hotplug also finally works !
When pulling a drive I see link down events and re-plugging the drive I see:

[  156.984966] ata20: found unknown device (class 0)
[  157.148919] ata20: softreset failed (device not ready)
[  162.922094] ata20: link is slow to respond, please be patient (ready=0)
[  167.193191] ata20: found unknown device (class 0)
[  167.357190] ata20: softreset failed (device not ready)
[  168.701232] ata20: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[  168.712580] ata20.00: ATA-12: ...
...
[  168.829910] sd 19:0:0:0: [sdj] Write Protect is off
[  168.835403] sd 19:0:0:0: [sdj] Mode Sense: 00 3a 00 00
[  168.835584] sd 19:0:0:0: [sdj] Write cache: enabled, read cache: enabled,
doesn't support DPO or FUA
[  168.845872] sd 19:0:0:0: [sdj] Preferred minimum I/O size 4096 bytes
[  170.601826] sd 19:0:0:0: [sdj] Attached SCSI removable disk

So this seems all good to me. Feel free to add:

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


-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Tue, Feb 06, 2024 at 10:13:41PM +0100, 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
> 
> 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, see:
> https://studylib.net/doc/10034428/esata---microsoft-center
> 
> 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.
> 
> Damien: considering that the Intel VMD + ahci_intel_pcs_quirk() bug turned
> out to have nothing to do with LPM, it was simply the fact that the
> ahci_intel_pcs_quirk() was only applied if there was an explicit entry in
> ahci_pci_tbl. So since that bug is totally unrelated to LPM, I no longer
> think that this series need to wait for a fix for that bug.
> 
> 
> Link to v1:
> https://lore.kernel.org/linux-ide/20240201161507.1147521-1-cassel@kernel.org/
> 
> Changes since v1:
> -Picked up tags from Damien.
> -Moved the comment in front of ahci_mark_external_port() to inside the
>  function.
> -Modified the comment in patch 4/5 to more clearly state hotplug removal
>  events.
> -Rewrote the commit message for patch 4/5 to be more detailed.
> 
> 
> 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    | 135 +++++++++++++++++++++++-------------------
>  drivers/ata/ahci.h    |   9 +--
>  drivers/ata/libahci.c |   7 ---
>  4 files changed, 78 insertions(+), 78 deletions(-)
> 
> -- 
> 2.43.0
> 

Applied:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.9

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

[Thomas Weißschuh]

Hi everbody,

On 2024-02-06 22:13:43+0000, 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").
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 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 aa58ce615e79..4d3ec6d15ad1 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1648,9 +1648,10 @@ static void ahci_mark_external_port(struct ata_port *ap)
>  	void __iomem *port_mmio = ahci_port_base(ap);
>  	u32 tmp;
>  
> -	/* mark esata ports */
> +	/* mark external ports (hotplug-capable, eSATA) */
>  	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;
>  }

This seems to introduce a userspace regression.

GNOME/udisks are now automounting internal disks, which they didn't before.
See [0], [1], [2]

ATA_PFLAG_EXTERNAL is translated into GENHD_FL_REMOVABLE.
(Through ata_scsiop_inq_std(), scsi_add_lun(), sd_probe())

But GENHD_FL_REMOVABLE is not meant for hotpluggable devices but for
media-changable devices (See its description in include/linux/blkdev.h).

To indicate hotplug, dev_set_removable() is to be used.

(Both end up in "removable" sysfs attributes, but these have different
semantics...)

#regzbot introduced: 45b96d65ec68f625ad26ee16d2f556e29f715005

[0] https://bbs.archlinux.org/viewtopic.php?id=295958
[1] https://github.com/storaged-project/udisks/issues/1282
[2] https://github.com/util-linux/util-linux/issues/3088

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

[Damien Le Moal]

On 6/13/24 15:34, Thomas Weißschuh wrote:
> Hi everbody,
> 
> On 2024-02-06 22:13:43+0000, 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").
>>
>> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>> 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 aa58ce615e79..4d3ec6d15ad1 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1648,9 +1648,10 @@ static void ahci_mark_external_port(struct ata_port *ap)
>>  	void __iomem *port_mmio = ahci_port_base(ap);
>>  	u32 tmp;
>>  
>> -	/* mark esata ports */
>> +	/* mark external ports (hotplug-capable, eSATA) */
>>  	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;
>>  }
> 
> This seems to introduce a userspace regression.
> 
> GNOME/udisks are now automounting internal disks, which they didn't before.
> See [0], [1], [2]
> 
> ATA_PFLAG_EXTERNAL is translated into GENHD_FL_REMOVABLE.
> (Through ata_scsiop_inq_std(), scsi_add_lun(), sd_probe())
> 
> But GENHD_FL_REMOVABLE is not meant for hotpluggable devices but for
> media-changable devices (See its description in include/linux/blkdev.h).
> 
> To indicate hotplug, dev_set_removable() is to be used.
> 
> (Both end up in "removable" sysfs attributes, but these have different
> semantics...)

This should take care of the issue.

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 37ded3875ea3..170ed47ef74a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1912,11 +1912,8 @@ static unsigned int ata_scsiop_inq_std(struct
ata_scsi_args *args, u8 *rbuf)
                2
        };

-       /* set scsi removable (RMB) bit per ata bit, or if the
-        * AHCI port says it's external (Hotplug-capable, eSATA).
-        */
-       if (ata_id_removable(args->id) ||
-           (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
+       /* Set scsi removable (RMB) bit per ata bit. */
+       if (ata_id_removable(args->id))
                hdr[1] |= (1 << 7);

        if (args->dev->class == ATA_DEV_ZAC) {

BUT, need to check what SAT & SATA-IO have to say about this.

> 
> #regzbot introduced: 45b96d65ec68f625ad26ee16d2f556e29f715005
> 
> [0] https://bbs.archlinux.org/viewtopic.php?id=295958
> [1] https://github.com/storaged-project/udisks/issues/1282
> [2] https://github.com/util-linux/util-linux/issues/3088
> 

-- 
Damien Le Moal
Western Digital Research

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

[Thomas Weißschuh]

+Cc LKML for people to find it more easily.

On 2024-06-13 17:29:31+0000, Damien Le Moal wrote:
> On 6/13/24 15:34, Thomas Weißschuh wrote:
> > Hi everbody,
> > 
> > On 2024-02-06 22:13:43+0000, 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").
> >>
> >> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> >> 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 aa58ce615e79..4d3ec6d15ad1 100644
> >> --- a/drivers/ata/ahci.c
> >> +++ b/drivers/ata/ahci.c
> >> @@ -1648,9 +1648,10 @@ static void ahci_mark_external_port(struct ata_port *ap)
> >>  	void __iomem *port_mmio = ahci_port_base(ap);
> >>  	u32 tmp;
> >>  
> >> -	/* mark esata ports */
> >> +	/* mark external ports (hotplug-capable, eSATA) */
> >>  	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;
> >>  }
> > 
> > This seems to introduce a userspace regression.
> > 
> > GNOME/udisks are now automounting internal disks, which they didn't before.
> > See [0], [1], [2]
> > 
> > ATA_PFLAG_EXTERNAL is translated into GENHD_FL_REMOVABLE.
> > (Through ata_scsiop_inq_std(), scsi_add_lun(), sd_probe())
> > 
> > But GENHD_FL_REMOVABLE is not meant for hotpluggable devices but for
> > media-changable devices (See its description in include/linux/blkdev.h).
> > 
> > To indicate hotplug, dev_set_removable() is to be used.
> > 
> > (Both end up in "removable" sysfs attributes, but these have different
> > semantics...)
> 
> This should take care of the issue.
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 37ded3875ea3..170ed47ef74a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1912,11 +1912,8 @@ static unsigned int ata_scsiop_inq_std(struct
> ata_scsi_args *args, u8 *rbuf)
>                 2
>         };
> 
> -       /* set scsi removable (RMB) bit per ata bit, or if the
> -        * AHCI port says it's external (Hotplug-capable, eSATA).
> -        */
> -       if (ata_id_removable(args->id) ||
> -           (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
> +       /* Set scsi removable (RMB) bit per ata bit. */
> +       if (ata_id_removable(args->id))
>                 hdr[1] |= (1 << 7);
> 
>         if (args->dev->class == ATA_DEV_ZAC) {

Thanks, looks good.

Tested-by: Thomas Weißschuh <linux@weissschuh.net>

> BUT, need to check what SAT & SATA-IO have to say about this.

Who takes care of this?

> > #regzbot introduced: 45b96d65ec68f625ad26ee16d2f556e29f715005
> > 
> > [0] https://bbs.archlinux.org/viewtopic.php?id=295958
> > [1] https://github.com/storaged-project/udisks/issues/1282
> > [2] https://github.com/util-linux/util-linux/issues/3088

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

[Niklas Cassel]

On Thu, Jun 13, 2024 at 05:29:31PM +0900, Damien Le Moal wrote:
> On 6/13/24 15:34, Thomas Weißschuh wrote:
> > Hi everbody,
> > 
> > On 2024-02-06 22:13:43+0000, 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").
> >>
> >> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> >> 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 aa58ce615e79..4d3ec6d15ad1 100644
> >> --- a/drivers/ata/ahci.c
> >> +++ b/drivers/ata/ahci.c
> >> @@ -1648,9 +1648,10 @@ static void ahci_mark_external_port(struct ata_port *ap)
> >>  	void __iomem *port_mmio = ahci_port_base(ap);
> >>  	u32 tmp;
> >>  
> >> -	/* mark esata ports */
> >> +	/* mark external ports (hotplug-capable, eSATA) */
> >>  	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;
> >>  }
> > 
> > This seems to introduce a userspace regression.
> > 
> > GNOME/udisks are now automounting internal disks, which they didn't before.
> > See [0], [1], [2]
> > 
> > ATA_PFLAG_EXTERNAL is translated into GENHD_FL_REMOVABLE.
> > (Through ata_scsiop_inq_std(), scsi_add_lun(), sd_probe())
> > 
> > But GENHD_FL_REMOVABLE is not meant for hotpluggable devices but for
> > media-changable devices (See its description in include/linux/blkdev.h).
> > 
> > To indicate hotplug, dev_set_removable() is to be used.
> > 
> > (Both end up in "removable" sysfs attributes, but these have different
> > semantics...)
> 
> This should take care of the issue.
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 37ded3875ea3..170ed47ef74a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1912,11 +1912,8 @@ static unsigned int ata_scsiop_inq_std(struct
> ata_scsi_args *args, u8 *rbuf)
>                 2
>         };
> 
> -       /* set scsi removable (RMB) bit per ata bit, or if the
> -        * AHCI port says it's external (Hotplug-capable, eSATA).
> -        */
> -       if (ata_id_removable(args->id) ||
> -           (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
> +       /* Set scsi removable (RMB) bit per ata bit. */
> +       if (ata_id_removable(args->id))
>                 hdr[1] |= (1 << 7);
> 
>         if (args->dev->class == ATA_DEV_ZAC) {
> 
> BUT, need to check what SAT & SATA-IO have to say about this.

This is the correct fix, and we should merge it ASAP.
(We set the RMB bit only if ata_id_removable() is set.
ata_id_removable() is defined in CFA / CFast / Compact Flash,
that can insert a card in the SATA connected reader.)


The SCSI Removable Media Bit (RMB) should only be set for removable media,
where the device stays and the media changes, e.g. CD-ROM or floppy.

This bug was originally introduced in commit:
8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as removable")
which sets the RMB bit if the port has either the eSATA bit or the hot-plug
capable bit set. The reasoning was that the author wanted his eSATA ports to
get treated like a USB stick.

This is however wrong. See "20-082r23SPC-6: Removable Medium Bit Expectations"
which has since been integrated to SPC, which states that:

"Reports have been received that some USB Memory Stick device servers set the
removable medium (RMB) bit to one. The rub comes when the medium is actually
removed, because... The device server is removed concurrently with the medium
removal. If there is no device server, then there is no device server that is
waiting to have removable medium inserted.

Sufficient numbers of SCSI analysts see such a device:
• not as a device that supports removable medium;
but
• as a removable, hot pluggable device."

The definition of the RMB bit in SPC was then updated to highlight this fact.

Thus, a USB stick should not have the RMB bit set
(and neither shall a eSATA or a hot-plug capable port).


Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
external/removable") later changed so that the RMB bit is only set for
the eSATA bit (and not for the hot-plug capable bit), because of the
exact same problem as reported here... However, treating eSATA and
hot-plug capable ports differently is of course not correct.

From AHCI spec:
Hot Plug Capable Port (HPCP): When set to ‘1’, indicates that this port’s
signal and power connectors are externally accessible via a joint signal
and power connector for blindmate device hot plug.

So a hot-plug capable port is an external port, like the commit said.

If we want to fix so that a eSATA port or external port is actually
listed as removable, then, as Thomas said, dev_set_removable() seems
to be the correct way. SCSI does have a "HOT PLUGGABLE" field in
"6.7.2 Standard INQUIRY data", so the proper way to mark the SATA
device as removable is probably to let ata_scsiop_inq_std() set
the "HOT PLUGGABLE" field correctly (if ATA_PFLAG_EXTERNAL),
such that SCSI core can call dev_set_removable() with the proper
arguments. However, right now SCSI core does not call
dev_set_removable() at all. In fact, it seems to only be used by
drivers/mmc/core/bus.c, drivers/pci/probe.c, and drivers/usb/core/hub.c.


I suggest that we:
1) Merge Damien's fix.
2) Modify SCSI to call dev_set_removable() and modify ata_scsiop_inq_std()
   to set the "HOT PLUGGABLE" field.


Kind regards,
Niklas

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

[Niklas Cassel]

On Thu, Jun 13, 2024 at 03:13:54PM +0200, Niklas Cassel wrote:
> On Thu, Jun 13, 2024 at 05:29:31PM +0900, Damien Le Moal wrote:
> > On 6/13/24 15:34, Thomas Weißschuh wrote:

> I suggest that we:
> 1) Merge Damien's fix.

This might of course result in us getting other bug reports about their
distro no longer automounting their eSATA devices... and they might
consider that a user space regression as well.
(Since that behavior has now been there since 8a3e33cf92c7 ("ata: ahci:
find eSATA ports and flag them as removable"), which was merged in 2015.)

However, considering that the definition of the RMB bit has recently
(2020) been clarified, it is quite clear that we are currently violating
the spec, so I do still think that setting the RMB bit according to spec
is the right thing to do.


> 2) Modify SCSI to call dev_set_removable() and modify ata_scsiop_inq_std()
>    to set the "HOT PLUGGABLE" field.


Kind regards,
Niklas

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

[Thomas Weißschuh]

On 2024-06-13 15:38:51+0000, Niklas Cassel wrote:
> On Thu, Jun 13, 2024 at 03:13:54PM +0200, Niklas Cassel wrote:
> > On Thu, Jun 13, 2024 at 05:29:31PM +0900, Damien Le Moal wrote:
> > > On 6/13/24 15:34, Thomas Weißschuh wrote:
> 
> > I suggest that we:
> > 1) Merge Damien's fix.
> 
> This might of course result in us getting other bug reports about their
> distro no longer automounting their eSATA devices... and they might
> consider that a user space regression as well.
> (Since that behavior has now been there since 8a3e33cf92c7 ("ata: ahci:
> find eSATA ports and flag them as removable"), which was merged in 2015.)

This is quite likely.

How about reverting the "ata: ahci: a hotplug capable port is an external"
for now and work on a proper fix, including dev_set_removable() for an
upcoming release?

> However, considering that the definition of the RMB bit has recently
> (2020) been clarified, it is quite clear that we are currently violating
> the spec, so I do still think that setting the RMB bit according to spec
> is the right thing to do.
> 
> 
> > 2) Modify SCSI to call dev_set_removable() and modify ata_scsiop_inq_std()
> >    to set the "HOT PLUGGABLE" field.
> 
> 
> Kind regards,
> Niklas

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

[Niklas Cassel]

On Thu, Jun 13, 2024 at 04:49:43PM +0200, Thomas Weißschuh wrote:
> On 2024-06-13 15:38:51+0000, Niklas Cassel wrote:
> > On Thu, Jun 13, 2024 at 03:13:54PM +0200, Niklas Cassel wrote:
> > > On Thu, Jun 13, 2024 at 05:29:31PM +0900, Damien Le Moal wrote:
> > > > On 6/13/24 15:34, Thomas Weißschuh wrote:
> > 
> > > I suggest that we:
> > > 1) Merge Damien's fix.
> > 
> > This might of course result in us getting other bug reports about their
> > distro no longer automounting their eSATA devices... and they might
> > consider that a user space regression as well.
> > (Since that behavior has now been there since 8a3e33cf92c7 ("ata: ahci:
> > find eSATA ports and flag them as removable"), which was merged in 2015.)
> 
> This is quite likely.
> 
> How about reverting the "ata: ahci: a hotplug capable port is an external"
> for now and work on a proper fix, including dev_set_removable() for an
> upcoming release?

Perhaps I'm missing something here, but how will dev_set_removable(),
which sets a different sysfs attibute solve that "problem"?

I think that dev_set_removable() can be added as a follow up patch, since
IIUC it has nothing to do with your bug report.

Calling dev_set_removable(.., DEVICE_REMOVABLE) should simply mean that
the sysfs removable attribute ("fixed"/"removable"/"unknown") will be
correct, so lsblk sees the device as hot-pluggable.
But AFAICT, udisks will not automount the device just because the removable
attribute is set to "removable". You seem to be familiar with udisks,
is this understanding correct?


To be honest, I think that it is wrong to automount devices just because they
are hot-plug capable. eSATA and hot-plug cable ports are according to the
specification both external ports, and eSATA ports are also hot-plug capable.

I guess you could trigger on an uevent if a device is attached after boot,
but automounting a device during boot seems wrong to me.

Regardless, it seems quite clear that the RMB bit should not be set for
neither eSATA nor hot-plug cable ports.


Kind regards,
Niklas

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

[Thomas Weißschuh]

On 2024-06-13 17:37:51+0000, Niklas Cassel wrote:
> On Thu, Jun 13, 2024 at 04:49:43PM +0200, Thomas Weißschuh wrote:
> > On 2024-06-13 15:38:51+0000, Niklas Cassel wrote:
> > > On Thu, Jun 13, 2024 at 03:13:54PM +0200, Niklas Cassel wrote:
> > > > On Thu, Jun 13, 2024 at 05:29:31PM +0900, Damien Le Moal wrote:
> > > > > On 6/13/24 15:34, Thomas Weißschuh wrote:
> > > 
> > > > I suggest that we:
> > > > 1) Merge Damien's fix.
> > > 
> > > This might of course result in us getting other bug reports about their
> > > distro no longer automounting their eSATA devices... and they might
> > > consider that a user space regression as well.
> > > (Since that behavior has now been there since 8a3e33cf92c7 ("ata: ahci:
> > > find eSATA ports and flag them as removable"), which was merged in 2015.)
> > 
> > This is quite likely.
> > 
> > How about reverting the "ata: ahci: a hotplug capable port is an external"
> > for now and work on a proper fix, including dev_set_removable() for an
> > upcoming release?
> 
> Perhaps I'm missing something here, but how will dev_set_removable(),
> which sets a different sysfs attibute solve that "problem"?

Indeed, it finally won't help.
But only reverting that single commit should minimize the impact on
users and give time to work on and discuss something better.

The real solution then has time to go through a proper release cycle.

> I think that dev_set_removable() can be added as a follow up patch, since
> IIUC it has nothing to do with your bug report.
> 
> Calling dev_set_removable(.., DEVICE_REMOVABLE) should simply mean that
> the sysfs removable attribute ("fixed"/"removable"/"unknown") will be
> correct, so lsblk sees the device as hot-pluggable.
> But AFAICT, udisks will not automount the device just because the removable
> attribute is set to "removable". You seem to be familiar with udisks,
> is this understanding correct?

From src/udiskslinuxblock.c:

  /* Provide easy access to _only_ the following devices
   *
   *  - anything connected via known local buses (e.g. USB or Firewire, MMC or MemoryStick)
   *  - any device with removable media
   *
   * Be careful when extending this list as we don't want to automount
   * the world when (inadvertently) connecting to a SAN.
   */

(Local policy can override this either through "noauto" in fstab or the
UDISKS_AUTO udev property, but I'm not very familiar with udisks)

> To be honest, I think that it is wrong to automount devices just because they
> are hot-plug capable. eSATA and hot-plug cable ports are according to the
> specification both external ports, and eSATA ports are also hot-plug capable.

Agreed, as does udisk.

> I guess you could trigger on an uevent if a device is attached after boot,
> but automounting a device during boot seems wrong to me.
> 
> Regardless, it seems quite clear that the RMB bit should not be set for
> neither eSATA nor hot-plug cable ports.

Ack.

Thanks,
Thomas

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

[Niklas Cassel]

On Thu, Jun 13, 2024 at 07:33:09PM +0200, Thomas Weißschuh wrote:
> On 2024-06-13 17:37:51+0000, Niklas Cassel wrote:
> > On Thu, Jun 13, 2024 at 04:49:43PM +0200, Thomas Weißschuh wrote:
> > > On 2024-06-13 15:38:51+0000, Niklas Cassel wrote:
> > > > On Thu, Jun 13, 2024 at 03:13:54PM +0200, Niklas Cassel wrote:
> > > > > On Thu, Jun 13, 2024 at 05:29:31PM +0900, Damien Le Moal wrote:
> > > > > > On 6/13/24 15:34, Thomas Weißschuh wrote:
> > > > 
> > > > > I suggest that we:
> > > > > 1) Merge Damien's fix.
> > > > 
> > > > This might of course result in us getting other bug reports about their
> > > > distro no longer automounting their eSATA devices... and they might
> > > > consider that a user space regression as well.
> > > > (Since that behavior has now been there since 8a3e33cf92c7 ("ata: ahci:
> > > > find eSATA ports and flag them as removable"), which was merged in 2015.)
> > > 
> > > This is quite likely.
> > > 
> > > How about reverting the "ata: ahci: a hotplug capable port is an external"
> > > for now and work on a proper fix, including dev_set_removable() for an
> > > upcoming release?
> > 
> > Perhaps I'm missing something here, but how will dev_set_removable(),
> > which sets a different sysfs attibute solve that "problem"?
> 
> Indeed, it finally won't help.
> But only reverting that single commit should minimize the impact on
> users and give time to work on and discuss something better.

Reverting is not a good solution, because that means that we will not
disable LPM on hot-plug capable devices, which means that we will break
hot-plug. So that would be an even more serious bug :)

In my opinion, it seems quite clear that the current code is wrong
(at least according to the SPC-6 specification), so I see no reason
why we shouldn't just make the code spec compliant.

(and if a device is not spec complinant, it should be quirked.)

Damien, if you feel otherwise, please say so.


Kind regards,
Niklas