[PATCH 0/6] Various cleanups

[PATCH 0/6] Various cleanups

[Damien Le Moal]

6 patches to clean libata code and better document parameters (config
and AHCI driver module parameters).

Overall, no functional changes are introduced.

Damien Le Moal (6):
  ata: libata: Remove ATA_DFLAG_ZAC device flag
  ata: libata-scsi: Cleanup ata_scsi_offline_dev()
  ata: Fix SATA_MOBILE_LPM_POLICY description in Kconfig
  ata: libata: Improve LPM policies description
  ata: ahci: Clarify mobile_lpm_policy description
  ata: libata-eh: Move and rename ata_eh_set_lpm()

 drivers/ata/Kconfig       |  36 +++--
 drivers/ata/ahci.c        |   4 +-
 drivers/ata/libata-core.c |  13 +-
 drivers/ata/libata-eh.c   | 304 +++++++++++++++++++-------------------
 drivers/ata/libata-scsi.c |  20 ++-
 drivers/ata/libata.h      |   9 +-
 include/linux/libata.h    |  18 ++-
 7 files changed, 211 insertions(+), 193 deletions(-)

-- 
2.49.0

[PATCH 1/6] ata: libata: Remove ATA_DFLAG_ZAC device flag

[Damien Le Moal]

The ATA device flag ATA_DFLAG_ZAC is used to indicate if a devie is a
host managed or host aware zoned device. However, this flag is not used
in the hot path and only used during device scanning/revalidation and
for inquiry and sense SCSI command translation.

Save one bit from struct ata_device flags field by replacing this flag
with the internal helper function ata_dev_is_zac(). This function
returns true if the device class is ATA_DEV_ZAC (host managed ZAC device
case) or if its identify data reports it supports the zoned command set
(host aware ZAC device case).

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 13 +------------
 drivers/ata/libata-scsi.c |  5 ++---
 drivers/ata/libata.h      |  7 +++++++
 include/linux/libata.h    |  1 -
 4 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 79b20da0a256..3918ea624e0b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2421,18 +2421,7 @@ static void ata_dev_config_zac(struct ata_device *dev)
 	dev->zac_zones_optimal_nonseq = U32_MAX;
 	dev->zac_zones_max_open = U32_MAX;
 
-	/*
-	 * Always set the 'ZAC' flag for Host-managed devices.
-	 */
-	if (dev->class == ATA_DEV_ZAC)
-		dev->flags |= ATA_DFLAG_ZAC;
-	else if (ata_id_zoned_cap(dev->id) == 0x01)
-		/*
-		 * Check for host-aware devices.
-		 */
-		dev->flags |= ATA_DFLAG_ZAC;
-
-	if (!(dev->flags & ATA_DFLAG_ZAC))
+	if (!ata_dev_is_zac(dev))
 		return;
 
 	if (!ata_identify_page_supported(dev, ATA_LOG_ZONED_INFORMATION)) {
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a21c9895408d..ccd7651710be 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1923,8 +1923,7 @@ static unsigned int ata_scsiop_inq_00(struct ata_device *dev,
 	};
 
 	for (i = 0; i < sizeof(pages); i++) {
-		if (pages[i] == 0xb6 &&
-		    !(dev->flags & ATA_DFLAG_ZAC))
+		if (pages[i] == 0xb6 && !ata_dev_is_zac(dev))
 			continue;
 		rbuf[num_pages + 4] = pages[i];
 		num_pages++;
@@ -2181,7 +2180,7 @@ static unsigned int ata_scsiop_inq_b2(struct ata_device *dev,
 static unsigned int ata_scsiop_inq_b6(struct ata_device *dev,
 				      struct scsi_cmnd *cmd, u8 *rbuf)
 {
-	if (!(dev->flags & ATA_DFLAG_ZAC)) {
+	if (!ata_dev_is_zac(dev)) {
 		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
 		return 0;
 	}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index ce5c628fa6fd..48ee7acb87af 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -44,6 +44,13 @@ static inline bool ata_sstatus_online(u32 sstatus)
 	return (sstatus & 0xf) == 0x3;
 }
 
+static inline bool ata_dev_is_zac(struct ata_device *dev)
+{
+	/* Host managed device or host aware device */
+	return dev->class == ATA_DEV_ZAC ||
+		ata_id_zoned_cap(dev->id) == 0x01;
+}
+
 #ifdef CONFIG_ATA_FORCE
 extern void ata_force_cbl(struct ata_port *ap);
 #else
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1e5aec839041..721f0805b6c9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -144,7 +144,6 @@ enum {
 	ATA_DFLAG_DEVSLP	= (1 << 27), /* device supports Device Sleep */
 	ATA_DFLAG_ACPI_DISABLED = (1 << 28), /* ACPI for the device is disabled */
 	ATA_DFLAG_D_SENSE	= (1 << 29), /* Descriptor sense requested */
-	ATA_DFLAG_ZAC		= (1 << 30), /* ZAC device */
 
 	ATA_DFLAG_FEATURES_MASK	= (ATA_DFLAG_TRUSTED | ATA_DFLAG_DA |	\
 				   ATA_DFLAG_DEVSLP | ATA_DFLAG_NCQ_SEND_RECV | \
-- 
2.49.0

[PATCH 2/6] ata: libata-scsi: Cleanup ata_scsi_offline_dev()

[Damien Le Moal]

Change the function ata_scsi_offline_dev() to return a bool and change
this function kdoc comment to have the correct mention of its call site.
No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 15 +++++++--------
 drivers/ata/libata.h      |  2 +-
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index ccd7651710be..b502b123008a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4633,24 +4633,23 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
  *	ata_scsi_offline_dev - offline attached SCSI device
  *	@dev: ATA device to offline attached SCSI device for
  *
- *	This function is called from ata_eh_hotplug() and responsible
- *	for taking the SCSI device attached to @dev offline.  This
- *	function is called with host lock which protects dev->sdev
- *	against clearing.
+ *	This function is called from ata_eh_detach_dev() and is responsible for
+ *	taking the SCSI device attached to @dev offline.  This function is
+ *	called with host lock which protects dev->sdev against clearing.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  *
  *	RETURNS:
- *	1 if attached SCSI device exists, 0 otherwise.
+ *	true if attached SCSI device exists, false otherwise.
  */
-int ata_scsi_offline_dev(struct ata_device *dev)
+bool ata_scsi_offline_dev(struct ata_device *dev)
 {
 	if (dev->sdev) {
 		scsi_device_set_state(dev->sdev, SDEV_OFFLINE);
-		return 1;
+		return true;
 	}
-	return 0;
+	return false;
 }
 
 /**
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 48ee7acb87af..8e68f4556962 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -144,7 +144,7 @@ extern struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
 extern int ata_scsi_add_hosts(struct ata_host *host,
 			      const struct scsi_host_template *sht);
 extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
-extern int ata_scsi_offline_dev(struct ata_device *dev);
+extern bool ata_scsi_offline_dev(struct ata_device *dev);
 extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
 extern void ata_scsi_set_sense(struct ata_device *dev,
 			       struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
-- 
2.49.0

[PATCH 3/6] ata: Fix SATA_MOBILE_LPM_POLICY description in Kconfig

[Damien Le Moal]

Improve the description of the possible default SATA link power
management policies and add the missing description for policy 5.
No functional changes.

Fixes: a5ec5a7bfd1f ("ata: ahci: Support state with min power but Partial low power state")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/Kconfig | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e00536b49552..1d53d7b568bd 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -117,23 +117,39 @@ config SATA_AHCI
 
 config SATA_MOBILE_LPM_POLICY
 	int "Default SATA Link Power Management policy"
-	range 0 4
+	range 0 5
 	default 3
 	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 ubiquitous across laptops, desktops and servers.
-
-	  The value set has the following meanings:
+	  Each policy combines power saving states and features:
+	   - Partial: The Phy logic is powered but is in a reduced power
+                      state. The exit latency from this state is no longer than
+                      10us).
+	   - Slumber: The Phy logic is powered but is in an even lower power
+                      state. The exit latency from this state is potentially
+		      longer, but no longer than 10ms.
+	   - 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.
+	   - HIPM: Host Initiated Power Management (host automatic transisitons
+		   to partial and slumber).
+	   - DIPM: Device Initiated Power Management (device automatic
+		   transitions to partial and slumber).
+
+	  The possible values for the default SATA link power management
+	  policies are:
 		0 => Keep firmware settings
-		1 => Maximum performance
-		2 => Medium power
-		3 => Medium power with Device Initiated PM enabled
-		4 => Minimum power
-
-	  Note "Minimum power" is known to cause issues, including disk
-	  corruption, with some disks and should not be used.
+		1 => No power savings (maximum performance)
+		2 => HIPM (Partial)
+		3 => HIPM (Partial) and DIPM (Partial and Slumber)
+		4 => HIPM (Partial and DevSleep) and DIPM (Partial and Slumber)
+		5 => HIPM (Slumber and DevSleep) and DIPM (Partial and Slumber)
+
+	  Excluding the value 0, higher values represent policies with higher
+	  power savings.
 
 config SATA_AHCI_PLATFORM
 	tristate "Platform AHCI SATA support"
-- 
2.49.0

[PATCH 4/6] ata: libata: Improve LPM policies description

[Damien Le Moal]

Improve the comments describing the different values of enum
ata_lpm_policy in include/linux/libata.h. The comments match the
description given for the CONFIG_SATA_MOBILE_LPM_POLICY config
parameter.

No functional changes.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 include/linux/libata.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 721f0805b6c9..f8bdf167bad9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -499,16 +499,23 @@ enum ata_completion_errors {
 };
 
 /*
- * Link power management policy: If you alter this, you also need to
- * alter libata-sata.c (for the ascii descriptions)
+ * Link power management policy: If you alter this, you also need to alter
+ * the policy names used with the sysfs attribute link_power_management_policy
+ * defined in libata-sata.c
  */
 enum ata_lpm_policy {
+	/* 0 => Keep firmware settings */
 	ATA_LPM_UNKNOWN,
+	/* 1 => No power savings (maximum performance) */
 	ATA_LPM_MAX_POWER,
+	/* 2 => HIPM (Partial) */
 	ATA_LPM_MED_POWER,
-	ATA_LPM_MED_POWER_WITH_DIPM, /* Med power + DIPM as win IRST does */
-	ATA_LPM_MIN_POWER_WITH_PARTIAL, /* Min Power + partial and slumber */
-	ATA_LPM_MIN_POWER, /* Min power + no partial (slumber only) */
+	/* 3 => HIPM (Partial) and DIPM (Partial and Slumber) */
+	ATA_LPM_MED_POWER_WITH_DIPM,
+	/* 4 => HIPM (Partial and DevSleep) and DIPM (Partial and Slumber) */
+	ATA_LPM_MIN_POWER_WITH_PARTIAL,
+	/* 5 => HIPM (Slumber and DevSleep) and DIPM (Partial and Slumber) */
+	ATA_LPM_MIN_POWER,
 };
 
 enum ata_lpm_hints {
-- 
2.49.0

[PATCH 5/6] ata: ahci: Clarify mobile_lpm_policy description

[Damien Le Moal]

Despite its name, the mobile_lpm_policy module parameter defines the
default LPM policy to use for an AHCI adapter for all chipsets,
including desktop and server chipsets. Clarify this point in the
parameter description.

No functional changes.

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 e5e5c2e81d09..9347d0ec8793 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -674,7 +674,9 @@ MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
 
 static int mobile_lpm_policy = -1;
 module_param(mobile_lpm_policy, int, 0644);
-MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
+MODULE_PARM_DESC(mobile_lpm_policy,
+		 "Default LPM policy. Despite its name, this parameter applies "
+		 "to all chipsets, including desktop and servers chipsets");
 
 static char *ahci_mask_port_map;
 module_param_named(mask_port_map, ahci_mask_port_map, charp, 0444);
-- 
2.49.0

[PATCH 6/6] ata: libata-eh: Move and rename ata_eh_set_lpm()

[Damien Le Moal]

Move the definition of the function ata_eh_set_lpm() to avoid its
unnecessary forward declaration and rename the function to
ata_eh_link_set_lpm() to clarify that it acts on a link.
No functional changes.

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

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 1f90fcd2b3c4..f98d5123e1e4 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -153,8 +153,6 @@ ata_eh_cmd_timeout_table[ATA_EH_CMD_TIMEOUT_TABLE_SIZE] = {
 #undef CMDS
 
 static void __ata_port_freeze(struct ata_port *ap);
-static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
-			  struct ata_device **r_failed_dev);
 #ifdef CONFIG_PM
 static void ata_eh_handle_port_suspend(struct ata_port *ap);
 static void ata_eh_handle_port_resume(struct ata_port *ap);
@@ -2073,6 +2071,154 @@ static void ata_eh_get_success_sense(struct ata_link *link)
 	ata_eh_done(link, dev, ATA_EH_GET_SUCCESS_SENSE);
 }
 
+/**
+ *	ata_eh_link_set_lpm - configure SATA interface power management
+ *	@link: link to configure
+ *	@policy: the link power management policy
+ *	@r_failed_dev: out parameter for failed device
+ *
+ *	Enable SATA Interface power management.  This will enable
+ *	Device Interface Power Management (DIPM) for min_power and
+ *	medium_power_with_dipm policies, and then call driver specific
+ *	callbacks for enabling Host Initiated Power management.
+ *
+ *	LOCKING:
+ *	EH context.
+ *
+ *	RETURNS:
+ *	0 on success, -errno on failure.
+ */
+static int ata_eh_link_set_lpm(struct ata_link *link,
+			       enum ata_lpm_policy policy,
+			       struct ata_device **r_failed_dev)
+{
+	struct ata_port *ap = ata_is_host_link(link) ? link->ap : NULL;
+	struct ata_eh_context *ehc = &link->eh_context;
+	struct ata_device *dev, *link_dev = NULL, *lpm_dev = NULL;
+	enum ata_lpm_policy old_policy = link->lpm_policy;
+	bool host_has_dipm = !(link->ap->flags & ATA_FLAG_NO_DIPM);
+	unsigned int hints = ATA_LPM_EMPTY | ATA_LPM_HIPM;
+	unsigned int err_mask;
+	int rc;
+
+	/* if the link or host doesn't do LPM, noop */
+	if (!IS_ENABLED(CONFIG_SATA_HOST) ||
+	    (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm))
+		return 0;
+
+	/*
+	 * This function currently assumes that it will never be supplied policy
+	 * ATA_LPM_UNKNOWN.
+	 */
+	if (WARN_ON_ONCE(policy == ATA_LPM_UNKNOWN))
+		return 0;
+
+	/*
+	 * DIPM is enabled only for ATA_LPM_MIN_POWER,
+	 * ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MED_POWER_WITH_DIPM, as
+	 * some devices misbehave when the host NACKs transition to SLUMBER.
+	 */
+	ata_for_each_dev(dev, link, ENABLED) {
+		bool dev_has_hipm = ata_id_has_hipm(dev->id);
+		bool dev_has_dipm = ata_id_has_dipm(dev->id);
+
+		/* find the first enabled and LPM enabled devices */
+		if (!link_dev)
+			link_dev = dev;
+
+		if (!lpm_dev &&
+		    (dev_has_hipm || (dev_has_dipm && host_has_dipm)))
+			lpm_dev = dev;
+
+		hints &= ~ATA_LPM_EMPTY;
+		if (!dev_has_hipm)
+			hints &= ~ATA_LPM_HIPM;
+
+		/* disable DIPM before changing link config */
+		if (dev_has_dipm) {
+			err_mask = ata_dev_set_feature(dev,
+					SETFEATURES_SATA_DISABLE, SATA_DIPM);
+			if (err_mask && err_mask != AC_ERR_DEV) {
+				ata_dev_warn(dev,
+					     "failed to disable DIPM, Emask 0x%x\n",
+					     err_mask);
+				rc = -EIO;
+				goto fail;
+			}
+		}
+	}
+
+	if (ap) {
+		rc = ap->ops->set_lpm(link, policy, hints);
+		if (!rc && ap->slave_link)
+			rc = ap->ops->set_lpm(ap->slave_link, policy, hints);
+	} else
+		rc = sata_pmp_set_lpm(link, policy, hints);
+
+	/*
+	 * Attribute link config failure to the first (LPM) enabled
+	 * device on the link.
+	 */
+	if (rc) {
+		if (rc == -EOPNOTSUPP) {
+			link->flags |= ATA_LFLAG_NO_LPM;
+			return 0;
+		}
+		dev = lpm_dev ? lpm_dev : link_dev;
+		goto fail;
+	}
+
+	/*
+	 * Low level driver acked the transition.  Issue DIPM command
+	 * with the new policy set.
+	 */
+	link->lpm_policy = policy;
+	if (ap && ap->slave_link)
+		ap->slave_link->lpm_policy = policy;
+
+	/*
+	 * Host config updated, enable DIPM if transitioning to
+	 * ATA_LPM_MIN_POWER, ATA_LPM_MIN_POWER_WITH_PARTIAL, or
+	 * ATA_LPM_MED_POWER_WITH_DIPM.
+	 */
+	ata_for_each_dev(dev, link, ENABLED) {
+		bool dev_has_dipm = ata_id_has_dipm(dev->id);
+
+		if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && host_has_dipm &&
+		    dev_has_dipm) {
+			err_mask = ata_dev_set_feature(dev,
+					SETFEATURES_SATA_ENABLE, SATA_DIPM);
+			if (err_mask && err_mask != AC_ERR_DEV) {
+				ata_dev_warn(dev,
+					"failed to enable DIPM, Emask 0x%x\n",
+					err_mask);
+				rc = -EIO;
+				goto fail;
+			}
+		}
+	}
+
+	link->last_lpm_change = jiffies;
+	link->flags |= ATA_LFLAG_CHANGED;
+
+	return 0;
+
+fail:
+	/* restore the old policy */
+	link->lpm_policy = old_policy;
+	if (ap && ap->slave_link)
+		ap->slave_link->lpm_policy = old_policy;
+
+	/* if no device or only one more chance is left, disable LPM */
+	if (!dev || ehc->tries[dev->devno] <= 2) {
+		ata_link_warn(link, "disabling LPM on the link\n");
+		link->flags |= ATA_LFLAG_NO_LPM;
+	}
+	if (r_failed_dev)
+		*r_failed_dev = dev;
+	return rc;
+}
+
 /**
  *	ata_eh_link_autopsy - analyze error and determine recovery action
  *	@link: host link to perform autopsy on
@@ -3123,8 +3269,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 			 * to ap->target_lpm_policy after revalidation is done.
 			 */
 			if (link->lpm_policy > ATA_LPM_MAX_POWER) {
-				rc = ata_eh_set_lpm(link, ATA_LPM_MAX_POWER,
-						    r_failed_dev);
+				rc = ata_eh_link_set_lpm(link, ATA_LPM_MAX_POWER,
+							 r_failed_dev);
 				if (rc)
 					goto err;
 			}
@@ -3408,153 +3554,6 @@ static int ata_eh_maybe_retry_flush(struct ata_device *dev)
 	return rc;
 }
 
-/**
- *	ata_eh_set_lpm - configure SATA interface power management
- *	@link: link to configure power management
- *	@policy: the link power management policy
- *	@r_failed_dev: out parameter for failed device
- *
- *	Enable SATA Interface power management.  This will enable
- *	Device Interface Power Management (DIPM) for min_power and
- *	medium_power_with_dipm policies, and then call driver specific
- *	callbacks for enabling Host Initiated Power management.
- *
- *	LOCKING:
- *	EH context.
- *
- *	RETURNS:
- *	0 on success, -errno on failure.
- */
-static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
-			  struct ata_device **r_failed_dev)
-{
-	struct ata_port *ap = ata_is_host_link(link) ? link->ap : NULL;
-	struct ata_eh_context *ehc = &link->eh_context;
-	struct ata_device *dev, *link_dev = NULL, *lpm_dev = NULL;
-	enum ata_lpm_policy old_policy = link->lpm_policy;
-	bool host_has_dipm = !(link->ap->flags & ATA_FLAG_NO_DIPM);
-	unsigned int hints = ATA_LPM_EMPTY | ATA_LPM_HIPM;
-	unsigned int err_mask;
-	int rc;
-
-	/* if the link or host doesn't do LPM, noop */
-	if (!IS_ENABLED(CONFIG_SATA_HOST) ||
-	    (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm))
-		return 0;
-
-	/*
-	 * This function currently assumes that it will never be supplied policy
-	 * ATA_LPM_UNKNOWN.
-	 */
-	if (WARN_ON_ONCE(policy == ATA_LPM_UNKNOWN))
-		return 0;
-
-	/*
-	 * DIPM is enabled only for ATA_LPM_MIN_POWER,
-	 * ATA_LPM_MIN_POWER_WITH_PARTIAL, and ATA_LPM_MED_POWER_WITH_DIPM, as
-	 * some devices misbehave when the host NACKs transition to SLUMBER.
-	 */
-	ata_for_each_dev(dev, link, ENABLED) {
-		bool dev_has_hipm = ata_id_has_hipm(dev->id);
-		bool dev_has_dipm = ata_id_has_dipm(dev->id);
-
-		/* find the first enabled and LPM enabled devices */
-		if (!link_dev)
-			link_dev = dev;
-
-		if (!lpm_dev &&
-		    (dev_has_hipm || (dev_has_dipm && host_has_dipm)))
-			lpm_dev = dev;
-
-		hints &= ~ATA_LPM_EMPTY;
-		if (!dev_has_hipm)
-			hints &= ~ATA_LPM_HIPM;
-
-		/* disable DIPM before changing link config */
-		if (dev_has_dipm) {
-			err_mask = ata_dev_set_feature(dev,
-					SETFEATURES_SATA_DISABLE, SATA_DIPM);
-			if (err_mask && err_mask != AC_ERR_DEV) {
-				ata_dev_warn(dev,
-					     "failed to disable DIPM, Emask 0x%x\n",
-					     err_mask);
-				rc = -EIO;
-				goto fail;
-			}
-		}
-	}
-
-	if (ap) {
-		rc = ap->ops->set_lpm(link, policy, hints);
-		if (!rc && ap->slave_link)
-			rc = ap->ops->set_lpm(ap->slave_link, policy, hints);
-	} else
-		rc = sata_pmp_set_lpm(link, policy, hints);
-
-	/*
-	 * Attribute link config failure to the first (LPM) enabled
-	 * device on the link.
-	 */
-	if (rc) {
-		if (rc == -EOPNOTSUPP) {
-			link->flags |= ATA_LFLAG_NO_LPM;
-			return 0;
-		}
-		dev = lpm_dev ? lpm_dev : link_dev;
-		goto fail;
-	}
-
-	/*
-	 * Low level driver acked the transition.  Issue DIPM command
-	 * with the new policy set.
-	 */
-	link->lpm_policy = policy;
-	if (ap && ap->slave_link)
-		ap->slave_link->lpm_policy = policy;
-
-	/*
-	 * Host config updated, enable DIPM if transitioning to
-	 * ATA_LPM_MIN_POWER, ATA_LPM_MIN_POWER_WITH_PARTIAL, or
-	 * ATA_LPM_MED_POWER_WITH_DIPM.
-	 */
-	ata_for_each_dev(dev, link, ENABLED) {
-		bool dev_has_dipm = ata_id_has_dipm(dev->id);
-
-		if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && host_has_dipm &&
-		    dev_has_dipm) {
-			err_mask = ata_dev_set_feature(dev,
-					SETFEATURES_SATA_ENABLE, SATA_DIPM);
-			if (err_mask && err_mask != AC_ERR_DEV) {
-				ata_dev_warn(dev,
-					"failed to enable DIPM, Emask 0x%x\n",
-					err_mask);
-				rc = -EIO;
-				goto fail;
-			}
-		}
-	}
-
-	link->last_lpm_change = jiffies;
-	link->flags |= ATA_LFLAG_CHANGED;
-
-	return 0;
-
-fail:
-	/* restore the old policy */
-	link->lpm_policy = old_policy;
-	if (ap && ap->slave_link)
-		ap->slave_link->lpm_policy = old_policy;
-
-	/* if no device or only one more chance is left, disable LPM */
-	if (!dev || ehc->tries[dev->devno] <= 2) {
-		ata_link_warn(link, "disabling LPM on the link\n");
-		link->flags |= ATA_LFLAG_NO_LPM;
-	}
-	if (r_failed_dev)
-		*r_failed_dev = dev;
-	return rc;
-}
-
 int ata_link_nr_enabled(struct ata_link *link)
 {
 	struct ata_device *dev;
@@ -3943,7 +3942,8 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 	config_lpm:
 		/* configure link power saving */
 		if (link->lpm_policy != ap->target_lpm_policy) {
-			rc = ata_eh_set_lpm(link, ap->target_lpm_policy, &dev);
+			rc = ata_eh_link_set_lpm(link, ap->target_lpm_policy,
+						 &dev);
 			if (rc)
 				goto rest_fail;
 		}
-- 
2.49.0

Re: [PATCH 1/6] ata: libata: Remove ATA_DFLAG_ZAC device flag

[Hannes Reinecke]

On 6/27/25 03:11, Damien Le Moal wrote:
> The ATA device flag ATA_DFLAG_ZAC is used to indicate if a devie is a
> host managed or host aware zoned device. However, this flag is not used
> in the hot path and only used during device scanning/revalidation and
> for inquiry and sense SCSI command translation.
> 
> Save one bit from struct ata_device flags field by replacing this flag
> with the internal helper function ata_dev_is_zac(). This function
> returns true if the device class is ATA_DEV_ZAC (host managed ZAC device
> case) or if its identify data reports it supports the zoned command set
> (host aware ZAC device case).
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 13 +------------
>   drivers/ata/libata-scsi.c |  5 ++---
>   drivers/ata/libata.h      |  7 +++++++
>   include/linux/libata.h    |  1 -
>   4 files changed, 10 insertions(+), 16 deletions(-)
> 
Goodbye, ZAC ...

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 2/6] ata: libata-scsi: Cleanup ata_scsi_offline_dev()

[Hannes Reinecke]

On 6/27/25 03:11, Damien Le Moal wrote:
> Change the function ata_scsi_offline_dev() to return a bool and change
> this function kdoc comment to have the correct mention of its call site.
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 15 +++++++--------
>   drivers/ata/libata.h      |  2 +-
>   2 files changed, 8 insertions(+), 9 deletions(-)
> 
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 3/6] ata: Fix SATA_MOBILE_LPM_POLICY description in Kconfig

[Hannes Reinecke]

On 6/27/25 03:11, Damien Le Moal wrote:
> Improve the description of the possible default SATA link power
> management policies and add the missing description for policy 5.
> No functional changes.
> 
> Fixes: a5ec5a7bfd1f ("ata: ahci: Support state with min power but Partial low power state")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/Kconfig | 36 ++++++++++++++++++++++++++----------
>   1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e00536b49552..1d53d7b568bd 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -117,23 +117,39 @@ config SATA_AHCI
>   
>   config SATA_MOBILE_LPM_POLICY
>   	int "Default SATA Link Power Management policy"
> -	range 0 4
> +	range 0 5
>   	default 3
>   	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 ubiquitous across laptops, desktops and servers.
> -
> -	  The value set has the following meanings:
> +	  Each policy combines power saving states and features:
> +	   - Partial: The Phy logic is powered but is in a reduced power
> +                      state. The exit latency from this state is no longer than
> +                      10us).
> +	   - Slumber: The Phy logic is powered but is in an even lower power
> +                      state. The exit latency from this state is potentially
> +		      longer, but no longer than 10ms.
> +	   - 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.
> +	   - HIPM: Host Initiated Power Management (host automatic transisitons
> +		   to partial and slumber).
> +	   - DIPM: Device Initiated Power Management (device automatic
> +		   transitions to partial and slumber).
> +
> +	  The possible values for the default SATA link power management
> +	  policies are:
>   		0 => Keep firmware settings
> -		1 => Maximum performance
> -		2 => Medium power
> -		3 => Medium power with Device Initiated PM enabled
> -		4 => Minimum power
> -
> -	  Note "Minimum power" is known to cause issues, including disk
> -	  corruption, with some disks and should not be used.
> +		1 => No power savings (maximum performance)
> +		2 => HIPM (Partial)
> +		3 => HIPM (Partial) and DIPM (Partial and Slumber)
> +		4 => HIPM (Partial and DevSleep) and DIPM (Partial and Slumber)
> +		5 => HIPM (Slumber and DevSleep) and DIPM (Partial and Slumber)
> +
> +	  Excluding the value 0, higher values represent policies with higher
> +	  power savings.
>   
>   config SATA_AHCI_PLATFORM
>   	tristate "Platform AHCI SATA support"

Hmm. Isn't it worth creating an official documentation somewhere in
Documentation/* to have this available without having to look at the
source code?

Otherwise:
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 4/6] ata: libata: Improve LPM policies description

[Hannes Reinecke]

On 6/27/25 03:11, Damien Le Moal wrote:
> Improve the comments describing the different values of enum
> ata_lpm_policy in include/linux/libata.h. The comments match the
> description given for the CONFIG_SATA_MOBILE_LPM_POLICY config
> parameter.
> 
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   include/linux/libata.h | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 721f0805b6c9..f8bdf167bad9 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -499,16 +499,23 @@ enum ata_completion_errors {
>   };
>   
>   /*
> - * Link power management policy: If you alter this, you also need to
> - * alter libata-sata.c (for the ascii descriptions)
> + * Link power management policy: If you alter this, you also need to alter
> + * the policy names used with the sysfs attribute link_power_management_policy
> + * defined in libata-sata.c
>    */
>   enum ata_lpm_policy {
> +	/* 0 => Keep firmware settings */
>   	ATA_LPM_UNKNOWN,
> +	/* 1 => No power savings (maximum performance) */
>   	ATA_LPM_MAX_POWER,
> +	/* 2 => HIPM (Partial) */
>   	ATA_LPM_MED_POWER,
> -	ATA_LPM_MED_POWER_WITH_DIPM, /* Med power + DIPM as win IRST does */
> -	ATA_LPM_MIN_POWER_WITH_PARTIAL, /* Min Power + partial and slumber */
> -	ATA_LPM_MIN_POWER, /* Min power + no partial (slumber only) */
> +	/* 3 => HIPM (Partial) and DIPM (Partial and Slumber) */
> +	ATA_LPM_MED_POWER_WITH_DIPM,
> +	/* 4 => HIPM (Partial and DevSleep) and DIPM (Partial and Slumber) */
> +	ATA_LPM_MIN_POWER_WITH_PARTIAL,
> +	/* 5 => HIPM (Slumber and DevSleep) and DIPM (Partial and Slumber) */
> +	ATA_LPM_MIN_POWER,
>   };
>   
>   enum ata_lpm_hints {

It feels really weird to have the values for the enum in the _comment_.
I'd rather drop them and just have the comments without values.

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 5/6] ata: ahci: Clarify mobile_lpm_policy description

[Hannes Reinecke]

On 6/27/25 03:11, Damien Le Moal wrote:
> Despite its name, the mobile_lpm_policy module parameter defines the
> default LPM policy to use for an AHCI adapter for all chipsets,
> including desktop and server chipsets. Clarify this point in the
> parameter description.
> 
> No functional changes.
> 
> 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 e5e5c2e81d09..9347d0ec8793 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -674,7 +674,9 @@ MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
>   
>   static int mobile_lpm_policy = -1;
>   module_param(mobile_lpm_policy, int, 0644);
> -MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
> +MODULE_PARM_DESC(mobile_lpm_policy,
> +		 "Default LPM policy. Despite its name, this parameter applies "
> +		 "to all chipsets, including desktop and servers chipsets");
>   
>   static char *ahci_mask_port_map;
>   module_param_named(mask_port_map, ahci_mask_port_map, charp, 0444);

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 6/6] ata: libata-eh: Move and rename ata_eh_set_lpm()

[Hannes Reinecke]

On 6/27/25 03:11, Damien Le Moal wrote:
> Move the definition of the function ata_eh_set_lpm() to avoid its
> unnecessary forward declaration and rename the function to
> ata_eh_link_set_lpm() to clarify that it acts on a link.
> No functional changes.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-eh.c | 304 ++++++++++++++++++++--------------------
>   1 file changed, 152 insertions(+), 152 deletions(-)
> 
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 3/6] ata: Fix SATA_MOBILE_LPM_POLICY description in Kconfig

[Damien Le Moal]

On 6/27/25 15:03, Hannes Reinecke wrote:
> On 6/27/25 03:11, Damien Le Moal wrote:
>> Improve the description of the possible default SATA link power
>> management policies and add the missing description for policy 5.
>> No functional changes.
>>
>> Fixes: a5ec5a7bfd1f ("ata: ahci: Support state with min power but Partial low power state")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/ata/Kconfig | 36 ++++++++++++++++++++++++++----------
>>   1 file changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index e00536b49552..1d53d7b568bd 100644
>> --- a/drivers/ata/Kconfig
>> +++ b/drivers/ata/Kconfig
>> @@ -117,23 +117,39 @@ config SATA_AHCI
>>   
>>   config SATA_MOBILE_LPM_POLICY
>>   	int "Default SATA Link Power Management policy"
>> -	range 0 4
>> +	range 0 5
>>   	default 3
>>   	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 ubiquitous across laptops, desktops and servers.
>> -
>> -	  The value set has the following meanings:
>> +	  Each policy combines power saving states and features:
>> +	   - Partial: The Phy logic is powered but is in a reduced power
>> +                      state. The exit latency from this state is no longer than
>> +                      10us).
>> +	   - Slumber: The Phy logic is powered but is in an even lower power
>> +                      state. The exit latency from this state is potentially
>> +		      longer, but no longer than 10ms.
>> +	   - 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.
>> +	   - HIPM: Host Initiated Power Management (host automatic transisitons
>> +		   to partial and slumber).
>> +	   - DIPM: Device Initiated Power Management (device automatic
>> +		   transitions to partial and slumber).
>> +
>> +	  The possible values for the default SATA link power management
>> +	  policies are:
>>   		0 => Keep firmware settings
>> -		1 => Maximum performance
>> -		2 => Medium power
>> -		3 => Medium power with Device Initiated PM enabled
>> -		4 => Minimum power
>> -
>> -	  Note "Minimum power" is known to cause issues, including disk
>> -	  corruption, with some disks and should not be used.
>> +		1 => No power savings (maximum performance)
>> +		2 => HIPM (Partial)
>> +		3 => HIPM (Partial) and DIPM (Partial and Slumber)
>> +		4 => HIPM (Partial and DevSleep) and DIPM (Partial and Slumber)
>> +		5 => HIPM (Slumber and DevSleep) and DIPM (Partial and Slumber)
>> +
>> +	  Excluding the value 0, higher values represent policies with higher
>> +	  power savings.
>>   
>>   config SATA_AHCI_PLATFORM
>>   	tristate "Platform AHCI SATA support"
> 
> Hmm. Isn't it worth creating an official documentation somewhere in
> Documentation/* to have this available without having to look at the
> source code?

Yes, I am considering this.
I did not write it in the cover letter, but these patches are the first part of
a much bigger patch series that addresses a lot of LPM issues that we recently
discovered doing completely unrelated tests. Once that is out, documenting
everything will be a good thing to do.

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


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/6] ata: libata: Improve LPM policies description

[Damien Le Moal]

On 6/27/25 15:04, Hannes Reinecke wrote:
> On 6/27/25 03:11, Damien Le Moal wrote:
>> Improve the comments describing the different values of enum
>> ata_lpm_policy in include/linux/libata.h. The comments match the
>> description given for the CONFIG_SATA_MOBILE_LPM_POLICY config
>> parameter.
>>
>> No functional changes.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   include/linux/libata.h | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 721f0805b6c9..f8bdf167bad9 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -499,16 +499,23 @@ enum ata_completion_errors {
>>   };
>>   
>>   /*
>> - * Link power management policy: If you alter this, you also need to
>> - * alter libata-sata.c (for the ascii descriptions)
>> + * Link power management policy: If you alter this, you also need to alter
>> + * the policy names used with the sysfs attribute link_power_management_policy
>> + * defined in libata-sata.c
>>    */
>>   enum ata_lpm_policy {
>> +	/* 0 => Keep firmware settings */
>>   	ATA_LPM_UNKNOWN,
>> +	/* 1 => No power savings (maximum performance) */
>>   	ATA_LPM_MAX_POWER,
>> +	/* 2 => HIPM (Partial) */
>>   	ATA_LPM_MED_POWER,
>> -	ATA_LPM_MED_POWER_WITH_DIPM, /* Med power + DIPM as win IRST does */
>> -	ATA_LPM_MIN_POWER_WITH_PARTIAL, /* Min Power + partial and slumber */
>> -	ATA_LPM_MIN_POWER, /* Min power + no partial (slumber only) */
>> +	/* 3 => HIPM (Partial) and DIPM (Partial and Slumber) */
>> +	ATA_LPM_MED_POWER_WITH_DIPM,
>> +	/* 4 => HIPM (Partial and DevSleep) and DIPM (Partial and Slumber) */
>> +	ATA_LPM_MIN_POWER_WITH_PARTIAL,
>> +	/* 5 => HIPM (Slumber and DevSleep) and DIPM (Partial and Slumber) */
>> +	ATA_LPM_MIN_POWER,
>>   };
>>   
>>   enum ata_lpm_hints {
> 
> It feels really weird to have the values for the enum in the _comment_.
> I'd rather drop them and just have the comments without values.

That is fair. The intent here was to link with CONFIG_SATA_MOBILE_LPM_POLICY
which defines the default LPM policy to use. The possible values of this enum
are what goes into that config option. But I can state that in the comment above
the enum.

> 
> Cheers,
> 
> Hannes


-- 
Damien Le Moal
Western Digital Research