[PATCH 00/10] Improve link power management

[PATCH 00/10] Improve link power management

[Damien Le Moal]

This patch series improves ATA link power management, mainly for the
AHCI driver. Follow-up patches will address libsas and AHCI platform
adapters.

These patches are a mix of code cleanup and LPM management improvements
around the application of an appropriate initial link power management
policy and preventing users or system daemons from changing a port link
power management policy through sysfs when the port or the device does
not support LPM policies or the port is an external port (and using LPM
would break the port hotplug capability).

Damien Le Moal (10):
  ata: libata-core: Introduce ata_dev_config_lpm()
  ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()
  ata: libata-core: Advertize device support for DIPM and HIPM features
  ata: libata-eh: Avoid unnecessary resets when revalidating devices
  ata: libata-sata: Disallow changing LPM state if not supported
  ata: ahci: Disable DIPM if host lacks support
  ata: ahci: Disallow LPM policy control for external ports
  ata: ahci: Disallow LPM policy control if not supported
  ata: libata-core: Reduce the number of messages signaling broken LPM
  ata: libata_eh: Add debug messages to ata_eh_link_set_lpm()

 drivers/ata/ahci.c        | 16 +++++++--
 drivers/ata/libata-core.c | 70 +++++++++++++++++++++++++++++----------
 drivers/ata/libata-eh.c   | 32 +++++++++++++++++-
 drivers/ata/libata-sata.c |  5 +++
 4 files changed, 102 insertions(+), 21 deletions(-)

-- 
2.50.0

[PATCH 01/10] ata: libata-core: Introduce ata_dev_config_lpm()

[Damien Le Moal]

If the port of a device does not support Device Initiated Power
Management (DIPM), that is, the port is flagged with ATA_FLAG_NO_DIPM,
the DIPM feature of a device should be disabled when the device is first
configured.

Introduce the function ata_dev_config_lpm() to disable DIPM on a
device that supports this feature if the port of the device is flagged
with ATA_FLAG_NO_DIPM. ata_dev_config_lpm() is called from
ata_dev_configure(), ensuring that a device DIPM feature is disabled
when it cannot be used.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3918ea624e0b..0d85474f6640 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2790,6 +2790,27 @@ static void ata_dev_config_cpr(struct ata_device *dev)
 	kfree(buf);
 }
 
+/*
+ * Configure features related to link power management.
+ */
+static void ata_dev_config_lpm(struct ata_device *dev)
+{
+	struct ata_port *ap = dev->link->ap;
+	unsigned int err_mask;
+
+	/*
+	 * If the device port does not support Device Initiated Power Management
+	 * (DIPM), and the device supports this feature, disable it.
+	 */
+	if (ap->flags & ATA_FLAG_NO_DIPM && ata_id_has_dipm(dev->id)) {
+		err_mask = ata_dev_set_feature(dev,
+					SETFEATURES_SATA_DISABLE, SATA_DIPM);
+		if (err_mask && err_mask != AC_ERR_DEV)
+			ata_dev_err(dev, "Disable DIPM failed, Emask 0x%x\n",
+				    err_mask);
+	}
+}
+
 static void ata_dev_print_features(struct ata_device *dev)
 {
 	if (!(dev->flags & ATA_DFLAG_FEATURES_MASK))
@@ -2963,6 +2984,7 @@ int ata_dev_configure(struct ata_device *dev)
 			ata_dev_config_chs(dev);
 		}
 
+		ata_dev_config_lpm(dev);
 		ata_dev_config_fua(dev);
 		ata_dev_config_devslp(dev);
 		ata_dev_config_sense_reporting(dev);
-- 
2.50.0

[PATCH 02/10] ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()

[Damien Le Moal]

Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a
device in ata_dev_configure() to the function ata_dev_config_lpm().
This allows having all LPM related settings in one place to facilitate
maintenance.

No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 43 +++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 0d85474f6640..fdce96fd3ffa 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev)
 	struct ata_port *ap = dev->link->ap;
 	unsigned int err_mask;
 
+	if (ap->flags & ATA_FLAG_NO_LPM) {
+		/*
+		 * When the port does not support LPM, we cannot support it on
+		 * the device either.
+		 */
+		dev->quirks |= ATA_QUIRK_NOLPM;
+	} else {
+		/*
+		 * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for
+		 * them.
+		 */
+		if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
+		    (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
+			dev->quirks |= ATA_QUIRK_NOLPM;
+
+		/* ATI specific quirk */
+		if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) &&
+		    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
+			dev->quirks |= ATA_QUIRK_NOLPM;
+	}
+
+	if (dev->quirks & ATA_QUIRK_NOLPM) {
+		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
+		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
+	}
+
 	/*
 	 * If the device port does not support Device Initiated Power Management
 	 * (DIPM), and the device supports this feature, disable it.
@@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev)
 	if (rc)
 		return rc;
 
-	/* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */
-	if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
-	    (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
-		dev->quirks |= ATA_QUIRK_NOLPM;
-
-	if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI &&
-	    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
-		dev->quirks |= ATA_QUIRK_NOLPM;
-
-	if (ap->flags & ATA_FLAG_NO_LPM)
-		dev->quirks |= ATA_QUIRK_NOLPM;
-
-	if (dev->quirks & ATA_QUIRK_NOLPM) {
-		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
-		dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER;
-	}
-
 	/* let ACPI work its magic */
 	rc = ata_acpi_on_devcfg(dev);
 	if (rc)
-- 
2.50.0

[PATCH 03/10] ata: libata-core: Advertize device support for DIPM and HIPM features

[Damien Le Moal]

Modify ata_dev_print_features() to advertize if a device supports the
Device Initiated Power Management (DIPM) and Host Initiated Power
Management (HIPM) features.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index fdce96fd3ffa..d1dff9018a3a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2843,11 +2843,13 @@ static void ata_dev_print_features(struct ata_device *dev)
 		return;
 
 	ata_dev_info(dev,
-		     "Features:%s%s%s%s%s%s%s%s\n",
+		     "Features:%s%s%s%s%s%s%s%s%s%s\n",
 		     dev->flags & ATA_DFLAG_FUA ? " FUA" : "",
 		     dev->flags & ATA_DFLAG_TRUSTED ? " Trust" : "",
 		     dev->flags & ATA_DFLAG_DA ? " Dev-Attention" : "",
 		     dev->flags & ATA_DFLAG_DEVSLP ? " Dev-Sleep" : "",
+		     ata_id_has_hipm(dev->id) ? " HIPM" : "",
+		     ata_id_has_dipm(dev->id) ? " DIPM" : "",
 		     dev->flags & ATA_DFLAG_NCQ_SEND_RECV ? " NCQ-sndrcv" : "",
 		     dev->flags & ATA_DFLAG_NCQ_PRIO ? " NCQ-prio" : "",
 		     dev->flags & ATA_DFLAG_CDL ? " CDL" : "",
-- 
2.50.0

[PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

[Damien Le Moal]

In ata_eh_revalidate_and_attach(), a link LPM policy is always
set to ATA_LPM_MAX_POWER before calling ata_dev_revalidate() to ensure
that the call to ata_phys_link_offline() does not return true, thus
causing an unnecessary device reset. This change was introduced
with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless hard reset
when revalidating link").

However, setting the link LPM policy to ATA_LPM_MAX_POWER may be
visible only after some time, depending on the power state the link was
in. E.g. transitioning out of the Partial state should take no longer
than a few microseconds, but transitioning out of the Slumber or
DevSleep state may take several milliseconds. So despite the changes
introduced with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless
hard reset when revalidating link"), we can still endup with
ata_phys_link_offline() seeing a link SCR_STATUS register signaling that
the device is present (DET is equal to 1h) but that the link PHY is
still in a low power mode (e.g. IPM is 2h, signaling "Interface in
Partial power management state"). In such cases, ata_phys_link_offline()
returns true, causing an EIO return for ata_eh_revalidate_and_attach()
and a device reset.

Avoid such unnecessary device resets by introducing a relaxed version
of the link offline test implemented by ata_phys_link_offline() with
the new helper function ata_eh_link_established(). This functions
returns true if for the link SCR_STATUS register we see that:
 - A device is still present, that is, the DET field is 1h (Device
   presence detected but Phy communication not established) or 3h
   (Device presence detected and Phy communication established).
 - Communication is established, that is, the IPM field is not 0h,
   indicating that the PHY is online or in a low power state.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index f98d5123e1e4..7f5d13f9ca73 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2071,6 +2071,33 @@ static void ata_eh_get_success_sense(struct ata_link *link)
 	ata_eh_done(link, dev, ATA_EH_GET_SUCCESS_SENSE);
 }
 
+/*
+ * Check if a link is established. This is a relaxed version of
+ * ata_phys_link_online() which accounts for the fact that this is potentially
+ * called after changing the link power management policy, which may not be
+ * reflected immediately in the SSTAUS register (e.g., we may still be seeing
+ * the PHY in partial, slumber or devsleep Partial power management state.
+ * So check that:
+ * - A device is still present, that is, DET is 1h (Device presence detected
+ *   but Phy communication not established) or 3h (Device presence detected and
+ *   Phy communication established)
+ * - Communication is established, that is, IPM is not 0h, indicating that PHY
+ *   is online or in a low power state.
+ */
+static bool ata_eh_link_established(struct ata_link *link)
+{
+	u32 sstatus;
+	u8 det, ipm;
+
+	if (sata_scr_read(link, SCR_STATUS, &sstatus))
+		return false;
+
+	det = sstatus & 0x0f;
+	ipm = (sstatus >> 8) & 0x0f;
+
+	return (det & 0x01) && ipm;
+}
+
 /**
  *	ata_eh_link_set_lpm - configure SATA interface power management
  *	@link: link to configure
@@ -3275,7 +3302,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 					goto err;
 			}
 
-			if (ata_phys_link_offline(ata_dev_phys_link(dev))) {
+			if (!ata_eh_link_established(ata_dev_phys_link(dev))) {
 				rc = -EIO;
 				goto err;
 			}
-- 
2.50.0

[PATCH 05/10] ata: libata-sata: Disallow changing LPM state if not supported

[Damien Le Moal]

Modify ata_scsi_lpm_store() to return an error if a user attempts to set
a link power management policy for a port that does not support LPM,
that is, ports flagged with ATA_FLAG_NO_LPM.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-sata.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index cb46ce276bb1..47169c469f43 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -924,6 +924,11 @@ static ssize_t ata_scsi_lpm_store(struct device *device,
 
 	spin_lock_irqsave(ap->lock, flags);
 
+	if (ap->flags & ATA_FLAG_NO_LPM) {
+		count = -EOPNOTSUPP;
+		goto out_unlock;
+	}
+
 	ata_for_each_link(link, ap, EDGE) {
 		ata_for_each_dev(dev, &ap->link, ENABLED) {
 			if (dev->quirks & ATA_QUIRK_NOLPM) {
-- 
2.50.0

[PATCH 06/10] ata: ahci: Disable DIPM if host lacks support

[Damien Le Moal]

The AHCI specification version 1.3.1 section 8.3.1.4 (Software
Requirements and Precedence) states that:

If CAP.SSC or CAP.PSC is cleared to ‘0’, software should disable
device-initiated power management by issuing the appropriate SET
FEATURES command to the device.

To satisfy this constraint and force ata_dev_configure to disable the
device DIPM feature, modify ahci_update_initial_lpm_policy() to set the
ATA_FLAG_NO_DIPM flag on ports that have a host with either the
ATA_HOST_NO_PART flag set or the ATA_HOST_NO_SSC flag set.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/ahci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1b4151d95888..0760fa47d90e 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1780,6 +1780,13 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
 		return;
 	}
 
+	/* If no Partial or no Slumber, we cannot support DIPM. */
+	if ((ap->host->flags & ATA_HOST_NO_PART) ||
+	    (ap->host->flags & ATA_HOST_NO_SSC)) {
+		ata_port_dbg(ap, "Host does not support DIPM\n");
+		ap->flags |= ATA_FLAG_NO_DIPM;
+	}
+
 	/* If no LPM states are supported by the HBA, do not bother with LPM */
 	if ((ap->host->flags & ATA_HOST_NO_PART) &&
 	    (ap->host->flags & ATA_HOST_NO_SSC) &&
-- 
2.50.0

[PATCH 07/10] ata: ahci: Disallow LPM policy control for external ports

[Damien Le Moal]

Commit ae1f3db006b7 ("ata: ahci: do not enable LPM on external ports")
added an early return in ahci_update_initial_lpm_policy() for all
ports flagged as external with the ATA_PFLAG_EXTERNAL port flag (e.g.
eSATA ports or hotplug capable ports) so that the target_lpm_policy of
these ports is unchanged and set to ATA_LPM_UNKNOWN. thus forcing libata
EH to not be called for external port. The goal of this change is to
preserve the initial power management policy to not break the hotplug
capability of external ports.

However, this change is incomplete as users or system daemon (e.g.
systemd-udevd) can still apply the system preferred power management
policy through sysfs, thus potentially unknowingly breaking the port
hotplug capability.

Modify ahci_update_initial_lpm_policy() to flag external ports with
ATA_FLAG_NO_LPM to prevent changes to the LPM policy by users through
the sysfs link_power_management_policy host attribute. Also set the
target_lpm_policy of external ports to ATA_LPM_MAX_POWER to ensure
that the port is not in a low power state preventing hotplug operations.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/ahci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 0760fa47d90e..34698ae39f55 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1776,7 +1776,9 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
 	 * LPM if the port advertises itself as an external port.
 	 */
 	if (ap->pflags & ATA_PFLAG_EXTERNAL) {
-		ata_port_dbg(ap, "external port, not enabling LPM\n");
+		ata_port_warn(ap, "External port, forcing LPM max_power\n");
+		ap->flags |= ATA_FLAG_NO_LPM;
+		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
 		return;
 	}
 
-- 
2.50.0

[PATCH 08/10] ata: ahci: Disallow LPM policy control if not supported

[Damien Le Moal]

Commit fa997b0576c9 ("ata: ahci: Do not enable LPM if no LPM states are
supported by the HBA") introduced an early return in
ahci_update_initial_lpm_policy() to ensure that the target_lpm_policy
of ports belonging to a host that does not support the Partial, Slumber
and DevSleep power states is unchanged and remains set to
ATA_LPM_UNKNOWN and thus prevents the execution of
ata_eh_link_set_lpm().

However, a user or a system daemon (e.g. systemd-udevd) may still
attempt changing the LPM policy through the sysfs
link_power_management_policy of the host.

Improve this to prevent sysfs LPM policy changes by setting the flag
ATA_FLOAG_NO_LPM for the port of such host, and initialize the port
target_lpm_policy to ATA_LPM_MAX_POWER to guarantee that no unsupported
low power state is being used on the port and its link.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/ahci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 34698ae39f55..737f5d1bde11 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1793,7 +1793,10 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
 	if ((ap->host->flags & ATA_HOST_NO_PART) &&
 	    (ap->host->flags & ATA_HOST_NO_SSC) &&
 	    (ap->host->flags & ATA_HOST_NO_DEVSLP)) {
-		ata_port_dbg(ap, "no LPM states supported, not enabling LPM\n");
+		ata_port_warn(ap,
+			"No LPM states supported, forcing LPM max_power\n");
+		ap->flags |= ATA_FLAG_NO_LPM;
+		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
 		return;
 	}
 
-- 
2.50.0

[PATCH 09/10] ata: libata-core: Reduce the number of messages signaling broken LPM

[Damien Le Moal]

ata_dev_config_lpm() prints the message "LPM support broken, forcing
max_power" for devices that have the ATA_QUIRK_NOLPM quirk flag set.
This messages is repeated for every device revalidation, which is not
necessary, but also erroneously printed for devices without a broken LPM
support when connected to a port that does not support LPM (e.g. because
the port is an external one with hotplug capability).

Since in all cases the device port target_lpm_policy is set to
ATA_LPM_MAX_POWER, avoid the "LPM broken" message repetition and
erroneous output by generating it only if the port target_lpm_policy is
not already set to ATA_LPM_MAX_POWER.

This change will suppress the "LPM broken" message for genuine cases of
a device having broken LPM if the initial LPM policy is set to
ATA_LPM_MAX_POWER through CONFIG_SATA_MOBILE_LPM_POLICY. This is not a
problem as the ATA_LPM_MAX_POWER policy implies that LPM is disabled and
unused, which is safe for devices with broken LPM.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d1dff9018a3a..3e6cf26af4e4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2819,7 +2819,8 @@ static void ata_dev_config_lpm(struct ata_device *dev)
 			dev->quirks |= ATA_QUIRK_NOLPM;
 	}
 
-	if (dev->quirks & ATA_QUIRK_NOLPM) {
+	if (dev->quirks & ATA_QUIRK_NOLPM &&
+	    ap->target_lpm_policy != ATA_LPM_MAX_POWER) {
 		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
 		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
 	}
-- 
2.50.0

[PATCH 10/10] ata: libata_eh: Add debug messages to ata_eh_link_set_lpm()

[Damien Le Moal]

To facilitate field debugging of link power management related issues,
add a debug message to ata_eh_link_set_lpm() to easily track LPM policy
changes done from EH context, that is, during device scan and
revalidation, error handling, and when a policy change is issued through
a host sysfs link_power_management_policy attribute.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 7f5d13f9ca73..7134a4ff6535 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2140,6 +2140,9 @@ static int ata_eh_link_set_lpm(struct ata_link *link,
 	if (WARN_ON_ONCE(policy == ATA_LPM_UNKNOWN))
 		return 0;
 
+	ata_link_dbg(link, "Set LPM policy: %d -> %d\n",
+		     old_policy, policy);
+
 	/*
 	 * DIPM is enabled only for ATA_LPM_MIN_POWER,
 	 * ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MED_POWER_WITH_DIPM, as
-- 
2.50.0

Re: [PATCH 08/10] ata: ahci: Disallow LPM policy control if not supported

[Sergey Shtylyov]

On 6/30/25 9:26 AM, Damien Le Moal wrote:

> Commit fa997b0576c9 ("ata: ahci: Do not enable LPM if no LPM states are
> supported by the HBA") introduced an early return in
> ahci_update_initial_lpm_policy() to ensure that the target_lpm_policy
> of ports belonging to a host that does not support the Partial, Slumber
> and DevSleep power states is unchanged and remains set to
> ATA_LPM_UNKNOWN and thus prevents the execution of
> ata_eh_link_set_lpm().
> 
> However, a user or a system daemon (e.g. systemd-udevd) may still
> attempt changing the LPM policy through the sysfs
> link_power_management_policy of the host.
> 
> Improve this to prevent sysfs LPM policy changes by setting the flag
> ATA_FLOAG_NO_LPM for the port of such host, and initialize the port

    s/FLOAG/FLAG/? :-)

> target_lpm_policy to ATA_LPM_MAX_POWER to guarantee that no unsupported
> low power state is being used on the port and its link.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

[...]

MBR, Sergey

Re: [PATCH 01/10] ata: libata-core: Introduce ata_dev_config_lpm()

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:28PM +0900, Damien Le Moal wrote:
> If the port of a device does not support Device Initiated Power
> Management (DIPM), that is, the port is flagged with ATA_FLAG_NO_DIPM,
> the DIPM feature of a device should be disabled when the device is first
> configured.
> 
> Introduce the function ata_dev_config_lpm() to disable DIPM on a
> device that supports this feature if the port of the device is flagged
> with ATA_FLAG_NO_DIPM. ata_dev_config_lpm() is called from
> ata_dev_configure(), ensuring that a device DIPM feature is disabled
> when it cannot be used.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 3918ea624e0b..0d85474f6640 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2790,6 +2790,27 @@ static void ata_dev_config_cpr(struct ata_device *dev)
>  	kfree(buf);
>  }
>  
> +/*
> + * Configure features related to link power management.
> + */
> +static void ata_dev_config_lpm(struct ata_device *dev)
> +{
> +	struct ata_port *ap = dev->link->ap;
> +	unsigned int err_mask;
> +
> +	/*
> +	 * If the device port does not support Device Initiated Power Management
> +	 * (DIPM), and the device supports this feature, disable it.

Perhaps mention in the comment that even though DIPM is disabled by default,
because of the "Software Settings Preservation feature", DIPM can be enabled
(even across a COMRESET) when using LPM policy "Keep FW settings".


> +	 */
> +	if (ap->flags & ATA_FLAG_NO_DIPM && ata_id_has_dipm(dev->id)) {
> +		err_mask = ata_dev_set_feature(dev,
> +					SETFEATURES_SATA_DISABLE, SATA_DIPM);
> +		if (err_mask && err_mask != AC_ERR_DEV)
> +			ata_dev_err(dev, "Disable DIPM failed, Emask 0x%x\n",
> +				    err_mask);
> +	}
> +}
> +
>  static void ata_dev_print_features(struct ata_device *dev)
>  {
>  	if (!(dev->flags & ATA_DFLAG_FEATURES_MASK))
> @@ -2963,6 +2984,7 @@ int ata_dev_configure(struct ata_device *dev)
>  			ata_dev_config_chs(dev);
>  		}
>  
> +		ata_dev_config_lpm(dev);
>  		ata_dev_config_fua(dev);
>  		ata_dev_config_devslp(dev);
>  		ata_dev_config_sense_reporting(dev);
> -- 
> 2.50.0
> 

Re: [PATCH 02/10] ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:29PM +0900, Damien Le Moal wrote:
> Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a
> device in ata_dev_configure() to the function ata_dev_config_lpm().
> This allows having all LPM related settings in one place to facilitate
> maintenance.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 03/10] ata: libata-core: Advertize device support for DIPM and HIPM features

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:30PM +0900, Damien Le Moal wrote:
> Modify ata_dev_print_features() to advertize if a device supports the
> Device Initiated Power Management (DIPM) and Host Initiated Power
> Management (HIPM) features.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:31PM +0900, Damien Le Moal wrote:
> In ata_eh_revalidate_and_attach(), a link LPM policy is always
> set to ATA_LPM_MAX_POWER before calling ata_dev_revalidate() to ensure
> that the call to ata_phys_link_offline() does not return true, thus
> causing an unnecessary device reset. This change was introduced
> with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless hard reset
> when revalidating link").
> 
> However, setting the link LPM policy to ATA_LPM_MAX_POWER may be
> visible only after some time, depending on the power state the link was
> in. E.g. transitioning out of the Partial state should take no longer
> than a few microseconds, but transitioning out of the Slumber or
> DevSleep state may take several milliseconds. So despite the changes
> introduced with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless
> hard reset when revalidating link"), we can still endup with
> ata_phys_link_offline() seeing a link SCR_STATUS register signaling that
> the device is present (DET is equal to 1h) but that the link PHY is
> still in a low power mode (e.g. IPM is 2h, signaling "Interface in
> Partial power management state"). In such cases, ata_phys_link_offline()
> returns true, causing an EIO return for ata_eh_revalidate_and_attach()
> and a device reset.
> 
> Avoid such unnecessary device resets by introducing a relaxed version
> of the link offline test implemented by ata_phys_link_offline() with
> the new helper function ata_eh_link_established(). This functions
> returns true if for the link SCR_STATUS register we see that:
>  - A device is still present, that is, the DET field is 1h (Device
>    presence detected but Phy communication not established) or 3h
>    (Device presence detected and Phy communication established).
>  - Communication is established, that is, the IPM field is not 0h,
>    indicating that the PHY is online or in a low power state.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 05/10] ata: libata-sata: Disallow changing LPM state if not supported

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:32PM +0900, Damien Le Moal wrote:
> Modify ata_scsi_lpm_store() to return an error if a user attempts to set
> a link power management policy for a port that does not support LPM,
> that is, ports flagged with ATA_FLAG_NO_LPM.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 06/10] ata: ahci: Disable DIPM if host lacks support

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:33PM +0900, Damien Le Moal wrote:
> The AHCI specification version 1.3.1 section 8.3.1.4 (Software
> Requirements and Precedence) states that:
> 
> If CAP.SSC or CAP.PSC is cleared to ‘0’, software should disable
> device-initiated power management by issuing the appropriate SET
> FEATURES command to the device.
> 
> To satisfy this constraint and force ata_dev_configure to disable the
> device DIPM feature, modify ahci_update_initial_lpm_policy() to set the
> ATA_FLAG_NO_DIPM flag on ports that have a host with either the
> ATA_HOST_NO_PART flag set or the ATA_HOST_NO_SSC flag set.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 07/10] ata: ahci: Disallow LPM policy control for external ports

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:34PM +0900, Damien Le Moal wrote:
> Commit ae1f3db006b7 ("ata: ahci: do not enable LPM on external ports")
> added an early return in ahci_update_initial_lpm_policy() for all
> ports flagged as external with the ATA_PFLAG_EXTERNAL port flag (e.g.
> eSATA ports or hotplug capable ports) so that the target_lpm_policy of
> these ports is unchanged and set to ATA_LPM_UNKNOWN. thus forcing libata
> EH to not be called for external port. The goal of this change is to
> preserve the initial power management policy to not break the hotplug
> capability of external ports.
> 
> However, this change is incomplete as users or system daemon (e.g.
> systemd-udevd) can still apply the system preferred power management
> policy through sysfs, thus potentially unknowingly breaking the port
> hotplug capability.
> 
> Modify ahci_update_initial_lpm_policy() to flag external ports with
> ATA_FLAG_NO_LPM to prevent changes to the LPM policy by users through
> the sysfs link_power_management_policy host attribute. Also set the
> target_lpm_policy of external ports to ATA_LPM_MAX_POWER to ensure
> that the port is not in a low power state preventing hotplug operations.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/ahci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 0760fa47d90e..34698ae39f55 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1776,7 +1776,9 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>  	 * LPM if the port advertises itself as an external port.
>  	 */
>  	if (ap->pflags & ATA_PFLAG_EXTERNAL) {
> -		ata_port_dbg(ap, "external port, not enabling LPM\n");
> +		ata_port_warn(ap, "External port, forcing LPM max_power\n");

Should this really be a warning?
External ports are quite common, so it seems like this could easily trigger 32
lines of prints on a controller with 32 ports.

We already have the " ext" print when printing each port during probe,
perhaps that is enough?


Kind regards,
Niklas

Re: [PATCH 08/10] ata: ahci: Disallow LPM policy control if not supported

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:35PM +0900, Damien Le Moal wrote:
> Commit fa997b0576c9 ("ata: ahci: Do not enable LPM if no LPM states are
> supported by the HBA") introduced an early return in
> ahci_update_initial_lpm_policy() to ensure that the target_lpm_policy
> of ports belonging to a host that does not support the Partial, Slumber
> and DevSleep power states is unchanged and remains set to
> ATA_LPM_UNKNOWN and thus prevents the execution of
> ata_eh_link_set_lpm().
> 
> However, a user or a system daemon (e.g. systemd-udevd) may still
> attempt changing the LPM policy through the sysfs
> link_power_management_policy of the host.
> 
> Improve this to prevent sysfs LPM policy changes by setting the flag
> ATA_FLOAG_NO_LPM for the port of such host, and initialize the port
> target_lpm_policy to ATA_LPM_MAX_POWER to guarantee that no unsupported
> low power state is being used on the port and its link.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/ahci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 34698ae39f55..737f5d1bde11 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1793,7 +1793,10 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>  	if ((ap->host->flags & ATA_HOST_NO_PART) &&
>  	    (ap->host->flags & ATA_HOST_NO_SSC) &&
>  	    (ap->host->flags & ATA_HOST_NO_DEVSLP)) {
> -		ata_port_dbg(ap, "no LPM states supported, not enabling LPM\n");
> +		ata_port_warn(ap,
> +			"No LPM states supported, forcing LPM max_power\n");

Do we really want this to be a warning?

I don't think there is anything wrong with an HBA that does not support any
LPM states, so it seems a bit excessive to give a warning about it.

e.g. [PATCH 06/10] ata: ahci: Disable DIPM if host lacks support
was only a at_port_dbg().


Or, if you really want to keep this warning, then perhaps we should move this
if-statement
(and the ap->pflags & ATA_PFLAG_EXTERNAL if-statement)
below the

update_policy:

label

And change it to:

if (policy != ATA_LPM_MAX_POWER &&
   (ap->host->flags & ATA_HOST_NO_PART) &&
   (ap->host->flags & ATA_HOST_NO_SSC) &&
   (ap->host->flags & ATA_HOST_NO_DEVSLP)) {


(and add the same policy != ATA_LPM_MAX_POWER &&
guard to the ap->pflags & ATA_PFLAG_EXTERNAL if-statement)

But I think that I prefer to just keep it as ata_port_dbg().


Kind regards,
Niklas

Re: [PATCH 09/10] ata: libata-core: Reduce the number of messages signaling broken LPM

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:36PM +0900, Damien Le Moal wrote:
> ata_dev_config_lpm() prints the message "LPM support broken, forcing
> max_power" for devices that have the ATA_QUIRK_NOLPM quirk flag set.
> This messages is repeated for every device revalidation, which is not
> necessary, but also erroneously printed for devices without a broken LPM
> support when connected to a port that does not support LPM (e.g. because
> the port is an external one with hotplug capability).
> 
> Since in all cases the device port target_lpm_policy is set to
> ATA_LPM_MAX_POWER, avoid the "LPM broken" message repetition and
> erroneous output by generating it only if the port target_lpm_policy is
> not already set to ATA_LPM_MAX_POWER.
> 
> This change will suppress the "LPM broken" message for genuine cases of
> a device having broken LPM if the initial LPM policy is set to
> ATA_LPM_MAX_POWER through CONFIG_SATA_MOBILE_LPM_POLICY. This is not a
> problem as the ATA_LPM_MAX_POWER policy implies that LPM is disabled and
> unused, which is safe for devices with broken LPM.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 10/10] ata: libata_eh: Add debug messages to ata_eh_link_set_lpm()

[Niklas Cassel]

On Mon, Jun 30, 2025 at 03:26:37PM +0900, Damien Le Moal wrote:
> To facilitate field debugging of link power management related issues,
> add a debug message to ata_eh_link_set_lpm() to easily track LPM policy
> changes done from EH context, that is, during device scan and
> revalidation, error handling, and when a policy change is issued through
> a host sysfs link_power_management_policy attribute.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-eh.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 7f5d13f9ca73..7134a4ff6535 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2140,6 +2140,9 @@ static int ata_eh_link_set_lpm(struct ata_link *link,
>  	if (WARN_ON_ONCE(policy == ATA_LPM_UNKNOWN))
>  		return 0;
>  
> +	ata_link_dbg(link, "Set LPM policy: %d -> %d\n",
> +		     old_policy, policy);

Nit: This is smaller than 80 columns, so it can be one line.

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH 10/10] ata: libata_eh: Add debug messages to ata_eh_link_set_lpm()

[Sergey Shtylyov]

On 6/30/25 6:11 PM, Niklas Cassel wrote:

>> To facilitate field debugging of link power management related issues,
>> add a debug message to ata_eh_link_set_lpm() to easily track LPM policy
>> changes done from EH context, that is, during device scan and
>> revalidation, error handling, and when a policy change is issued through
>> a host sysfs link_power_management_policy attribute.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  drivers/ata/libata-eh.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 7f5d13f9ca73..7134a4ff6535 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -2140,6 +2140,9 @@ static int ata_eh_link_set_lpm(struct ata_link *link,
>>  	if (WARN_ON_ONCE(policy == ATA_LPM_UNKNOWN))
>>  		return 0;
>>  
>> +	ata_link_dbg(link, "Set LPM policy: %d -> %d\n",
>> +		     old_policy, policy);
> 
> Nit: This is smaller than 80 columns, so it can be one line.

   The new checkpatch limit is 100 columns anyway... :-)

> Reviewed-by: Niklas Cassel <cassel@kernel.org>

MBR, Sergey

Re: [PATCH 01/10] ata: libata-core: Introduce ata_dev_config_lpm()

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> If the port of a device does not support Device Initiated Power
> Management (DIPM), that is, the port is flagged with ATA_FLAG_NO_DIPM,
> the DIPM feature of a device should be disabled when the device is first
> configured.
> 
> Introduce the function ata_dev_config_lpm() to disable DIPM on a
> device that supports this feature if the port of the device is flagged
> with ATA_FLAG_NO_DIPM. ata_dev_config_lpm() is called from
> ata_dev_configure(), ensuring that a device DIPM feature is disabled
> when it cannot be used.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 3918ea624e0b..0d85474f6640 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2790,6 +2790,27 @@ static void ata_dev_config_cpr(struct ata_device *dev)
>   	kfree(buf);
>   }
>   
> +/*
> + * Configure features related to link power management.
> + */
> +static void ata_dev_config_lpm(struct ata_device *dev)
> +{
> +	struct ata_port *ap = dev->link->ap;
> +	unsigned int err_mask;
> +
> +	/*
> +	 * If the device port does not support Device Initiated Power Management
> +	 * (DIPM), and the device supports this feature, disable it.
> +	 */
> +	if (ap->flags & ATA_FLAG_NO_DIPM && ata_id_has_dipm(dev->id)) {
> +		err_mask = ata_dev_set_feature(dev,
> +					SETFEATURES_SATA_DISABLE, SATA_DIPM);
> +		if (err_mask && err_mask != AC_ERR_DEV)
> +			ata_dev_err(dev, "Disable DIPM failed, Emask 0x%x\n",
> +				    err_mask);
> +	}
> +}
> +

Why is it called 'ata_dev_config_lpm()', when it actually disables DIPM?
Why not 'ata_dev_config_dipm()'?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 02/10] ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a
> device in ata_dev_configure() to the function ata_dev_config_lpm().
> This allows having all LPM related settings in one place to facilitate
> maintenance.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 43 +++++++++++++++++++++++----------------
>   1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 0d85474f6640..fdce96fd3ffa 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev)
>   	struct ata_port *ap = dev->link->ap;
>   	unsigned int err_mask;
>   
> +	if (ap->flags & ATA_FLAG_NO_LPM) {
> +		/*
> +		 * When the port does not support LPM, we cannot support it on
> +		 * the device either.
> +		 */
> +		dev->quirks |= ATA_QUIRK_NOLPM;
> +	} else {
> +		/*
> +		 * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for
> +		 * them.
> +		 */
> +		if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
> +		    (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
> +			dev->quirks |= ATA_QUIRK_NOLPM;
> +
> +		/* ATI specific quirk */
> +		if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) &&
> +		    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
> +			dev->quirks |= ATA_QUIRK_NOLPM;
> +	}
> +
> +	if (dev->quirks & ATA_QUIRK_NOLPM) {
> +		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
> +		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
> +	}
> +
>   	/*
>   	 * If the device port does not support Device Initiated Power Management
>   	 * (DIPM), and the device supports this feature, disable it.
> @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev)
>   	if (rc)
>   		return rc;
>   
> -	/* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */
> -	if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
> -	    (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
> -		dev->quirks |= ATA_QUIRK_NOLPM;
> -
> -	if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI &&
> -	    ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
> -		dev->quirks |= ATA_QUIRK_NOLPM;
> -
> -	if (ap->flags & ATA_FLAG_NO_LPM)
> -		dev->quirks |= ATA_QUIRK_NOLPM;
> -
> -	if (dev->quirks & ATA_QUIRK_NOLPM) {
> -		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
> -		dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER;
> -	}
> -
>   	/* let ACPI work its magic */
>   	rc = ata_acpi_on_devcfg(dev);
>   	if (rc)

And this now is only dealing with modifying LPM setting, independent on
any DIPM setting. Why not make two functions (one for DIPM and one for
LPM) so make matters less confusing?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 03/10] ata: libata-core: Advertize device support for DIPM and HIPM features

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> Modify ata_dev_print_features() to advertize if a device supports the
> Device Initiated Power Management (DIPM) and Host Initiated Power
> Management (HIPM) features.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index fdce96fd3ffa..d1dff9018a3a 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2843,11 +2843,13 @@ static void ata_dev_print_features(struct ata_device *dev)
>   		return;
>   
>   	ata_dev_info(dev,
> -		     "Features:%s%s%s%s%s%s%s%s\n",
> +		     "Features:%s%s%s%s%s%s%s%s%s%s\n",
>   		     dev->flags & ATA_DFLAG_FUA ? " FUA" : "",
>   		     dev->flags & ATA_DFLAG_TRUSTED ? " Trust" : "",
>   		     dev->flags & ATA_DFLAG_DA ? " Dev-Attention" : "",
>   		     dev->flags & ATA_DFLAG_DEVSLP ? " Dev-Sleep" : "",
> +		     ata_id_has_hipm(dev->id) ? " HIPM" : "",
> +		     ata_id_has_dipm(dev->id) ? " DIPM" : "",
>   		     dev->flags & ATA_DFLAG_NCQ_SEND_RECV ? " NCQ-sndrcv" : "",
>   		     dev->flags & ATA_DFLAG_NCQ_PRIO ? " NCQ-prio" : "",
>   		     dev->flags & ATA_DFLAG_CDL ? " CDL" : "",

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> In ata_eh_revalidate_and_attach(), a link LPM policy is always
> set to ATA_LPM_MAX_POWER before calling ata_dev_revalidate() to ensure
> that the call to ata_phys_link_offline() does not return true, thus
> causing an unnecessary device reset. This change was introduced
> with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless hard reset
> when revalidating link").
> 
> However, setting the link LPM policy to ATA_LPM_MAX_POWER may be
> visible only after some time, depending on the power state the link was
> in. E.g. transitioning out of the Partial state should take no longer
> than a few microseconds, but transitioning out of the Slumber or
> DevSleep state may take several milliseconds. So despite the changes
> introduced with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless
> hard reset when revalidating link"), we can still endup with
> ata_phys_link_offline() seeing a link SCR_STATUS register signaling that
> the device is present (DET is equal to 1h) but that the link PHY is
> still in a low power mode (e.g. IPM is 2h, signaling "Interface in
> Partial power management state"). In such cases, ata_phys_link_offline()
> returns true, causing an EIO return for ata_eh_revalidate_and_attach()
> and a device reset.
> 
> Avoid such unnecessary device resets by introducing a relaxed version
> of the link offline test implemented by ata_phys_link_offline() with
> the new helper function ata_eh_link_established(). This functions
> returns true if for the link SCR_STATUS register we see that:
>   - A device is still present, that is, the DET field is 1h (Device
>     presence detected but Phy communication not established) or 3h
>     (Device presence detected and Phy communication established).
>   - Communication is established, that is, the IPM field is not 0h,
>     indicating that the PHY is online or in a low power state.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-eh.c | 29 ++++++++++++++++++++++++++++-
>   1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index f98d5123e1e4..7f5d13f9ca73 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2071,6 +2071,33 @@ static void ata_eh_get_success_sense(struct ata_link *link)
>   	ata_eh_done(link, dev, ATA_EH_GET_SUCCESS_SENSE);
>   }
>   
> +/*
> + * Check if a link is established. This is a relaxed version of
> + * ata_phys_link_online() which accounts for the fact that this is potentially
> + * called after changing the link power management policy, which may not be
> + * reflected immediately in the SSTAUS register (e.g., we may still be seeing
> + * the PHY in partial, slumber or devsleep Partial power management state.
> + * So check that:
> + * - A device is still present, that is, DET is 1h (Device presence detected
> + *   but Phy communication not established) or 3h (Device presence detected and
> + *   Phy communication established)
> + * - Communication is established, that is, IPM is not 0h, indicating that PHY
> + *   is online or in a low power state.
> + */
> +static bool ata_eh_link_established(struct ata_link *link)
> +{
> +	u32 sstatus;
> +	u8 det, ipm;
> +
> +	if (sata_scr_read(link, SCR_STATUS, &sstatus))
> +		return false;
> +
> +	det = sstatus & 0x0f;
> +	ipm = (sstatus >> 8) & 0x0f;
> +
> +	return (det & 0x01) && ipm;
> +}
> +
>   /**
>    *	ata_eh_link_set_lpm - configure SATA interface power management
>    *	@link: link to configure
> @@ -3275,7 +3302,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
>   					goto err;
>   			}
>   
> -			if (ata_phys_link_offline(ata_dev_phys_link(dev))) {
> +			if (!ata_eh_link_established(ata_dev_phys_link(dev))) {
>   				rc = -EIO;
>   				goto err;
>   			}

Makes me wonder: if the phy is taking some time, don't we need to wait
at some point for the transition to complete?
 From a cursory glance we just continue, and (apparently) hope that
everything will be well eventually.
Hmm?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 05/10] ata: libata-sata: Disallow changing LPM state if not supported

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> Modify ata_scsi_lpm_store() to return an error if a user attempts to set
> a link power management policy for a port that does not support LPM,
> that is, ports flagged with ATA_FLAG_NO_LPM.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-sata.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index cb46ce276bb1..47169c469f43 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -924,6 +924,11 @@ static ssize_t ata_scsi_lpm_store(struct device *device,
>   
>   	spin_lock_irqsave(ap->lock, flags);
>   
> +	if (ap->flags & ATA_FLAG_NO_LPM) {
> +		count = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}
> +
>   	ata_for_each_link(link, ap, EDGE) {
>   		ata_for_each_dev(dev, &ap->link, ENABLED) {
>   			if (dev->quirks & ATA_QUIRK_NOLPM) {

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 06/10] ata: ahci: Disable DIPM if host lacks support

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> The AHCI specification version 1.3.1 section 8.3.1.4 (Software
> Requirements and Precedence) states that:
> 
> If CAP.SSC or CAP.PSC is cleared to ‘0’, software should disable
> device-initiated power management by issuing the appropriate SET
> FEATURES command to the device.
> 
> To satisfy this constraint and force ata_dev_configure to disable the
> device DIPM feature, modify ahci_update_initial_lpm_policy() to set the
> ATA_FLAG_NO_DIPM flag on ports that have a host with either the
> ATA_HOST_NO_PART flag set or the ATA_HOST_NO_SSC flag set.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/ahci.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 1b4151d95888..0760fa47d90e 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1780,6 +1780,13 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>   		return;
>   	}
>   
> +	/* If no Partial or no Slumber, we cannot support DIPM. */
> +	if ((ap->host->flags & ATA_HOST_NO_PART) ||
> +	    (ap->host->flags & ATA_HOST_NO_SSC)) {
> +		ata_port_dbg(ap, "Host does not support DIPM\n");
> +		ap->flags |= ATA_FLAG_NO_DIPM;
> +	}
> +
>   	/* If no LPM states are supported by the HBA, do not bother with LPM */
>   	if ((ap->host->flags & ATA_HOST_NO_PART) &&
>   	    (ap->host->flags & ATA_HOST_NO_SSC) &&

Reviewed-by: Hannes Reinecke <hare@suse.de.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 07/10] ata: ahci: Disallow LPM policy control for external ports

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> Commit ae1f3db006b7 ("ata: ahci: do not enable LPM on external ports")
> added an early return in ahci_update_initial_lpm_policy() for all
> ports flagged as external with the ATA_PFLAG_EXTERNAL port flag (e.g.
> eSATA ports or hotplug capable ports) so that the target_lpm_policy of
> these ports is unchanged and set to ATA_LPM_UNKNOWN. thus forcing libata
> EH to not be called for external port. The goal of this change is to
> preserve the initial power management policy to not break the hotplug
> capability of external ports.
> 
> However, this change is incomplete as users or system daemon (e.g.
> systemd-udevd) can still apply the system preferred power management
> policy through sysfs, thus potentially unknowingly breaking the port
> hotplug capability.
> 
> Modify ahci_update_initial_lpm_policy() to flag external ports with
> ATA_FLAG_NO_LPM to prevent changes to the LPM policy by users through
> the sysfs link_power_management_policy host attribute. Also set the
> target_lpm_policy of external ports to ATA_LPM_MAX_POWER to ensure
> that the port is not in a low power state preventing hotplug operations.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/ahci.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 0760fa47d90e..34698ae39f55 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1776,7 +1776,9 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>   	 * LPM if the port advertises itself as an external port.
>   	 */
>   	if (ap->pflags & ATA_PFLAG_EXTERNAL) {
> -		ata_port_dbg(ap, "external port, not enabling LPM\n");
> +		ata_port_warn(ap, "External port, forcing LPM max_power\n");
> +		ap->flags |= ATA_FLAG_NO_LPM;
> +		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
>   		return;
>   	}
>   
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 08/10] ata: ahci: Disallow LPM policy control if not supported

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> Commit fa997b0576c9 ("ata: ahci: Do not enable LPM if no LPM states are
> supported by the HBA") introduced an early return in
> ahci_update_initial_lpm_policy() to ensure that the target_lpm_policy
> of ports belonging to a host that does not support the Partial, Slumber
> and DevSleep power states is unchanged and remains set to
> ATA_LPM_UNKNOWN and thus prevents the execution of
> ata_eh_link_set_lpm().
> 
> However, a user or a system daemon (e.g. systemd-udevd) may still
> attempt changing the LPM policy through the sysfs
> link_power_management_policy of the host.
> 
> Improve this to prevent sysfs LPM policy changes by setting the flag
> ATA_FLOAG_NO_LPM for the port of such host, and initialize the port
> target_lpm_policy to ATA_LPM_MAX_POWER to guarantee that no unsupported
> low power state is being used on the port and its link.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/ahci.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 34698ae39f55..737f5d1bde11 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1793,7 +1793,10 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>   	if ((ap->host->flags & ATA_HOST_NO_PART) &&
>   	    (ap->host->flags & ATA_HOST_NO_SSC) &&
>   	    (ap->host->flags & ATA_HOST_NO_DEVSLP)) {
> -		ata_port_dbg(ap, "no LPM states supported, not enabling LPM\n");
> +		ata_port_warn(ap,
> +			"No LPM states supported, forcing LPM max_power\n");
> +		ap->flags |= ATA_FLAG_NO_LPM;
> +		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
>   		return;
>   	}
>   

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 09/10] ata: libata-core: Reduce the number of messages signaling broken LPM

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> ata_dev_config_lpm() prints the message "LPM support broken, forcing
> max_power" for devices that have the ATA_QUIRK_NOLPM quirk flag set.
> This messages is repeated for every device revalidation, which is not
> necessary, but also erroneously printed for devices without a broken LPM
> support when connected to a port that does not support LPM (e.g. because
> the port is an external one with hotplug capability).
> 
> Since in all cases the device port target_lpm_policy is set to
> ATA_LPM_MAX_POWER, avoid the "LPM broken" message repetition and
> erroneous output by generating it only if the port target_lpm_policy is
> not already set to ATA_LPM_MAX_POWER.
> 
> This change will suppress the "LPM broken" message for genuine cases of
> a device having broken LPM if the initial LPM policy is set to
> ATA_LPM_MAX_POWER through CONFIG_SATA_MOBILE_LPM_POLICY. This is not a
> problem as the ATA_LPM_MAX_POWER policy implies that LPM is disabled and
> unused, which is safe for devices with broken LPM.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d1dff9018a3a..3e6cf26af4e4 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2819,7 +2819,8 @@ static void ata_dev_config_lpm(struct ata_device *dev)
>   			dev->quirks |= ATA_QUIRK_NOLPM;
>   	}
>   
> -	if (dev->quirks & ATA_QUIRK_NOLPM) {
> +	if (dev->quirks & ATA_QUIRK_NOLPM &&
> +	    ap->target_lpm_policy != ATA_LPM_MAX_POWER) {
>   		ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
>   		ap->target_lpm_policy = ATA_LPM_MAX_POWER;
>   	}

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 10/10] ata: libata_eh: Add debug messages to ata_eh_link_set_lpm()

[Hannes Reinecke]

On 6/30/25 08:26, Damien Le Moal wrote:
> To facilitate field debugging of link power management related issues,
> add a debug message to ata_eh_link_set_lpm() to easily track LPM policy
> changes done from EH context, that is, during device scan and
> revalidation, error handling, and when a policy change is issued through
> a host sysfs link_power_management_policy attribute.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-eh.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 7f5d13f9ca73..7134a4ff6535 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2140,6 +2140,9 @@ static int ata_eh_link_set_lpm(struct ata_link *link,
>   	if (WARN_ON_ONCE(policy == ATA_LPM_UNKNOWN))
>   		return 0;
>   
> +	ata_link_dbg(link, "Set LPM policy: %d -> %d\n",
> +		     old_policy, policy);
> +
>   	/*
>   	 * DIPM is enabled only for ATA_LPM_MIN_POWER,
>   	 * ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MED_POWER_WITH_DIPM, as

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 02/10] ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()

[Damien Le Moal]

On 7/1/25 3:13 PM, Hannes Reinecke wrote:
> On 6/30/25 08:26, Damien Le Moal wrote:
>> Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a
>> device in ata_dev_configure() to the function ata_dev_config_lpm().
>> This allows having all LPM related settings in one place to facilitate
>> maintenance.
>>
>> No functional changes.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/ata/libata-core.c | 43 +++++++++++++++++++++++----------------
>>   1 file changed, 26 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 0d85474f6640..fdce96fd3ffa 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev)
>>       struct ata_port *ap = dev->link->ap;
>>       unsigned int err_mask;
>>   +    if (ap->flags & ATA_FLAG_NO_LPM) {
>> +        /*
>> +         * When the port does not support LPM, we cannot support it on
>> +         * the device either.
>> +         */
>> +        dev->quirks |= ATA_QUIRK_NOLPM;
>> +    } else {
>> +        /*
>> +         * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for
>> +         * them.
>> +         */
>> +        if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
>> +            (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
>> +            dev->quirks |= ATA_QUIRK_NOLPM;
>> +
>> +        /* ATI specific quirk */
>> +        if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) &&
>> +            ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
>> +            dev->quirks |= ATA_QUIRK_NOLPM;
>> +    }
>> +
>> +    if (dev->quirks & ATA_QUIRK_NOLPM) {
>> +        ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
>> +        ap->target_lpm_policy = ATA_LPM_MAX_POWER;
>> +    }
>> +
>>       /*
>>        * If the device port does not support Device Initiated Power Management
>>        * (DIPM), and the device supports this feature, disable it.
>> @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev)
>>       if (rc)
>>           return rc;
>>   -    /* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */
>> -    if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
>> -        (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
>> -        dev->quirks |= ATA_QUIRK_NOLPM;
>> -
>> -    if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI &&
>> -        ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
>> -        dev->quirks |= ATA_QUIRK_NOLPM;
>> -
>> -    if (ap->flags & ATA_FLAG_NO_LPM)
>> -        dev->quirks |= ATA_QUIRK_NOLPM;
>> -
>> -    if (dev->quirks & ATA_QUIRK_NOLPM) {
>> -        ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
>> -        dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER;
>> -    }
>> -
>>       /* let ACPI work its magic */
>>       rc = ata_acpi_on_devcfg(dev);
>>       if (rc)
> 
> And this now is only dealing with modifying LPM setting, independent on
> any DIPM setting. Why not make two functions (one for DIPM and one for
> LPM) so make matters less confusing?

"less confusing" with all LPM things is I think not possible :)

The idea is to keep things together as much as possible to facilitate
tweaking/maintenance. There is more like this coming to get port capabilities
out of ahci.c and into generic libata so that platform AHCI and libsas adapters
can be supported too. Right now, it is pretty much LPM == AHCI...

> 
> Cheers,
> 
> Hannes


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

[Damien Le Moal]

On 7/1/25 3:23 PM, Hannes Reinecke wrote:
> On 6/30/25 08:26, Damien Le Moal wrote:
>> In ata_eh_revalidate_and_attach(), a link LPM policy is always
>> set to ATA_LPM_MAX_POWER before calling ata_dev_revalidate() to ensure
>> that the call to ata_phys_link_offline() does not return true, thus
>> causing an unnecessary device reset. This change was introduced
>> with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless hard reset
>> when revalidating link").
>>
>> However, setting the link LPM policy to ATA_LPM_MAX_POWER may be
>> visible only after some time, depending on the power state the link was
>> in. E.g. transitioning out of the Partial state should take no longer
>> than a few microseconds, but transitioning out of the Slumber or
>> DevSleep state may take several milliseconds. So despite the changes
>> introduced with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless
>> hard reset when revalidating link"), we can still endup with
>> ata_phys_link_offline() seeing a link SCR_STATUS register signaling that
>> the device is present (DET is equal to 1h) but that the link PHY is
>> still in a low power mode (e.g. IPM is 2h, signaling "Interface in
>> Partial power management state"). In such cases, ata_phys_link_offline()
>> returns true, causing an EIO return for ata_eh_revalidate_and_attach()
>> and a device reset.
>>
>> Avoid such unnecessary device resets by introducing a relaxed version
>> of the link offline test implemented by ata_phys_link_offline() with
>> the new helper function ata_eh_link_established(). This functions
>> returns true if for the link SCR_STATUS register we see that:
>>   - A device is still present, that is, the DET field is 1h (Device
>>     presence detected but Phy communication not established) or 3h
>>     (Device presence detected and Phy communication established).
>>   - Communication is established, that is, the IPM field is not 0h,
>>     indicating that the PHY is online or in a low power state.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/ata/libata-eh.c | 29 ++++++++++++++++++++++++++++-
>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index f98d5123e1e4..7f5d13f9ca73 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -2071,6 +2071,33 @@ static void ata_eh_get_success_sense(struct ata_link
>> *link)
>>       ata_eh_done(link, dev, ATA_EH_GET_SUCCESS_SENSE);
>>   }
>>   +/*
>> + * Check if a link is established. This is a relaxed version of
>> + * ata_phys_link_online() which accounts for the fact that this is potentially
>> + * called after changing the link power management policy, which may not be
>> + * reflected immediately in the SSTAUS register (e.g., we may still be seeing
>> + * the PHY in partial, slumber or devsleep Partial power management state.
>> + * So check that:
>> + * - A device is still present, that is, DET is 1h (Device presence detected
>> + *   but Phy communication not established) or 3h (Device presence detected and
>> + *   Phy communication established)
>> + * - Communication is established, that is, IPM is not 0h, indicating that PHY
>> + *   is online or in a low power state.
>> + */
>> +static bool ata_eh_link_established(struct ata_link *link)
>> +{
>> +    u32 sstatus;
>> +    u8 det, ipm;
>> +
>> +    if (sata_scr_read(link, SCR_STATUS, &sstatus))
>> +        return false;
>> +
>> +    det = sstatus & 0x0f;
>> +    ipm = (sstatus >> 8) & 0x0f;
>> +
>> +    return (det & 0x01) && ipm;
>> +}
>> +
>>   /**
>>    *    ata_eh_link_set_lpm - configure SATA interface power management
>>    *    @link: link to configure
>> @@ -3275,7 +3302,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link
>> *link,
>>                       goto err;
>>               }
>>   -            if (ata_phys_link_offline(ata_dev_phys_link(dev))) {
>> +            if (!ata_eh_link_established(ata_dev_phys_link(dev))) {
>>                   rc = -EIO;
>>                   goto err;
>>               }
> 
> Makes me wonder: if the phy is taking some time, don't we need to wait
> at some point for the transition to complete?

There is a 10ms wait already in sata_link_scr_lpm() but it seems to not always
be enough. The specs say that transitions out of HIPM "shall not take more than
10ms", but hey, we all know how devices always follow the specs, right ? :)

> From a cursory glance we just continue, and (apparently) hope that
> everything will be well eventually.
> Hmm?

It is fine to continue because transitions out of DIPM/HIPM/DevSleep are
automatic if you send a command. So we actually do not need to wait at all and
probably can remove that 10ms sleep in sata_link_scr_lpm(). But I have not for now.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 02/10] ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()

[Hannes Reinecke]

On 7/1/25 08:43, Damien Le Moal wrote:
> On 7/1/25 3:13 PM, Hannes Reinecke wrote:
>> On 6/30/25 08:26, Damien Le Moal wrote:
>>> Move the various cases of setting the ATA_QUIRK_NOLPM quirk flag for a
>>> device in ata_dev_configure() to the function ata_dev_config_lpm().
>>> This allows having all LPM related settings in one place to facilitate
>>> maintenance.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>> ---
>>>    drivers/ata/libata-core.c | 43 +++++++++++++++++++++++----------------
>>>    1 file changed, 26 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 0d85474f6640..fdce96fd3ffa 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -2798,6 +2798,32 @@ static void ata_dev_config_lpm(struct ata_device *dev)
>>>        struct ata_port *ap = dev->link->ap;
>>>        unsigned int err_mask;
>>>    +    if (ap->flags & ATA_FLAG_NO_LPM) {
>>> +        /*
>>> +         * When the port does not support LPM, we cannot support it on
>>> +         * the device either.
>>> +         */
>>> +        dev->quirks |= ATA_QUIRK_NOLPM;
>>> +    } else {
>>> +        /*
>>> +         * Some WD SATA-1 drives have issues with LPM, turn on NOLPM for
>>> +         * them.
>>> +         */
>>> +        if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
>>> +            (dev->id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
>>> +            dev->quirks |= ATA_QUIRK_NOLPM;
>>> +
>>> +        /* ATI specific quirk */
>>> +        if ((dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI) &&
>>> +            ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
>>> +            dev->quirks |= ATA_QUIRK_NOLPM;
>>> +    }
>>> +
>>> +    if (dev->quirks & ATA_QUIRK_NOLPM) {
>>> +        ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
>>> +        ap->target_lpm_policy = ATA_LPM_MAX_POWER;
>>> +    }
>>> +
>>>        /*
>>>         * If the device port does not support Device Initiated Power Management
>>>         * (DIPM), and the device supports this feature, disable it.
>>> @@ -2881,23 +2907,6 @@ int ata_dev_configure(struct ata_device *dev)
>>>        if (rc)
>>>            return rc;
>>>    -    /* some WD SATA-1 drives have issues with LPM, turn on NOLPM for them */
>>> -    if ((dev->quirks & ATA_QUIRK_WD_BROKEN_LPM) &&
>>> -        (id[ATA_ID_SATA_CAPABILITY] & 0xe) == 0x2)
>>> -        dev->quirks |= ATA_QUIRK_NOLPM;
>>> -
>>> -    if (dev->quirks & ATA_QUIRK_NO_LPM_ON_ATI &&
>>> -        ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI))
>>> -        dev->quirks |= ATA_QUIRK_NOLPM;
>>> -
>>> -    if (ap->flags & ATA_FLAG_NO_LPM)
>>> -        dev->quirks |= ATA_QUIRK_NOLPM;
>>> -
>>> -    if (dev->quirks & ATA_QUIRK_NOLPM) {
>>> -        ata_dev_warn(dev, "LPM support broken, forcing max_power\n");
>>> -        dev->link->ap->target_lpm_policy = ATA_LPM_MAX_POWER;
>>> -    }
>>> -
>>>        /* let ACPI work its magic */
>>>        rc = ata_acpi_on_devcfg(dev);
>>>        if (rc)
>>
>> And this now is only dealing with modifying LPM setting, independent on
>> any DIPM setting. Why not make two functions (one for DIPM and one for
>> LPM) so make matters less confusing?
> 
> "less confusing" with all LPM things is I think not possible :)
> 
> The idea is to keep things together as much as possible to facilitate
> tweaking/maintenance. There is more like this coming to get port capabilities
> out of ahci.c and into generic libata so that platform AHCI and libsas adapters
> can be supported too. Right now, it is pretty much LPM == AHCI...
> 
Ok, makes sense.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

[Hannes Reinecke]

On 7/1/25 08:48, Damien Le Moal wrote:
> On 7/1/25 3:23 PM, Hannes Reinecke wrote:
>> On 6/30/25 08:26, Damien Le Moal wrote:
>>> In ata_eh_revalidate_and_attach(), a link LPM policy is always
>>> set to ATA_LPM_MAX_POWER before calling ata_dev_revalidate() to ensure
>>> that the call to ata_phys_link_offline() does not return true, thus
>>> causing an unnecessary device reset. This change was introduced
>>> with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless hard reset
>>> when revalidating link").
>>>
>>> However, setting the link LPM policy to ATA_LPM_MAX_POWER may be
>>> visible only after some time, depending on the power state the link was
>>> in. E.g. transitioning out of the Partial state should take no longer
>>> than a few microseconds, but transitioning out of the Slumber or
>>> DevSleep state may take several milliseconds. So despite the changes
>>> introduced with commit 71d7b6e51ad3 ("ata: libata-eh: avoid needless
>>> hard reset when revalidating link"), we can still endup with
>>> ata_phys_link_offline() seeing a link SCR_STATUS register signaling that
>>> the device is present (DET is equal to 1h) but that the link PHY is
>>> still in a low power mode (e.g. IPM is 2h, signaling "Interface in
>>> Partial power management state"). In such cases, ata_phys_link_offline()
>>> returns true, causing an EIO return for ata_eh_revalidate_and_attach()
>>> and a device reset.
>>>
>>> Avoid such unnecessary device resets by introducing a relaxed version
>>> of the link offline test implemented by ata_phys_link_offline() with
>>> the new helper function ata_eh_link_established(). This functions
>>> returns true if for the link SCR_STATUS register we see that:
>>>    - A device is still present, that is, the DET field is 1h (Device
>>>      presence detected but Phy communication not established) or 3h
>>>      (Device presence detected and Phy communication established).
>>>    - Communication is established, that is, the IPM field is not 0h,
>>>      indicating that the PHY is online or in a low power state.
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>> ---
>>>    drivers/ata/libata-eh.c | 29 ++++++++++++++++++++++++++++-
>>>    1 file changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>> index f98d5123e1e4..7f5d13f9ca73 100644
>>> --- a/drivers/ata/libata-eh.c
>>> +++ b/drivers/ata/libata-eh.c
>>> @@ -2071,6 +2071,33 @@ static void ata_eh_get_success_sense(struct ata_link
>>> *link)
>>>        ata_eh_done(link, dev, ATA_EH_GET_SUCCESS_SENSE);
>>>    }
>>>    +/*
>>> + * Check if a link is established. This is a relaxed version of
>>> + * ata_phys_link_online() which accounts for the fact that this is potentially
>>> + * called after changing the link power management policy, which may not be
>>> + * reflected immediately in the SSTAUS register (e.g., we may still be seeing
>>> + * the PHY in partial, slumber or devsleep Partial power management state.
>>> + * So check that:
>>> + * - A device is still present, that is, DET is 1h (Device presence detected
>>> + *   but Phy communication not established) or 3h (Device presence detected and
>>> + *   Phy communication established)
>>> + * - Communication is established, that is, IPM is not 0h, indicating that PHY
>>> + *   is online or in a low power state.
>>> + */
>>> +static bool ata_eh_link_established(struct ata_link *link)
>>> +{
>>> +    u32 sstatus;
>>> +    u8 det, ipm;
>>> +
>>> +    if (sata_scr_read(link, SCR_STATUS, &sstatus))
>>> +        return false;
>>> +
>>> +    det = sstatus & 0x0f;
>>> +    ipm = (sstatus >> 8) & 0x0f;
>>> +
>>> +    return (det & 0x01) && ipm;
>>> +}
>>> +
>>>    /**
>>>     *    ata_eh_link_set_lpm - configure SATA interface power management
>>>     *    @link: link to configure
>>> @@ -3275,7 +3302,7 @@ static int ata_eh_revalidate_and_attach(struct ata_link
>>> *link,
>>>                        goto err;
>>>                }
>>>    -            if (ata_phys_link_offline(ata_dev_phys_link(dev))) {
>>> +            if (!ata_eh_link_established(ata_dev_phys_link(dev))) {
>>>                    rc = -EIO;
>>>                    goto err;
>>>                }
>>
>> Makes me wonder: if the phy is taking some time, don't we need to wait
>> at some point for the transition to complete?
> 
> There is a 10ms wait already in sata_link_scr_lpm() but it seems to not always
> be enough. The specs say that transitions out of HIPM "shall not take more than
> 10ms", but hey, we all know how devices always follow the specs, right ? :)
> 
>>  From a cursory glance we just continue, and (apparently) hope that
>> everything will be well eventually.
>> Hmm?
> 
> It is fine to continue because transitions out of DIPM/HIPM/DevSleep are
> automatic if you send a command. So we actually do not need to wait at all and
> probably can remove that 10ms sleep in sata_link_scr_lpm(). But I have not for now.
> 
> 
Ah. Maybe adding that to the description.
... or maybe not, as we seemed to be the only ones caring about this
kinda stuff :-)

Reviewed-by: Hannes Reinecke <hare@suse.de.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

[Niklas Cassel]

On Tue, Jul 01, 2025 at 03:48:15PM +0900, Damien Le Moal wrote:
> 
> There is a 10ms wait already in sata_link_scr_lpm() but it seems to not always
> be enough. The specs say that transitions out of HIPM "shall not take more than
> 10ms", but hey, we all know how devices always follow the specs, right ? :)

HIPM / ALPM includes Partial, Slumber and DevSleep.

10 ms is for Slumber.

DevSleep can be much longer than that:
          - DevSleep: The Phy logic may be powered down. The exit latency from
                      this state is no longer than 20 ms, unless otherwise
                      specified by DETO in the device Identify Device Data log.


From PxDEVSLP.DETO:
  Device Sleep Exit Timeout (DETO): This field specifies the maximum duration (in
  approximate 1ms granularity) from DEVSLP de-assertion until the device is ready to
  accept OOB. The nominal value is 20ms while the max value is 255ms depending
  on device identification information.


So technically it can be 255 ms :)


Note that we do actually read and save DETO:
https://github.com/torvalds/linux/blob/v6.16-rc4/drivers/ata/libahci.c#L2293-L2295

But like you said below, with this patch, we should be able to remove the sleep
if we wanted, so I don't think that we necessarily need to increase it.


> 
> > From a cursory glance we just continue, and (apparently) hope that
> > everything will be well eventually.
> > Hmm?
> 
> It is fine to continue because transitions out of DIPM/HIPM/DevSleep are
> automatic if you send a command. So we actually do not need to wait at all and
> probably can remove that 10ms sleep in sata_link_scr_lpm(). But I have not for now.

Re: [PATCH 04/10] ata: libata-eh: Avoid unnecessary resets when revalidating devices

[Damien Le Moal]

On 7/1/25 6:24 PM, Niklas Cassel wrote:
> On Tue, Jul 01, 2025 at 03:48:15PM +0900, Damien Le Moal wrote:
>>
>> There is a 10ms wait already in sata_link_scr_lpm() but it seems to not always
>> be enough. The specs say that transitions out of HIPM "shall not take more than
>> 10ms", but hey, we all know how devices always follow the specs, right ? :)
> 
> HIPM / ALPM includes Partial, Slumber and DevSleep.
> 
> 10 ms is for Slumber.

Oops. Yes, I meant slumber when I said HIPM...
My brain is in lower power state today :)

> DevSleep can be much longer than that:
>           - DevSleep: The Phy logic may be powered down. The exit latency from
>                       this state is no longer than 20 ms, unless otherwise
>                       specified by DETO in the device Identify Device Data log.
> 
> 
> From PxDEVSLP.DETO:
>   Device Sleep Exit Timeout (DETO): This field specifies the maximum duration (in
>   approximate 1ms granularity) from DEVSLP de-assertion until the device is ready to
>   accept OOB. The nominal value is 20ms while the max value is 255ms depending
>   on device identification information.
> 
> 
> So technically it can be 255 ms :)

Yep. Hence why the 10ms sleep is sometimes not enough.

> Note that we do actually read and save DETO:
> https://github.com/torvalds/linux/blob/v6.16-rc4/drivers/ata/libahci.c#L2293-L2295
> 
> But like you said below, with this patch, we should be able to remove the sleep
> if we wanted, so I don't think that we necessarily need to increase it.

I definitely do not want to increase it !


-- 
Damien Le Moal
Western Digital Research