Skip to contentSkip to conversation

ata_eh_set_lpm() cleanups

PATCH Superseded Niklas Cassel 2 participants 13 messages May 14, 2025 – May 15, 2025 libata

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Niklas Cassel

author maintainer
#0 lore raw

cover ata_eh_set_lpm() cleanups

Hello Damien,

I was trying to understand ata_eh_set_lpm(), and decided that it needed some cleanups to make the code more understandable.

Please have a look.

Niklas Cassel (7):
ata: libata-eh: Update DIPM comments to reflect reality
ata: libata-eh: Add ata_eh_set_lpm() WARN_ON
ata: libata-eh: Drop dipm variable
ata: libata-eh: Introduce dev_has_dipm and dev_has_hipm variables
ata: libata-eh: Rename no_dipm variable to be more clear
ata: libata-eh: Host support has nothing to do with disabling DIPM
ata: libata-eh: Always disable DIPM before calling set_lpm() callback

 drivers/ata/libata-eh.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)
Signature · 1 line
2.49.0
author maintainer
#1 lore raw

PATCH 1/7 ata: libata-eh: Update DIPM comments to reflect reality

The comments describing which LPM policies that has DIPM enabled predates the introduction of the LPM policies ATA_LPM_MIN_POWER_WITH_PARTIAL and ATA_LPM_MED_POWER_WITH_DIPM.

Update the DIPM comments to reflect reality.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/ata/libata-eh.c +10 -5
index b990c1ee0b12..edbc5d7572d1 100644
@@ -3443,10 +3443,11 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		return 0;
 
 	/*
-	 * DIPM is enabled only for MIN_POWER as some devices
-	 * misbehave when the host NACKs transition to SLUMBER.  Order
-	 * device and link configurations such that the host always
-	 * allows DIPM requests.
+	 * 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.
+	 * Order device and link configurations such that the host always allows
+	 * DIPM requests.
 	 */
 	ata_for_each_dev(dev, link, ENABLED) {
 		bool hipm = ata_id_has_hipm(dev->id);

@@ -3505,7 +3506,11 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	if (ap && ap->slave_link)
 		ap->slave_link->lpm_policy = policy;
 
-	/* host config updated, enable DIPM if transitioning to MIN_POWER */
+	/*
+	 * 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) {
 		if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && !no_dipm &&
 		    ata_id_has_dipm(dev->id)) {
Signature · 1 line
2.49.0
author maintainer
#2 lore raw

PATCH 2/7 ata: libata-eh: Add ata_eh_set_lpm() WARN_ON

In reply to Niklas Cassel

link->lpm_policy is initialized to ATA_LPM_UNKNOWN in ata_eh_reset().

ata_eh_set_lpm() is then only called if link->lpm_policy != ap->target_lpm_policy (after reset)

and then only if link->lpm_policy > ATA_LPM_MAX_POWER (before revalidation).

This means that ata_eh_set_lpm() is currently never called with policy == ATA_LPM_UNKNOWN.

Add a WARN_ON so that it is more obvious from reading the code that this function is never called with policy == ATA_LPM_UNKNOWN.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/ata/libata-eh.c +7 -0
index edbc5d7572d1..d2ccdb9a2840 100644
@@ -3442,6 +3442,13 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	    (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(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
Signature · 1 line
2.49.0
maintainer
#3 lore raw

Re: [PATCH 2/7] ata: libata-eh: Add ata_eh_set_lpm() WARN_ON

Quoted 24 lines · Niklas Cassel

link->lpm_policy is initialized to ATA_LPM_UNKNOWN in ata_eh_reset().

ata_eh_set_lpm() is then only called if link->lpm_policy != ap->target_lpm_policy (after reset)

and then only if link->lpm_policy > ATA_LPM_MAX_POWER (before revalidation).

This means that ata_eh_set_lpm() is currently never called with policy == ATA_LPM_UNKNOWN.

Add a WARN_ON so that it is more obvious from reading the code that this function is never called with policy == ATA_LPM_UNKNOWN.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/ata/libata-eh.c +0 -0
index edbc5d7572d1..d2ccdb9a2840 100644

drivers/ata/libata-eh.c static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, · line 3449

Quoted 8 lines
drivers/ata/libata-eh.c +5 -0
@@ -3442,6 +3442,13 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	    (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(policy == ATA_LPM_UNKNOWN))

WARN_ON_ONCE() would be better here.

Quoted 5 lines
drivers/ata/libata-eh.c +2 -0
+		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
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#4 lore raw

PATCH 3/7 ata: libata-eh: Drop dipm variable

In reply to Niklas Cassel

The dipm variable is confusing.
Drop the variable and inline the expression at places where it is used.

This will make it easier to perform additional cleanups.

No functional change.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/ata/libata-eh.c +4 -3
index d2ccdb9a2840..34167263af87 100644
@@ -3458,13 +3458,13 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	 */
 	ata_for_each_dev(dev, link, ENABLED) {
 		bool hipm = ata_id_has_hipm(dev->id);
-		bool dipm = ata_id_has_dipm(dev->id) && !no_dipm;
 
 		/* find the first enabled and LPM enabled devices */
 		if (!link_dev)
 			link_dev = dev;
 
-		if (!lpm_dev && (hipm || dipm))
+		if (!lpm_dev &&
+		    (hipm || (ata_id_has_dipm(dev->id) && !no_dipm)))
 			lpm_dev = dev;
 
 		hints &= ~ATA_LPM_EMPTY;

@@ -3472,7 +3472,8 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dipm) {
+		if (policy < ATA_LPM_MED_POWER_WITH_DIPM &&
+		    (ata_id_has_dipm(dev->id) && !no_dipm)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
Signature · 1 line
2.49.0
maintainer
#5 lore raw

Re: [PATCH 3/7] ata: libata-eh: Drop dipm variable

Quoted 8 lines · Niklas Cassel

The dipm variable is confusing.
Drop the variable and inline the expression at places where it is used.

This will make it easier to perform additional cleanups.

No functional change.

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

I do not see the point of this patch given what patch 4 does. Can you squash 3 /7 and & 4/7 together ?

Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#6 lore raw

PATCH 4/7 ata: libata-eh: Introduce dev_has_dipm and dev_has_hipm variables

In reply to Niklas Cassel

Introduce dev_has_dipm and dev_has_hipm variables to make the code easier to read.

The dev_has_dipm variable is simply the hipm variable with a clearer name.

No functional change.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/ata/libata-eh.c +8 -6
index 34167263af87..272d074b78a1 100644
@@ -3457,23 +3457,23 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	 * DIPM requests.
 	 */
 	ata_for_each_dev(dev, link, ENABLED) {
-		bool hipm = ata_id_has_hipm(dev->id);
+		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 &&
-		    (hipm || (ata_id_has_dipm(dev->id) && !no_dipm)))
+		if (!lpm_dev && (dev_has_hipm || (dev_has_dipm && !no_dipm)))
 			lpm_dev = dev;
 
 		hints &= ~ATA_LPM_EMPTY;
-		if (!hipm)
+		if (!dev_has_hipm)
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
 		if (policy < ATA_LPM_MED_POWER_WITH_DIPM &&
-		    (ata_id_has_dipm(dev->id) && !no_dipm)) {
+		    (dev_has_dipm && !no_dipm)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {

@@ -3520,8 +3520,10 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	 * 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 && !no_dipm &&
-		    ata_id_has_dipm(dev->id)) {
+		    dev_has_dipm) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_ENABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
Signature · 1 line
2.49.0
author maintainer
#7 lore raw

PATCH 5/7 ata: libata-eh: Rename no_dipm variable to be more clear

In reply to Niklas Cassel

Rename the no_dipm variable to host_has_dipm, by inverting the expression, and and also having a clearer name.

No functional change.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/ata/libata-eh.c +5 -4
index 272d074b78a1..bc3654265053 100644
@@ -3432,7 +3432,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	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 no_dipm = link->ap->flags & ATA_FLAG_NO_DIPM;
+	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;

@@ -3464,7 +3464,8 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		if (!link_dev)
 			link_dev = dev;
 
-		if (!lpm_dev && (dev_has_hipm || (dev_has_dipm && !no_dipm)))
+		if (!lpm_dev &&
+		    (dev_has_hipm || (dev_has_dipm && host_has_dipm)))
 			lpm_dev = dev;
 
 		hints &= ~ATA_LPM_EMPTY;

@@ -3473,7 +3474,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 
 		/* disable DIPM before changing link config */
 		if (policy < ATA_LPM_MED_POWER_WITH_DIPM &&
-		    (dev_has_dipm && !no_dipm)) {
+		    (dev_has_dipm && host_has_dipm)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {

@@ -3522,7 +3523,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	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 && !no_dipm &&
+		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);
Signature · 1 line
2.49.0
author maintainer
#8 lore raw

PATCH 6/7 ata: libata-eh: Host support has nothing to do with disabling DIPM

In reply to Niklas Cassel

It makes sense to take host support for DIPM into account when enabling DIPM, however, it makes zero sense to take host support into account when disabling DIPM.

If the host does not support DIPM, that is a very good reason to not keep the feature enabled on the device side.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/ata/libata-eh.c +1 -2
index bc3654265053..91d97d98eed1 100644
@@ -3473,8 +3473,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy < ATA_LPM_MED_POWER_WITH_DIPM &&
-		    (dev_has_dipm && host_has_dipm)) {
+		if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dev_has_dipm) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
Signature · 1 line
2.49.0
author maintainer
#9 lore raw

PATCH 7/7 ata: libata-eh: Always disable DIPM before calling set_lpm() callback

In reply to Niklas Cassel

The current code performs a SET FEATURES command to disable DIPM if policy < ATA_LPM_MED_POWER_WITH_DIPM, this means that it will disable DIPM for policies:
ATA_LPM_UNKNOWN, ATA_LPM_MAX_POWER, ATA_LPM_MED_POWER
(but not for policy ATA_LPM_MED_POWER_WITH_DIPM).

The code called after calling the set_lpm() callback will later perform a SET FEATURES command to enable DIPM, if
policy >= ATA_LPM_MED_POWER_WITH_DIPM

It seems very logical to disable DIPM before the set_lpm() callback is called, because the set_lpm() callback is the function (for AHCI) that sets the proper bits in PxSCTL.IPM, reflecting the support of the HBA. PxSCTL.IPM controls the LPM states that the device is allowed to enter.

If the device tries to enter a state disabled by PxSCTL.IPM, the host will NAK the transition.

It is thus imperative that PxSCTL.IPM is written before DIPM is enabled, to stop the device from entering a state that the host does not support.

While AHCI 1.3.1, section "8.3.1.1 Device Initiated" states:
"""
By default, a device that supports initiating interface power management states has the capability disabled.
"""

This makes it seem harmless to not disable DIPM before writing PxSCTL.IPM, however, SATA 3.5a section "13.3.4 Enable/disable Device-initiated interface power state transitions feature" states:
"""
If Software Settings Preservation is enabled and Device Initiated Interface Power Management Software Settings Preservation is supported (see 13.7.11.2.26), then the enable/disable state shall persist across a COMRESET.
"""

Further SATA 3.5a, section "13.3.7 Enable/disable Software Settings Preservation feature" states that:
"""
By default, if the device supports software settings preservation, the feature is enabled on power-up.
"""

Which means that it is possible that DIPM is enabled at probe time, even after ata_eh_reset() has performed a COMRESET, thus, make sure that DIPM is always disabled before calling the set_lpm() callback (which will write PxSCTL.IPM).

For a LPM policy that includes DIPM, ata_eh_set_lpm() will still enable DIPM using a SET FEATURES command after the set_lpm() callback has been called (assuming that both the device and host supports DIPM).

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-eh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ata/libata-eh.c +1 -1
index 91d97d98eed1..1727248f135d 100644
@@ -3473,7 +3473,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dev_has_dipm) {
+		if (dev_has_dipm) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
Signature · 1 line
2.49.0
maintainer
#10 lore raw

Re: [PATCH 7/7] ata: libata-eh: Always disable DIPM before calling set_lpm() callback

Quoted 5 lines · Niklas Cassel

The current code performs a SET FEATURES command to disable DIPM if policy < ATA_LPM_MED_POWER_WITH_DIPM, this means that it will disable DIPM for policies:
ATA_LPM_UNKNOWN, ATA_LPM_MAX_POWER, ATA_LPM_MED_POWER
(but not for policy ATA_LPM_MED_POWER_WITH_DIPM).

[...]

drivers/ata/libata-eh.c static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, · line 3476

Quoted 10 lines
drivers/ata/libata-eh.c +1 -1
index 91d97d98eed1..1727248f135d 100644
@@ -3473,7 +3473,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dev_has_dipm) {
+		if (dev_has_dipm) {

This changes the same line that patch 6 changed... Can you squash these patches together ?

Quoted 3 lines
drivers/ata/libata-eh.c +0 -0
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer
#11 lore raw

Re: [PATCH 7/7] ata: libata-eh: Always disable DIPM before calling set_lpm() callback

Quoted 16 lines · Damien Le Moal
Quoted 5 lines · Niklas Cassel

The current code performs a SET FEATURES command to disable DIPM if policy < ATA_LPM_MED_POWER_WITH_DIPM, this means that it will disable DIPM for policies:
ATA_LPM_UNKNOWN, ATA_LPM_MAX_POWER, ATA_LPM_MED_POWER
(but not for policy ATA_LPM_MED_POWER_WITH_DIPM).

[...]

Quoted 7 lines
drivers/ata/libata-eh.c +0 -0
index 91d97d98eed1..1727248f135d 100644
@@ -3473,7 +3473,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
Quoted 6 lines

drivers/ata/libata-eh.c static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, · line 3476

Quoted 3 lines
drivers/ata/libata-eh.c +1 -1
 		/* disable DIPM before changing link config */
-		if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dev_has_dipm) {
+		if (dev_has_dipm) {

This changes the same line that patch 6 changed... Can you squash these patches together ?

It is two separate logical changes, so squashing them seems wrong IMO.

Kind regards,
Niklas

maintainer
#12 lore raw

Re: [PATCH 7/7] ata: libata-eh: Always disable DIPM before calling set_lpm() callback

Quoted 17 lines · Niklas Cassel
Quoted 16 lines · Damien Le Moal
Quoted 5 lines · Niklas Cassel

The current code performs a SET FEATURES command to disable DIPM if policy < ATA_LPM_MED_POWER_WITH_DIPM, this means that it will disable DIPM for policies:
ATA_LPM_UNKNOWN, ATA_LPM_MAX_POWER, ATA_LPM_MED_POWER
(but not for policy ATA_LPM_MED_POWER_WITH_DIPM).

[...]

Quoted 7 lines
drivers/ata/libata-eh.c +0 -0
index 91d97d98eed1..1727248f135d 100644
@@ -3473,7 +3473,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
Quoted 8 lines
Quoted 6 lines

drivers/ata/libata-eh.c static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy, · line 3476

Quoted 3 lines
drivers/ata/libata-eh.c +1 -1
 		/* disable DIPM before changing link config */
-		if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dev_has_dipm) {
+		if (dev_has_dipm) {

This changes the same line that patch 6 changed... Can you squash these patches together ?

It is two separate logical changes, so squashing them seems wrong IMO.

Hmm... Given that the same line is changed, I would prefer a single patch. But OK.

Signature · 2 lines
Damien Le Moal
Western Digital Research