[PATCH 0/7] ata_eh_set_lpm() cleanups

[PATCH 0/7] ata_eh_set_lpm() cleanups

[Niklas Cassel]

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

-- 
2.49.0

[PATCH 1/7] 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.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-eh.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

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

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

[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 <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 edbc5d7572d1..d2ccdb9a2840 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(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 3/7] ata: libata-eh: Drop dipm variable

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

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

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

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

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

[PATCH 5/7] 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 272d074b78a1..bc3654265053 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;
@@ -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);
-- 
2.49.0

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

[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 <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 bc3654265053..91d97d98eed1 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -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) {
-- 
2.49.0

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

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

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

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

[Damien Le Moal]

On 5/14/25 19:22, Niklas Cassel wrote:
> 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 <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 edbc5d7572d1..d2ccdb9a2840 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(policy == ATA_LPM_UNKNOWN))

WARN_ON_ONCE() would be better here.

> +		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


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 5/14/25 19:22, Niklas Cassel wrote:
> 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 <cassel@kernel.org>

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


-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 5/14/25 19:22, Niklas Cassel wrote:
> 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).

[...]

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 91d97d98eed1..1727248f135d 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -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 ?

>  			err_mask = ata_dev_set_feature(dev,
>  					SETFEATURES_SATA_DISABLE, SATA_DIPM);
>  			if (err_mask && err_mask != AC_ERR_DEV) {


-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Thu, May 15, 2025 at 02:42:17PM +0200, Damien Le Moal wrote:
> On 5/14/25 19:22, Niklas Cassel wrote:
> > 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).
> 
> [...]
> 
> > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > index 91d97d98eed1..1727248f135d 100644
> > --- a/drivers/ata/libata-eh.c
> > +++ b/drivers/ata/libata-eh.c
> > @@ -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 ?

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


Kind regards,
Niklas

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

[Damien Le Moal]

On 5/15/25 14:59, Niklas Cassel wrote:
> On Thu, May 15, 2025 at 02:42:17PM +0200, Damien Le Moal wrote:
>> On 5/14/25 19:22, Niklas Cassel wrote:
>>> 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).
>>
>> [...]
>>
>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>> index 91d97d98eed1..1727248f135d 100644
>>> --- a/drivers/ata/libata-eh.c
>>> +++ b/drivers/ata/libata-eh.c
>>> @@ -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 ?
> 
> 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.

-- 
Damien Le Moal
Western Digital Research