[PATCH v2 0/5] ata_eh_set_lpm() cleanups

[PATCH v2 0/5] ata_eh_set_lpm() cleanups

[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:
-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(-)

-- 
2.49.0

[PATCH v2 1/5] ata: libata-eh: Update DIPM comments to reflect reality

[Niklas Cassel]

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 <cassel@kernel.org>
---
 drivers/ata/libata-eh.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b990c1ee0b12..f39756a26751 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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)) {
-- 
2.49.0

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

[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 <cassel@kernel.org>
---
 drivers/ata/libata-eh.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index f39756a26751..89b7b2139a16 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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
-- 
2.49.0

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

[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 <cassel@kernel.org>
---
 drivers/ata/libata-eh.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 89b7b2139a16..dcb449edd315 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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) {
-- 
2.49.0

[PATCH v2 4/5] ata: libata-eh: Rename no_dipm variable to be more clear

[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 <cassel@kernel.org>
---
 drivers/ata/libata-eh.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index dcb449edd315..0e36a7806cff 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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);
-- 
2.49.0

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

[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 <cassel@kernel.org>
---
 drivers/ata/libata-eh.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 0e36a7806cff..c11d8e634bf7 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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) {
-- 
2.49.0

Re: [PATCH v2 0/5] ata_eh_set_lpm() cleanups

[Mikko Juhani Korhonen]

to 15.5.2025 klo 16.56 Niklas Cassel (cassel@kernel.org) kirjoitti:
>
> 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(-)
>
> --
> 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

Re: [PATCH v2 0/5] ata_eh_set_lpm() cleanups

[Niklas Cassel]

On Tue, May 20, 2025 at 07:36:34AM +0300, Mikko Juhani Korhonen wrote:
> to 15.5.2025 klo 16.56 Niklas Cassel (cassel@kernel.org) kirjoitti:
> >
> > 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(-)
> >
> > --
> > 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.

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

Re: [PATCH v2 0/5] ata_eh_set_lpm() cleanups

[Mikko Juhani Korhonen]

ti 20.5.2025 klo 12.33 Niklas Cassel (cassel@kernel.org) kirjoitti:
>
> On Tue, May 20, 2025 at 07:36:34AM +0300, Mikko Juhani Korhonen wrote:
> > to 15.5.2025 klo 16.56 Niklas Cassel (cassel@kernel.org) kirjoitti:
> > >
> > > 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(-)
> > >
> > > --
> > > 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.
>
> 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.

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

Re: [PATCH v2 0/5] ata_eh_set_lpm() cleanups

[Damien Le Moal]

On 2025/05/15 15:56, Niklas Cassel wrote:
> 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(-)

Applied to for-6.16. Thanks !


-- 
Damien Le Moal
Western Digital Research