[PATCH v2 0/6] misc LPM related fixes

[PATCH v2 0/6] misc LPM related fixes

[Niklas Cassel]

Hello there,

we had a recent bug report on the mailing list related to LPM, which made
me review the LPM related code.

While doing so, I found some issues. This series fixes those issues.

The most serious issue is that ATA_QUIRK_NOLPM was not getting applied for
ATAPI devices and that we read the per port area for unimplemented ports,
even though the AHCI specification explicitly forbids this.


Kind regards,
Niklas


Changes since v1:
-Fixed typos in commit messages
-Split early return patch into two, so that they only have a single
 Fixes-tag each.
-Reordered patches to make it easier to only apply the important ones
 for the current v6.19 kernel.


Niklas Cassel (6):
  ata: ahci: Do not read the per port area for unimplemented ports
  ata: libata: Call ata_dev_config_lpm() for ATAPI devices
  ata: libata-sata: Improve link_power_management_supported sysfs
    attribute
  ata: libata: Add cpr_log to ata_dev_print_features() early return
  ata: libata: Add DIPM and HIPM to ata_dev_print_features() early
    return
  ata: libata: Print features also for ATAPI devices

 drivers/ata/ahci.c        | 10 +++++-----
 drivers/ata/libata-core.c |  8 +++++++-
 drivers/ata/libata-sata.c |  2 +-
 3 files changed, 13 insertions(+), 7 deletions(-)


base-commit: 97e01439e902b743b8f89497e9c144e3ddda5e59
-- 
2.52.0

[PATCH v2 1/6] ata: ahci: Do not read the per port area for unimplemented ports

[Niklas Cassel]

An AHCI HBA specifies the number of ports it supports using CAP.NP.
The HBA is free to only make a subset of the number of ports available
using the PI (Ports Implemented) register.

libata currently creates dummy ports for HBA ports that are provided by
the HBA, but which are marked as "unavailable" using the PI register.

Each port will have a per port area of registers in the HBA, regardless
if the port is marked as "unavailable" or not.

ahci_mark_external_port() currently reads this per port area of registers
using readl() to see if the port is marked as external/hotplug-capable.

However, AHCI 1.3.1, section "3.1.4 Offset 0Ch: PI – Ports Implemented"
states: "Software must not read or write to registers within unavailable
ports."

Thus, make sure that we only call ahci_mark_external_port() and
ahci_update_initial_lpm_policy() for ports that are implemented.

From a libata perspective, this should not change anything related to LPM,
as dummy ports do not provide any ap->ops (they do not have a .set_lpm()
callback), so even if EH were to call .set_lpm() on a dummy port, it was
already a no-op.

Fixes: f7131935238d ("ata: ahci: move marking of external port earlier")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 7a7f88b3fa2b..931d0081169b 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2094,13 +2094,13 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		if (ap->flags & ATA_FLAG_EM)
 			ap->em_message_type = hpriv->em_msg_type;
 
-		ahci_mark_external_port(ap);
-
-		ahci_update_initial_lpm_policy(ap);
-
 		/* disabled/not-implemented port */
-		if (!(hpriv->port_map & (1 << i)))
+		if (!(hpriv->port_map & (1 << i))) {
 			ap->ops = &ata_dummy_port_ops;
+		} else {
+			ahci_mark_external_port(ap);
+			ahci_update_initial_lpm_policy(ap);
+		}
 	}
 
 	/* apply workaround for ASUS P5W DH Deluxe mainboard */
-- 
2.52.0

[PATCH v2 2/6] ata: libata: Call ata_dev_config_lpm() for ATAPI devices

[Niklas Cassel]

Commit d360121832d8 ("ata: libata-core: Introduce ata_dev_config_lpm()")
introduced ata_dev_config_lpm(). However, it only called this function for
ATA_DEV_ATA and ATA_DEV_ZAC devices, not for ATA_DEV_ATAPI devices.

Additionally, commit d99a9142e782 ("ata: libata-core: Move device LPM quirk
settings to ata_dev_config_lpm()") moved the LPM quirk application from
ata_dev_configure() to ata_dev_config_lpm(), causing LPM quirks for ATAPI
devices to no longer be applied.

Call ata_dev_config_lpm() also for ATAPI devices, such that LPM quirks are
applied for ATAPI devices with an entry in __ata_dev_quirks once again.

Fixes: d360121832d8 ("ata: libata-core: Introduce ata_dev_config_lpm()")
Fixes: d99a9142e782 ("ata: libata-core: Move device LPM quirk settings to ata_dev_config_lpm()")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b96105481784..1e8e35c10b35 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3148,6 +3148,8 @@ int ata_dev_configure(struct ata_device *dev)
 				     ata_mode_string(xfer_mask),
 				     cdb_intr_string, atapi_an_string,
 				     dma_dir_string);
+
+		ata_dev_config_lpm(dev);
 	}
 
 	/* determine max_sectors */
-- 
2.52.0

[PATCH v2 3/6] ata: libata-sata: Improve link_power_management_supported sysfs attribute

[Niklas Cassel]

The link_power_management_supported sysfs attribute is currently set as
true even for ata ports that lack a .set_lpm() callback, e.g. dummy ports.

This is a bit silly, because while writing to the
link_power_management_policy sysfs attribute will make ata_scsi_lpm_store()
update ap->target_lpm_policy (thus sysfs will reflect the new value) and
call ata_port_schedule_eh() for the port, it is essentially a no-op.

This is because for a port without a .set_lpm() callback, once EH gets to
run, the ata_eh_link_set_lpm() will simply return, since the port does not
provide a .set_lpm() callback.

Thus, make sure that the link_power_management_supported sysfs attribute
is set to false for ports that lack a .set_lpm() callback. This way the
link_power_management_policy sysfs attribute will no longer be writable,
so we will no longer be misleading users to think that their sysfs write
actually does something.

Fixes: 0060beec0bfa ("ata: libata-sata: Add link_power_management_supported sysfs attribute")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-sata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index b2817a2995d6..04e1e774645e 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -909,7 +909,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
 	struct ata_link *link;
 	struct ata_device *dev;
 
-	if (ap->flags & ATA_FLAG_NO_LPM)
+	if ((ap->flags & ATA_FLAG_NO_LPM) || !ap->ops->set_lpm)
 		return false;
 
 	ata_for_each_link(link, ap, EDGE) {
-- 
2.52.0

[PATCH v2 4/6] ata: libata: Add cpr_log to ata_dev_print_features() early return

[Niklas Cassel]

ata_dev_print_features() is supposed to return early and not print anything
if there are no features supported.

However, commit fe22e1c2f705 ("libata: support concurrent positioning
ranges log") added another feature to ata_dev_print_features() without
updating the early return conditional.

Add the missing feature to the early return conditional.

Fixes: fe22e1c2f705 ("libata: support concurrent positioning ranges log")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1e8e35c10b35..d8397982144a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2904,7 +2904,7 @@ static void ata_dev_config_lpm(struct ata_device *dev)
 
 static void ata_dev_print_features(struct ata_device *dev)
 {
-	if (!(dev->flags & ATA_DFLAG_FEATURES_MASK))
+	if (!(dev->flags & ATA_DFLAG_FEATURES_MASK) && !dev->cpr_log)
 		return;
 
 	ata_dev_info(dev,
-- 
2.52.0

[PATCH v2 5/6] ata: libata: Add DIPM and HIPM to ata_dev_print_features() early return

[Niklas Cassel]

ata_dev_print_features() is supposed to return early and not print anything
if there are no features supported.

However, commit b1f5af54f1f5 ("ata: libata-core: Advertize device support
for DIPM and HIPM features") added additional features to
ata_dev_print_features() without updating the early return conditional.

Add the missing features to the early return conditional.

Fixes: b1f5af54f1f5 ("ata: libata-core: Advertize device support for DIPM and HIPM features")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d8397982144a..7656aea7663f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2904,7 +2904,8 @@ static void ata_dev_config_lpm(struct ata_device *dev)
 
 static void ata_dev_print_features(struct ata_device *dev)
 {
-	if (!(dev->flags & ATA_DFLAG_FEATURES_MASK) && !dev->cpr_log)
+	if (!(dev->flags & ATA_DFLAG_FEATURES_MASK) && !dev->cpr_log &&
+	    !ata_id_has_hipm(dev->id) && !ata_id_has_dipm(dev->id))
 		return;
 
 	ata_dev_info(dev,
-- 
2.52.0

[PATCH v2 6/6] ata: libata: Print features also for ATAPI devices

[Niklas Cassel]

Commit d633b8a702ab ("libata: print feature list on device scan")
added a print of the features supported by the device for ATA_DEV_ATA and
ATA_DEV_ZAC devices, but not for ATA_DEV_ATAPI devices.

Fix this by printing the features also for ATAPI devices.

Before changes:
ata1.00: ATAPI: Slimtype DVD A  DU8AESH, 6C2M, max UDMA/133

After changes:
ata1.00: ATAPI: Slimtype DVD A  DU8AESH, 6C2M, max UDMA/133
ata1.00: Features: Dev-Attention HIPM DIPM

Fixes: d633b8a702ab ("libata: print feature list on device scan")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 7656aea7663f..43072b1d9221 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3151,6 +3151,9 @@ int ata_dev_configure(struct ata_device *dev)
 				     dma_dir_string);
 
 		ata_dev_config_lpm(dev);
+
+		if (print_info)
+			ata_dev_print_features(dev);
 	}
 
 	/* determine max_sectors */
-- 
2.52.0

Re: [PATCH v2 3/6] ata: libata-sata: Improve link_power_management_supported sysfs attribute

[Damien Le Moal]

On 1/12/26 13:20, Niklas Cassel wrote:
> The link_power_management_supported sysfs attribute is currently set as
> true even for ata ports that lack a .set_lpm() callback, e.g. dummy ports.
> 
> This is a bit silly, because while writing to the
> link_power_management_policy sysfs attribute will make ata_scsi_lpm_store()
> update ap->target_lpm_policy (thus sysfs will reflect the new value) and
> call ata_port_schedule_eh() for the port, it is essentially a no-op.
> 
> This is because for a port without a .set_lpm() callback, once EH gets to
> run, the ata_eh_link_set_lpm() will simply return, since the port does not
> provide a .set_lpm() callback.
> 
> Thus, make sure that the link_power_management_supported sysfs attribute
> is set to false for ports that lack a .set_lpm() callback. This way the
> link_power_management_policy sysfs attribute will no longer be writable,
> so we will no longer be misleading users to think that their sysfs write
> actually does something.
> 
> Fixes: 0060beec0bfa ("ata: libata-sata: Add link_power_management_supported sysfs attribute")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-sata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index b2817a2995d6..04e1e774645e 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -909,7 +909,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
>  	struct ata_link *link;
>  	struct ata_device *dev;
>  
> -	if (ap->flags & ATA_FLAG_NO_LPM)
> +	if ((ap->flags & ATA_FLAG_NO_LPM) || !ap->ops->set_lpm)

Can't we set ATA_FLAG_NO_LPM for ports that do not have set_lpm implemented
earlier when scanning ? That would be safer.


>  		return false;
>  
>  	ata_for_each_link(link, ap, EDGE) {


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/6] ata: libata-sata: Improve link_power_management_supported sysfs attribute

[Niklas Cassel]

On Mon, Jan 12, 2026 at 02:17:14PM +0100, Damien Le Moal wrote:
> On 1/12/26 13:20, Niklas Cassel wrote:
> > The link_power_management_supported sysfs attribute is currently set as
> > true even for ata ports that lack a .set_lpm() callback, e.g. dummy ports.
> >
> > This is a bit silly, because while writing to the
> > link_power_management_policy sysfs attribute will make ata_scsi_lpm_store()
> > update ap->target_lpm_policy (thus sysfs will reflect the new value) and
> > call ata_port_schedule_eh() for the port, it is essentially a no-op.
> >
> > This is because for a port without a .set_lpm() callback, once EH gets to
> > run, the ata_eh_link_set_lpm() will simply return, since the port does not
> > provide a .set_lpm() callback.
> >
> > Thus, make sure that the link_power_management_supported sysfs attribute
> > is set to false for ports that lack a .set_lpm() callback. This way the
> > link_power_management_policy sysfs attribute will no longer be writable,
> > so we will no longer be misleading users to think that their sysfs write
> > actually does something.
> >
> > Fixes: 0060beec0bfa ("ata: libata-sata: Add link_power_management_supported sysfs attribute")
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  drivers/ata/libata-sata.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index b2817a2995d6..04e1e774645e 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -909,7 +909,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
> >	struct ata_link *link;
> >	struct ata_device *dev;
> >
> > -	if (ap->flags & ATA_FLAG_NO_LPM)
> > +	if ((ap->flags & ATA_FLAG_NO_LPM) || !ap->ops->set_lpm)
>
> Can't we set ATA_FLAG_NO_LPM for ports that do not have set_lpm implemented
> earlier when scanning ? That would be safer.

No, because ATA_FLAG_NO_LPM means force LPM policy max power:
https://github.com/torvalds/linux/blob/v6.19-rc5/drivers/ata/libata-core.c#L2851-L2855

So when port flag ATA_FLAG_NO_LPM is set, ata_eh_link_set_lpm()
will be called with policy ATA_LPM_MAX_POWER.

So in my opinion, setting ap->flags |= ATA_FLAG_NO_LPM
when there is no .set_lpm() would just add to the existing mess,
since ATA_FLAG_NO_LPM mean calls .set_lpm() with ATA_LPM_MAX_POWER.



ata_eh_link_set_lpm() on the other hand, looks like this:

	if (!IS_ENABLED(CONFIG_SATA_HOST) ||
	    (link->flags & ATA_LFLAG_NO_LPM) || (ap && !ap->ops->set_lpm))
		return 0;

So this patch simply took inspiration from that function.


ATA_LFLAG_NO_LPM seems to mean something like: we called .set_lpm() on
the port, but the device disappeared from the port when doing so,
so make futher calls to set_lpm() for this link a no-op...
(No idea why it doesn't instead call set_lpm() with ATA_LPM_MAX_POWER?)

Yes, it is a bit unfortunate that the link flag and the port flag have
very similar names, but mean completely different things.


I'm not sure if we should set sysfs attribute
link_power_management_supported == false if ATA_LFLAG_NO_LPM is set
(Currently we don't). Because if so, the sysfs supported attribute could
potentially change value during runtime, isn't it supposed to be static?

If we really want to, I guess we could do something like:

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 04e1e774645e..1134943f49ae 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -913,6 +913,8 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
 		return false;
 
 	ata_for_each_link(link, ap, EDGE) {
+		if (link->flags & ATA_LFLAG_NO_LPM)
+			return false;
 		ata_for_each_dev(dev, &ap->link, ENABLED) {
 			if (dev->quirks & ATA_QUIRK_NOLPM)
 				return false;


But if so, that should probably be a different patch.

This patch was mainly to stop lying to the user that dummy ports could
change/set lpm_policy.


For the record, not all libata drivers provide a .set_lpm() callback.

Right now, the only drivers providing it are:
ata_piix.c:     .set_lpm                = piix_sidpr_set_lpm,
libahci.c:      .set_lpm                = ahci_set_lpm,


Kind regards,
Niklas

Re: [PATCH v2 3/6] ata: libata-sata: Improve link_power_management_supported sysfs attribute

[Damien Le Moal]

On 1/12/26 15:39, Niklas Cassel wrote:
> I'm not sure if we should set sysfs attribute
> link_power_management_supported == false if ATA_LFLAG_NO_LPM is set
> (Currently we don't). Because if so, the sysfs supported attribute could
> potentially change value during runtime, isn't it supposed to be static?
> 
> If we really want to, I guess we could do something like:
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 04e1e774645e..1134943f49ae 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -913,6 +913,8 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
>  		return false;
>  
>  	ata_for_each_link(link, ap, EDGE) {
> +		if (link->flags & ATA_LFLAG_NO_LPM)
> +			return false;

Yeah, I think we need this. NO_LPM == max power, but cannot be changed. That's
the same as for external ports. So we should report not supported so that
userspace cannot change it.

>  		ata_for_each_dev(dev, &ap->link, ENABLED) {
>  			if (dev->quirks & ATA_QUIRK_NOLPM)
>  				return false;
> 
> 
> But if so, that should probably be a different patch.
> 
> This patch was mainly to stop lying to the user that dummy ports could
> change/set lpm_policy.
> 
> 
> For the record, not all libata drivers provide a .set_lpm() callback.
> 
> Right now, the only drivers providing it are:
> ata_piix.c:     .set_lpm                = piix_sidpr_set_lpm,
> libahci.c:      .set_lpm                = ahci_set_lpm,
> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/6] ata: libata-sata: Improve link_power_management_supported sysfs attribute

[Niklas Cassel]

On Mon, Jan 12, 2026 at 04:05:18PM +0100, Damien Le Moal wrote:
> On 1/12/26 15:39, Niklas Cassel wrote:
> > I'm not sure if we should set sysfs attribute
> > link_power_management_supported == false if ATA_LFLAG_NO_LPM is set
> > (Currently we don't). Because if so, the sysfs supported attribute could
> > potentially change value during runtime, isn't it supposed to be static?
> > 
> > If we really want to, I guess we could do something like:
> > 
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index 04e1e774645e..1134943f49ae 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -913,6 +913,8 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
> >  		return false;
> >  
> >  	ata_for_each_link(link, ap, EDGE) {
> > +		if (link->flags & ATA_LFLAG_NO_LPM)
> > +			return false;
> 
> Yeah, I think we need this. NO_LPM == max power, but cannot be changed. That's
> the same as for external ports. So we should report not supported so that
> userspace cannot change it.

ata_scsi_lpm_supported() already checks port flag ATA_FLAG_NO_LPM:

https://github.com/torvalds/linux/blob/v6.19-rc5/drivers/ata/libata-sata.c#L912-L913


Port flag ATA_FLAG_NO_LPM:
call .set_lpm() with ATA_LPM_MAX_POWER.

Link flag ATA_LFLAG_NO_LPM:
we called .set_lpm() on
the port, but the device disappeared from the port when doing so,
so make futher calls to set_lpm() for this link a no-op...
(No idea why it doesn't instead call set_lpm() with ATA_LPM_MAX_POWER?)



Kind regards,
Niklas

Re: [PATCH v2 0/6] misc LPM related fixes

[wolf]

On 2026-01-12 13:20, Niklas Cassel wrote:
> Hello there,
> 
> we had a recent bug report on the mailing list related to LPM, which 
> made
> me review the LPM related code.
> 
> While doing so, I found some issues. This series fixes those issues.
> 
> The most serious issue is that ATA_QUIRK_NOLPM was not getting applied 
> for
> ATAPI devices and that we read the per port area for unimplemented 
> ports,
> even though the AHCI specification explicitly forbids this.
> 
> 
> Kind regards,
> Niklas
> 
> 
> Changes since v1:
> -Fixed typos in commit messages
> -Split early return patch into two, so that they only have a single
>  Fixes-tag each.
> -Reordered patches to make it easier to only apply the important ones
>  for the current v6.19 kernel.
> 
> 
> Niklas Cassel (6):
>   ata: ahci: Do not read the per port area for unimplemented ports
>   ata: libata: Call ata_dev_config_lpm() for ATAPI devices
>   ata: libata-sata: Improve link_power_management_supported sysfs
>     attribute
>   ata: libata: Add cpr_log to ata_dev_print_features() early return
>   ata: libata: Add DIPM and HIPM to ata_dev_print_features() early
>     return
>   ata: libata: Print features also for ATAPI devices
> 
>  drivers/ata/ahci.c        | 10 +++++-----
>  drivers/ata/libata-core.c |  8 +++++++-
>  drivers/ata/libata-sata.c |  2 +-
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> 
> base-commit: 97e01439e902b743b8f89497e9c144e3ddda5e59

Tested-by: Wolf <wolf@yoxt.cc>

Re: [PATCH v2 0/6] misc LPM related fixes

[Damien Le Moal]

On 1/12/26 13:20, Niklas Cassel wrote:
> Hello there,
> 
> we had a recent bug report on the mailing list related to LPM, which made
> me review the LPM related code.
> 
> While doing so, I found some issues. This series fixes those issues.
> 
> The most serious issue is that ATA_QUIRK_NOLPM was not getting applied for
> ATAPI devices and that we read the per port area for unimplemented ports,
> even though the AHCI specification explicitly forbids this.

Applied to for-6.20-fixes. Thanks !


-- 
Damien Le Moal
Western Digital Research