ata_eh_set_lpm() cleanups

PATCH Applied Niklas Cassel 3 participants 10 messages May 15, 2025 – May 23, 2025 libata

lore

Share & tools

lore

Niklas Cassel

author maintainer

2025-05-15 13:56 UTC #0 lore raw

v2 cover ata_eh_set_lpm() cleanups

Hello all,

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.

Changes since v1:
-Squashed patches 3 and 4 as requested by Damien. -Squashed patches 6 and 7 as requested by Damien.
-Changed WARN_ON to WARN_ON_ONCE.
-Changed patch 1 to remove parts of a comment that is no longer true.

Niklas Cassel (5):
ata: libata-eh: Update DIPM comments to reflect reality
ata: libata-eh: Add ata_eh_set_lpm() WARN_ON_ONCE
ata: libata-eh: Rename hipm and dipm variables
ata: libata-eh: Rename no_dipm variable to be more clear
ata: libata-eh: Keep DIPM disabled while modifying the allowed LPM
states

 drivers/ata/libata-eh.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

Signature · 1 line

2.49.0

Niklas Cassel

author maintainer

2025-05-15 13:56 UTC #1 lore raw

PATCH v2 1/5 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.

Also remove the sentence that claims that "Order device and link configurations such that the host always allows DIPM requests."

This comment is written before 24e0e61db3cb ("ata: libata: disallow dev-initiated LPM transitions to unsupported states").

Even though the set_lpm() call is done before enabling DIPM, the host will not always allow DIPM requests. For all LPM polcies where DIPM is enabled, only DIPM requests to LPM states that are supported by the HBA will be allowed.

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

---
 drivers/ata/libata-eh.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/ata/libata-eh.c +8 -5

index b990c1ee0b12..f39756a26751 100644
@@ -3443,10 +3443,9 @@ 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.
 	 */
 	ata_for_each_dev(dev, link, ENABLED) {
 		bool hipm = ata_id_has_hipm(dev->id);

@@ -3505,7 +3504,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

Niklas Cassel

author maintainer

2025-05-15 13:56 UTC #2 lore raw

PATCH v2 2/5 ata: libata-eh: Add ata_eh_set_lpm() WARN_ON_ONCE

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_ONCE 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 f39756a26751..89b7b2139a16 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_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

Signature · 1 line

2.49.0

Niklas Cassel

author maintainer

2025-05-15 13:56 UTC #3 lore raw

PATCH v2 3/5 ata: libata-eh: Rename hipm and dipm variables

In reply to Niklas Cassel

Rename the hipm and dipm variables to have a clearer name. Also fold in the usage of no_dipm, as that is required in order to give the dipm variable a more descriptive name.

No functional change.

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

---
 drivers/ata/libata-eh.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

drivers/ata/libata-eh.c +9 -6

index 89b7b2139a16..dcb449edd315 100644
@@ -3455,22 +3455,23 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	 * some devices misbehave when the host NACKs transition to SLUMBER.
 	 */
 	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;
+		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 || 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 && dipm) {
+		if (policy < ATA_LPM_MED_POWER_WITH_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) {

@@ -3517,8 +3518,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

Niklas Cassel

author maintainer

2025-05-15 13:56 UTC #4 lore raw

PATCH v2 4/5 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 dcb449edd315..0e36a7806cff 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;

@@ -3462,7 +3462,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;

@@ -3471,7 +3472,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) {

@@ -3520,7 +3521,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

Niklas Cassel

author maintainer

2025-05-15 13:56 UTC #5 lore raw

PATCH v2 5/5 ata: libata-eh: Keep DIPM disabled while modifying the allowed LPM states

In reply to Niklas Cassel

Currently, it is possible that LPM is enabled while calling the set_lpm() callback.

The current code performs a SET FEATURES command to disable DIPM if policy < ATA_LPM_MED_POWER_WITH_DIPM, this means that it will currently 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.

As we can see DIPM will not be disabled before calling set_lpm() if the LPM policy is: ATA_LPM_MED_POWER_WITH_DIPM, ATA_LPM_MIN_POWER_WITH_PARTIAL, or ATA_LPM_MIN_POWER.

Make sure that we always disable DIPM before calling the set_lpm() callback. This is 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.

If we do not disable DIPM before modifying PxSCTL.IPM, it is possible that DIPM will try (and will be allowed to) enter a LPM state that the HBA does not support (since we have not yet written PxSCTL.IPM, the HBA wasn't able to NAK the transition).

While at it, remove the guard of host support for DIPM around the disabling of DIPM. While it makes sense to take host support for DIPM into account when enabling DIPM, it makes zero sense to take host support into account when disabling DIPM.

If the host does not support DIPM, that is an even bigger reason why DIPM should be disabled 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 0e36a7806cff..c11d8e634bf7 100644
@@ -3471,8 +3471,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 (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

Mikko Juhani Korhonen

2025-05-20 04:37 UTC #6 lore raw

In reply to Niklas Cassel

to 15.5.2025 klo 16.56 Niklas Cassel ([email protected]) kirjoitti:

Quoted 24 lines

Hello all,
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.
Changes since v1:
(quoted diff) +0 -4
-Squashed patches 3 and 4 as requested by Damien.
-Squashed patches 6 and 7 as requested by Damien.
-Changed WARN_ON to WARN_ON_ONCE.
-Changed patch 1 to remove parts of a comment that is no longer true.
Niklas Cassel (5):
ata: libata-eh: Update DIPM comments to reflect reality
ata: libata-eh: Add ata_eh_set_lpm() WARN_ON_ONCE
ata: libata-eh: Rename hipm and dipm variables
ata: libata-eh: Rename no_dipm variable to be more clear
ata: libata-eh: Keep DIPM disabled while modifying the allowed LPM
states

Quoted 6 lines

 drivers/ata/libata-eh.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

--
2.49.0

Hello Niklas!

just tried this patch set for the non working case of my sata ports 5,6 -> WDC WD20EFAX-68FB5N0
but as was kind of expected there was not any change.

Best regards,
Mikko

Niklas Cassel

author maintainer

2025-05-20 09:33 UTC #7 lore raw

Quoted 34 lines · Mikko Juhani Korhonen

to 15.5.2025 klo 16.56 Niklas Cassel ([email protected]) kirjoitti:
Quoted 24 lines
Hello all,
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.
Changes since v1:
(quoted diff) +0 -4
-Squashed patches 3 and 4 as requested by Damien.
-Squashed patches 6 and 7 as requested by Damien.
-Changed WARN_ON to WARN_ON_ONCE.
-Changed patch 1 to remove parts of a comment that is no longer true.
Niklas Cassel (5):
ata: libata-eh: Update DIPM comments to reflect reality
ata: libata-eh: Add ata_eh_set_lpm() WARN_ON_ONCE
ata: libata-eh: Rename hipm and dipm variables
ata: libata-eh: Rename no_dipm variable to be more clear
ata: libata-eh: Keep DIPM disabled while modifying the allowed LPM
states
Quoted 6 lines
 drivers/ata/libata-eh.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)
--
2.49.0
Hello Niklas!

Quoted 3 lines

just tried this patch set for the non working case of my sata ports 5,6 -> WDC WD20EFAX-68FB5N0
but as was kind of expected there was not any change.

Hello Mikko,

Thank you for testing!

Yes, it was expected to not make any difference for your problem.

I'm not sure if you are able to workaround your problem by simply using ports other than ports 5,6.

If you feel that the problem has to be addressed, then I think that we would need to introduce a quirk for your motherboard name that disables LPM for ports 5,6 only.

Kind regards,
Niklas

Mikko Juhani Korhonen

2025-05-23 14:47 UTC #8 lore raw

ti 20.5.2025 klo 12.33 Niklas Cassel ([email protected]) kirjoitti:

Quoted 46 lines · Mikko Juhani Korhonen

Quoted 34 lines
to 15.5.2025 klo 16.56 Niklas Cassel ([email protected]) kirjoitti:
Quoted 24 lines
Hello all,
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.
Changes since v1:
(quoted diff) +0 -4
-Squashed patches 3 and 4 as requested by Damien.
-Squashed patches 6 and 7 as requested by Damien.
-Changed WARN_ON to WARN_ON_ONCE.
-Changed patch 1 to remove parts of a comment that is no longer true.
Niklas Cassel (5):
ata: libata-eh: Update DIPM comments to reflect reality
ata: libata-eh: Add ata_eh_set_lpm() WARN_ON_ONCE
ata: libata-eh: Rename hipm and dipm variables
ata: libata-eh: Rename no_dipm variable to be more clear
ata: libata-eh: Keep DIPM disabled while modifying the allowed LPM
states
Quoted 6 lines
 drivers/ata/libata-eh.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)
--
2.49.0
Hello Niklas!
Quoted 3 lines
just tried this patch set for the non working case of my sata ports 5,6 -> WDC WD20EFAX-68FB5N0
but as was kind of expected there was not any change.
Hello Mikko,
Thank you for testing!

Quoted 8 lines

Yes, it was expected to not make any difference for your problem.
I'm not sure if you are able to workaround your problem by simply using ports other than ports 5,6.
If you feel that the problem has to be addressed, then I think that we would need to introduce a quirk for your motherboard name that disables LPM for ports 5,6 only.

Well yes even if we don't have other bug reports?, to me it seems quite unlikely that this motherboard would work for others with even with other dipm capable drives on ports 5 and 6, so I'm for the quirk.

best regards,
Mikko

Damien Le Moal

maintainer Applied

2025-05-20 09:37 UTC #9 lore raw

In reply to Niklas Cassel

Quoted 23 lines · Niklas Cassel

Hello all,
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.
Changes since v1:
(quoted diff) +0 -4
-Squashed patches 3 and 4 as requested by Damien.
-Squashed patches 6 and 7 as requested by Damien.
-Changed WARN_ON to WARN_ON_ONCE.
-Changed patch 1 to remove parts of a comment that is no longer true.
Niklas Cassel (5):
ata: libata-eh: Update DIPM comments to reflect reality
ata: libata-eh: Add ata_eh_set_lpm() WARN_ON_ONCE
ata: libata-eh: Rename hipm and dipm variables
ata: libata-eh: Rename no_dipm variable to be more clear
ata: libata-eh: Keep DIPM disabled while modifying the allowed LPM
states

Quoted 2 lines

 drivers/ata/libata-eh.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

Applied to for-6.16. Thanks !

Signature · 2 lines

Damien Le Moal
Western Digital Research