Regression on linux-next (next-20250708)

Regression on linux-next (next-20250708)

[Borah, Chaitanya Kumar]

Hello Damien,

Hope you are doing well. I am Chaitanya from the linux graphics team in 
Intel.

This mail is regarding a regression we are seeing in our CI runs[1] on
linux-next repository.

Since the version next-20250708 [2], we are seeing the following regression

`````````````````````````````````````````````````````````````````````````````````
(kms_pm_rpm:5821) igt_pm-CRITICAL: Test assertion failure function 
__igt_pm_enable_sata_link_power_management, file ../lib/igt_pm.c:392:

(kms_pm_rpm:5821) igt_pm-CRITICAL: Failed assertion: write(fd, 
"min_power\n", strlen("min_power\n")) == strlen("min_power\n")

(kms_pm_rpm:5821) igt_pm-CRITICAL: Last errno: 95, Operation not supported

(kms_pm_rpm:5821) igt_pm-CRITICAL: error: -1 != 10

Test kms_pm_rpm failed.
`````````````````````````````````````````````````````````````````````````````````
Details log can be found in [3].

After bisecting the tree, the following patch [4] seems to be the first 
"bad" commit

`````````````````````````````````````````````````````````````````````````````````````````````````````````
commit 4edf1505b76d30e1e1e283d431e4f84ad01ddcef

Author: Damien Le Moal dlemoal@kernel.org

Date:   Tue Jul 1 21:53:18 2025 +0900


     ata: ahci: Disallow LPM policy control for external ports
`````````````````````````````````````````````````````````````````````````````````````````````````````````

For some context in our kms_pm_rpm tests, we enable min_power policy for 
SATA so that we can reach deep runtime power states and restore the 
original policy after finishing. [5][6]

IIUC, the above change is based on spec and not something which can be 
reverted. So as I see it, we have to drop this code path for external 
ports. However I am not sure if we can achieve deep power states without 
enforcing it through the sysfs entry.

Atleast for the basic-rte subtest, the test passes if we comment out the 
functions controlling the SATA ports. We will need more testing to 
determine if this approach work. Any thoughts on it?

Also, are there other ways to detect a port is external other than 
receiving EOPNOTSUPP on the sysfs write?


Thank you.

Regards

Chaitanya

[1] https://intel-gfx-ci.01.org/tree/linux-next/combined-alt.html?
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20250708
[3] 
https://gfx-ci.igk.intel.com/tree/linux-next/next-20250703/fi-ilk-650/igt@kms_pm_rpm@basic-pci-d3-state.html
[4] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20250708&id=4edf1505b76d30e1e1e283d431e4f84ad01ddcef
[5] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/intel/i915_pm_rpm.c?ref_type=heads
[6] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_pm.c?ref_type=heads#L457

Re: Regression on linux-next (next-20250708)

[Damien Le Moal]

On 7/25/25 15:43, Borah, Chaitanya Kumar wrote:
> Hello Damien,
> 
> Hope you are doing well. I am Chaitanya from the linux graphics team in 
> Intel.
> 
> This mail is regarding a regression we are seeing in our CI runs[1] on
> linux-next repository.
> 
> Since the version next-20250708 [2], we are seeing the following regression
> 
> `````````````````````````````````````````````````````````````````````````````````
> (kms_pm_rpm:5821) igt_pm-CRITICAL: Test assertion failure function 
> __igt_pm_enable_sata_link_power_management, file ../lib/igt_pm.c:392:
> 
> (kms_pm_rpm:5821) igt_pm-CRITICAL: Failed assertion: write(fd, 
> "min_power\n", strlen("min_power\n")) == strlen("min_power\n")
> 
> (kms_pm_rpm:5821) igt_pm-CRITICAL: Last errno: 95, Operation not supported
> 
> (kms_pm_rpm:5821) igt_pm-CRITICAL: error: -1 != 10
> 
> Test kms_pm_rpm failed.
> `````````````````````````````````````````````````````````````````````````````````
> Details log can be found in [3].
> 
> After bisecting the tree, the following patch [4] seems to be the first 
> "bad" commit
> 
> `````````````````````````````````````````````````````````````````````````````````````````````````````````
> commit 4edf1505b76d30e1e1e283d431e4f84ad01ddcef
> 
> Author: Damien Le Moal dlemoal@kernel.org
> 
> Date:   Tue Jul 1 21:53:18 2025 +0900
> 
> 
>      ata: ahci: Disallow LPM policy control for external ports
> `````````````````````````````````````````````````````````````````````````````````````````````````````````
> 
> For some context in our kms_pm_rpm tests, we enable min_power policy for 
> SATA so that we can reach deep runtime power states and restore the 
> original policy after finishing. [5][6]
> 
> IIUC, the above change is based on spec and not something which can be 
> reverted. So as I see it, we have to drop this code path for external 
> ports. However I am not sure if we can achieve deep power states without 
> enforcing it through the sysfs entry.

I am not entirely sure what you mean with the last sentence above, but for
external ports, LPM cannot be used if you want to keep the port hotplug
capability alive and working. Without keeping such port at max power state, we
cannot detect hotplug events (which is super annoying when you have e.g. a
server with front loading drive bays allowing swapping drives without shutting
the machine down).

> Atleast for the basic-rte subtest, the test passes if we comment out the 
> functions controlling the SATA ports. We will need more testing to 
> determine if this approach work. Any thoughts on it?

Niklas and I actually suspected that we would be getting "complaints" about this
change. Well... We did :)

The problem really is that external ports have never been properly handled by
libata so SATA hot-plugging never really worked reliably. Patches queued up for
6.17 before this patch prevent the kernel from changing the power state of
external port. And this patch was introduced after seeing systemd.udevd setting
external ports power state to min_power or lower states, thus breaking again the
hotplug capability.

The error you are seeing is thus entirely correct and expected.

The question is though: do we want the user to "ignore" hotplug capability and
instead priviledge low power states. I guess we should have such capability.

> Also, are there other ways to detect a port is external other than 
> receiving EOPNOTSUPP on the sysfs write?

There is not. But it would be easy to add a sysfs port attribute, e.g.
/sys/class/ata_port/ata1/external which says "0" for regular ports and "1" for
external ports. We could also make this attribute writable in the case of
external port so that doing:

echo 0 > /sys/class/ata_port/ata1/external

forces the kernel to ignore the external nature of the port and allow user
control of the port/device LPM state.

Would that work for your case ?

-- 
Damien Le Moal
Western Digital Research

Re: Regression on linux-next (next-20250708)

[Borah, Chaitanya Kumar]

On 7/25/2025 4:03 PM, Damien Le Moal wrote:
> On 7/25/25 15:43, Borah, Chaitanya Kumar wrote:
>> Hello Damien,
>>
>> Hope you are doing well. I am Chaitanya from the linux graphics team in
>> Intel.
>>
>> This mail is regarding a regression we are seeing in our CI runs[1] on
>> linux-next repository.
>>
>> Since the version next-20250708 [2], we are seeing the following regression
>>
>> `````````````````````````````````````````````````````````````````````````````````
>> (kms_pm_rpm:5821) igt_pm-CRITICAL: Test assertion failure function
>> __igt_pm_enable_sata_link_power_management, file ../lib/igt_pm.c:392:
>>
>> (kms_pm_rpm:5821) igt_pm-CRITICAL: Failed assertion: write(fd,
>> "min_power\n", strlen("min_power\n")) == strlen("min_power\n")
>>
>> (kms_pm_rpm:5821) igt_pm-CRITICAL: Last errno: 95, Operation not supported
>>
>> (kms_pm_rpm:5821) igt_pm-CRITICAL: error: -1 != 10
>>
>> Test kms_pm_rpm failed.
>> `````````````````````````````````````````````````````````````````````````````````
>> Details log can be found in [3].
>>
>> After bisecting the tree, the following patch [4] seems to be the first
>> "bad" commit
>>
>> `````````````````````````````````````````````````````````````````````````````````````````````````````````
>> commit 4edf1505b76d30e1e1e283d431e4f84ad01ddcef
>>
>> Author: Damien Le Moal dlemoal@kernel.org
>>
>> Date:   Tue Jul 1 21:53:18 2025 +0900
>>
>>
>>       ata: ahci: Disallow LPM policy control for external ports
>> `````````````````````````````````````````````````````````````````````````````````````````````````````````
>>
>> For some context in our kms_pm_rpm tests, we enable min_power policy for
>> SATA so that we can reach deep runtime power states and restore the
>> original policy after finishing. [5][6]
>>
>> IIUC, the above change is based on spec and not something which can be
>> reverted. So as I see it, we have to drop this code path for external
>> ports. However I am not sure if we can achieve deep power states without
>> enforcing it through the sysfs entry.
> 
> I am not entirely sure what you mean with the last sentence above, but for
> external ports, LPM cannot be used if you want to keep the port hotplug
> capability alive and working. Without keeping such port at max power state, we
> cannot detect hotplug events (which is super annoying when you have e.g. a
> server with front loading drive bays allowing swapping drives without shutting
> the machine down).
> 
>> Atleast for the basic-rte subtest, the test passes if we comment out the
>> functions controlling the SATA ports. We will need more testing to
>> determine if this approach work. Any thoughts on it?
> 
> Niklas and I actually suspected that we would be getting "complaints" about this
> change. Well... We did :)
> 
> The problem really is that external ports have never been properly handled by
> libata so SATA hot-plugging never really worked reliably. Patches queued up for
> 6.17 before this patch prevent the kernel from changing the power state of
> external port. And this patch was introduced after seeing systemd.udevd setting
> external ports power state to min_power or lower states, thus breaking again the
> hotplug capability.
> 
> The error you are seeing is thus entirely correct and expected.
> 
> The question is though: do we want the user to "ignore" hotplug capability and
> instead priviledge low power states. I guess we should have such capability.
> 

Atleast a case can be made for debugging and testing use-cases.

>> Also, are there other ways to detect a port is external other than
>> receiving EOPNOTSUPP on the sysfs write?
> 
> There is not. But it would be easy to add a sysfs port attribute, e.g.
> /sys/class/ata_port/ata1/external which says "0" for regular ports and "1" for
> external ports. We could also make this attribute writable in the case of
> external port so that doing:
> 
> echo 0 > /sys/class/ata_port/ata1/external
> 
> forces the kernel to ignore the external nature of the port and allow user
> control of the port/device LPM state.
> 
> Would that work for your case ?
> 

Something like this should solve our problem.

Regards

Chaitanya

Re: Regression on linux-next (next-20250708)

[Damien Le Moal]

On 7/29/25 01:37, Borah, Chaitanya Kumar wrote:
>> The question is though: do we want the user to "ignore" hotplug capability and
>> instead priviledge low power states. I guess we should have such capability.
>>
> 
> Atleast a case can be made for debugging and testing use-cases.
> 
>>> Also, are there other ways to detect a port is external other than
>>> receiving EOPNOTSUPP on the sysfs write?
>>
>> There is not. But it would be easy to add a sysfs port attribute, e.g.
>> /sys/class/ata_port/ata1/external which says "0" for regular ports and "1" for
>> external ports. We could also make this attribute writable in the case of
>> external port so that doing:
>>
>> echo 0 > /sys/class/ata_port/ata1/external
>>
>> forces the kernel to ignore the external nature of the port and allow user
>> control of the port/device LPM state.
>>
>> Would that work for your case ?
>>
> 
> Something like this should solve our problem.

I looked at this, but it is not a trivial change because of how we manage
features, which is that we do not really differentiate between "port/device
supports feature X" and "Disable X because of Y". So disabling something at
runtime instead of at device scan time (or revalidation) needs some code
massaging to remember the initial "port/device supports feature X".

One thing that would be easy to add is a "libata.force=ignore_external_ports"
module parameter to completely ignore the external nature of ports. That
probably will be the easiest solution for your case.

-- 
Damien Le Moal
Western Digital Research

Re: Regression on linux-next (next-20250708)

[Damien Le Moal]

[-- Attachment #1: Type: text/plain, Size: 3551 bytes --]

On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
> so that we can reach deep runtime power states and restore the original policy
> after finishing. [5][6]
> 
> IIUC, the above change is based on spec and not something which can be
> reverted. So as I see it, we have to drop this code path for external ports.
> However I am not sure if we can achieve deep power states without enforcing it
> through the sysfs entry.
> 
> Atleast for the basic-rte subtest, the test passes if we comment out the
> functions controlling the SATA ports. We will need more testing to determine if
> this approach work. Any thoughts on it?
> 
> Also, are there other ways to detect a port is external other than receiving
> EOPNOTSUPP on the sysfs write?

The attached patch adds the "link_power_management_supported" sysfs device
attribute for drives connected to AHCI. Would that work for you ?

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 229429ba5027..495fa096dd65 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -1089,6 +1089,7 @@ static struct ata_port_operations ich_pata_ops = {
 };

 static struct attribute *piix_sidpr_shost_attrs[] = {
+       &dev_attr_link_power_management_supported.attr,
        &dev_attr_link_power_management_policy.attr,
        NULL
 };
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index b335fb7e5cb4..c79abdfcd7a9 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -111,6 +111,7 @@ static DEVICE_ATTR(em_buffer, S_IWUSR | S_IRUGO,
 static DEVICE_ATTR(em_message_supported, S_IRUGO, ahci_show_em_supported, NULL);

 static struct attribute *ahci_shost_attrs[] = {
+       &dev_attr_link_power_management_supported.attr,
        &dev_attr_link_power_management_policy.attr,
        &dev_attr_em_message_type.attr,
        &dev_attr_em_message.attr,
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 0708686ca58a..82a1a72e47bf 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -900,6 +900,18 @@ static const char *ata_lpm_policy_names[] = {
        [ATA_LPM_MIN_POWER]             = "min_power",
 };

+static ssize_t ata_scsi_lpm_supported_show(struct device *dev,
+                                struct device_attribute *attr, char *buf)
+{
+       struct Scsi_Host *shost = class_to_shost(dev);
+       struct ata_port *ap = ata_shost_to_port(shost);
+
+       return sysfs_emit(buf, "%d\n", !(ap->flags & ATA_FLAG_NO_LPM));
+}
+DEVICE_ATTR(link_power_management_supported, S_IRUGO,
+           ata_scsi_lpm_supported_show, NULL);
+EXPORT_SYMBOL_GPL(dev_attr_link_power_management_supported);
+
 static ssize_t ata_scsi_lpm_store(struct device *device,
                                  struct device_attribute *attr,
                                  const char *buf, size_t count)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1c0580627dbb..e9a6f37bd7f9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -547,6 +547,7 @@ typedef void (*ata_postreset_fn_t)(struct ata_link *link,
unsigned int *classes)

 extern struct device_attribute dev_attr_unload_heads;
 #ifdef CONFIG_SATA_HOST
+extern struct device_attribute dev_attr_link_power_management_supported;
 extern struct device_attribute dev_attr_link_power_management_policy;
 extern struct device_attribute dev_attr_ncq_prio_supported;
 extern struct device_attribute dev_attr_ncq_prio_enable;


-- 
Damien Le Moal
Western Digital Research

[-- Attachment #2: 0001-ata-libata-sata-Add-link_power_management_supported-.patch --]
[-- Type: text/x-patch, Size: 3338 bytes --]

From e9937d86d37a6c1a9584ef232613d5a8a5a3b6a1 Mon Sep 17 00:00:00 2001
From: Damien Le Moal <dlemoal@kernel.org>
Date: Mon, 28 Jul 2025 13:04:29 +0900
Subject: [PATCH] ata: libata-sata: Add link_power_management_supported
 attribute

A port link power management policy can be controlled using the
link_power_management_policy sysfs device attribute. However, this
attribute exist also for device and ports that do not support LPM and in
such case, attempting to change the LPM policy will fail with
-EOPNOTSUPP.

Introduce the new sysfs link_power_management_supported device attribute
to indicate to the user if a port/device supports LPM and the attribute
link_power_management_policy can be used.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/ata_piix.c    |  1 +
 drivers/ata/libahci.c     |  1 +
 drivers/ata/libata-sata.c | 12 ++++++++++++
 include/linux/libata.h    |  1 +
 4 files changed, 15 insertions(+)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 229429ba5027..495fa096dd65 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -1089,6 +1089,7 @@ static struct ata_port_operations ich_pata_ops = {
 };
 
 static struct attribute *piix_sidpr_shost_attrs[] = {
+	&dev_attr_link_power_management_supported.attr,
 	&dev_attr_link_power_management_policy.attr,
 	NULL
 };
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index b335fb7e5cb4..c79abdfcd7a9 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -111,6 +111,7 @@ static DEVICE_ATTR(em_buffer, S_IWUSR | S_IRUGO,
 static DEVICE_ATTR(em_message_supported, S_IRUGO, ahci_show_em_supported, NULL);
 
 static struct attribute *ahci_shost_attrs[] = {
+	&dev_attr_link_power_management_supported.attr,
 	&dev_attr_link_power_management_policy.attr,
 	&dev_attr_em_message_type.attr,
 	&dev_attr_em_message.attr,
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 0708686ca58a..82a1a72e47bf 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -900,6 +900,18 @@ static const char *ata_lpm_policy_names[] = {
 	[ATA_LPM_MIN_POWER]		= "min_power",
 };
 
+static ssize_t ata_scsi_lpm_supported_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	struct ata_port *ap = ata_shost_to_port(shost);
+
+	return sysfs_emit(buf, "%d\n", !(ap->flags & ATA_FLAG_NO_LPM));
+}
+DEVICE_ATTR(link_power_management_supported, S_IRUGO,
+	    ata_scsi_lpm_supported_show, NULL);
+EXPORT_SYMBOL_GPL(dev_attr_link_power_management_supported);
+
 static ssize_t ata_scsi_lpm_store(struct device *device,
 				  struct device_attribute *attr,
 				  const char *buf, size_t count)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 1c0580627dbb..e9a6f37bd7f9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -547,6 +547,7 @@ typedef void (*ata_postreset_fn_t)(struct ata_link *link, unsigned int *classes)
 
 extern struct device_attribute dev_attr_unload_heads;
 #ifdef CONFIG_SATA_HOST
+extern struct device_attribute dev_attr_link_power_management_supported;
 extern struct device_attribute dev_attr_link_power_management_policy;
 extern struct device_attribute dev_attr_ncq_prio_supported;
 extern struct device_attribute dev_attr_ncq_prio_enable;
-- 
2.50.1

Re: Regression on linux-next (next-20250708)

[Borah, Chaitanya Kumar]

On 7/28/2025 9:41 AM, Damien Le Moal wrote:
> On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
>> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
>> so that we can reach deep runtime power states and restore the original policy
>> after finishing. [5][6]
>>
>> IIUC, the above change is based on spec and not something which can be
>> reverted. So as I see it, we have to drop this code path for external ports.
>> However I am not sure if we can achieve deep power states without enforcing it
>> through the sysfs entry.
>>
>> Atleast for the basic-rte subtest, the test passes if we comment out the
>> functions controlling the SATA ports. We will need more testing to determine if
>> this approach work. Any thoughts on it?
>>
>> Also, are there other ways to detect a port is external other than receiving
>> EOPNOTSUPP on the sysfs write?
> 
> The attached patch adds the "link_power_management_supported" sysfs device
> attribute for drives connected to AHCI. Would that work for you ?
> 

Yes this could work. I quickly hacked the test to ignore writing policy 
if this file returns 0.

Here is the state of the machine I am testing on.

/sys/class/scsi_host/host0/link_power_management_supported: 0
/sys/class/scsi_host/host1/link_power_management_supported: 0
/sys/class/scsi_host/host2/link_power_management_supported: 0
/sys/class/scsi_host/host3/link_power_management_supported: 0
/sys/class/scsi_host/host4/link_power_management_supported: 1
/sys/class/scsi_host/host5/link_power_management_supported: 1
/sys/class/scsi_host/host6/link_power_management_supported: 1
/sys/class/scsi_host/host7/link_power_management_supported: 1

Regards

Chaitanya

> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 229429ba5027..495fa096dd65 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -1089,6 +1089,7 @@ static struct ata_port_operations ich_pata_ops = {
>   };
> 
>   static struct attribute *piix_sidpr_shost_attrs[] = {
> +       &dev_attr_link_power_management_supported.attr,
>          &dev_attr_link_power_management_policy.attr,
>          NULL
>   };
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index b335fb7e5cb4..c79abdfcd7a9 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -111,6 +111,7 @@ static DEVICE_ATTR(em_buffer, S_IWUSR | S_IRUGO,
>   static DEVICE_ATTR(em_message_supported, S_IRUGO, ahci_show_em_supported, NULL);
> 
>   static struct attribute *ahci_shost_attrs[] = {
> +       &dev_attr_link_power_management_supported.attr,
>          &dev_attr_link_power_management_policy.attr,
>          &dev_attr_em_message_type.attr,
>          &dev_attr_em_message.attr,
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 0708686ca58a..82a1a72e47bf 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -900,6 +900,18 @@ static const char *ata_lpm_policy_names[] = {
>          [ATA_LPM_MIN_POWER]             = "min_power",
>   };
> 
> +static ssize_t ata_scsi_lpm_supported_show(struct device *dev,
> +                                struct device_attribute *attr, char *buf)
> +{
> +       struct Scsi_Host *shost = class_to_shost(dev);
> +       struct ata_port *ap = ata_shost_to_port(shost);
> +
> +       return sysfs_emit(buf, "%d\n", !(ap->flags & ATA_FLAG_NO_LPM));
> +}
> +DEVICE_ATTR(link_power_management_supported, S_IRUGO,
> +           ata_scsi_lpm_supported_show, NULL);
> +EXPORT_SYMBOL_GPL(dev_attr_link_power_management_supported);
> +
>   static ssize_t ata_scsi_lpm_store(struct device *device,
>                                    struct device_attribute *attr,
>                                    const char *buf, size_t count)
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 1c0580627dbb..e9a6f37bd7f9 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -547,6 +547,7 @@ typedef void (*ata_postreset_fn_t)(struct ata_link *link,
> unsigned int *classes)
> 
>   extern struct device_attribute dev_attr_unload_heads;
>   #ifdef CONFIG_SATA_HOST
> +extern struct device_attribute dev_attr_link_power_management_supported;
>   extern struct device_attribute dev_attr_link_power_management_policy;
>   extern struct device_attribute dev_attr_ncq_prio_supported;
>   extern struct device_attribute dev_attr_ncq_prio_enable;
> 
> 

Re: Regression on linux-next (next-20250708)

[Damien Le Moal]

On 7/29/25 01:24, Borah, Chaitanya Kumar wrote:
> 
> 
> On 7/28/2025 9:41 AM, Damien Le Moal wrote:
>> On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
>>> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
>>> so that we can reach deep runtime power states and restore the original policy
>>> after finishing. [5][6]
>>>
>>> IIUC, the above change is based on spec and not something which can be
>>> reverted. So as I see it, we have to drop this code path for external ports.
>>> However I am not sure if we can achieve deep power states without enforcing it
>>> through the sysfs entry.
>>>
>>> Atleast for the basic-rte subtest, the test passes if we comment out the
>>> functions controlling the SATA ports. We will need more testing to determine if
>>> this approach work. Any thoughts on it?
>>>
>>> Also, are there other ways to detect a port is external other than receiving
>>> EOPNOTSUPP on the sysfs write?
>>
>> The attached patch adds the "link_power_management_supported" sysfs device
>> attribute for drives connected to AHCI. Would that work for you ?
>>
> 
> Yes this could work. I quickly hacked the test to ignore writing policy 
> if this file returns 0.
> 
> Here is the state of the machine I am testing on.
> 
> /sys/class/scsi_host/host0/link_power_management_supported: 0
> /sys/class/scsi_host/host1/link_power_management_supported: 0
> /sys/class/scsi_host/host2/link_power_management_supported: 0
> /sys/class/scsi_host/host3/link_power_management_supported: 0
> /sys/class/scsi_host/host4/link_power_management_supported: 1
> /sys/class/scsi_host/host5/link_power_management_supported: 1
> /sys/class/scsi_host/host6/link_power_management_supported: 1
> /sys/class/scsi_host/host7/link_power_management_supported: 1

Looks good. My test machine looks exactly like this too.
I will send out this patch as it is useful anyway regardless of external/hotplug
ports since not all adapters/drives support LPM.

When posting this, can I tag it as a solution to the regression ?

And in addition to this patch, I will work on a flexible way of ignoring hotplug.


-- 
Damien Le Moal
Western Digital Research

Re: Regression on linux-next (next-20250708)

[Borah, Chaitanya Kumar]

On 7/29/2025 4:03 AM, Damien Le Moal wrote:
> On 7/29/25 01:24, Borah, Chaitanya Kumar wrote:
>>
>>
>> On 7/28/2025 9:41 AM, Damien Le Moal wrote:
>>> On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
>>>> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
>>>> so that we can reach deep runtime power states and restore the original policy
>>>> after finishing. [5][6]
>>>>
>>>> IIUC, the above change is based on spec and not something which can be
>>>> reverted. So as I see it, we have to drop this code path for external ports.
>>>> However I am not sure if we can achieve deep power states without enforcing it
>>>> through the sysfs entry.
>>>>
>>>> Atleast for the basic-rte subtest, the test passes if we comment out the
>>>> functions controlling the SATA ports. We will need more testing to determine if
>>>> this approach work. Any thoughts on it?
>>>>
>>>> Also, are there other ways to detect a port is external other than receiving
>>>> EOPNOTSUPP on the sysfs write?
>>>
>>> The attached patch adds the "link_power_management_supported" sysfs device
>>> attribute for drives connected to AHCI. Would that work for you ?
>>>
>>
>> Yes this could work. I quickly hacked the test to ignore writing policy
>> if this file returns 0.
>>
>> Here is the state of the machine I am testing on.
>>
>> /sys/class/scsi_host/host0/link_power_management_supported: 0
>> /sys/class/scsi_host/host1/link_power_management_supported: 0
>> /sys/class/scsi_host/host2/link_power_management_supported: 0
>> /sys/class/scsi_host/host3/link_power_management_supported: 0
>> /sys/class/scsi_host/host4/link_power_management_supported: 1
>> /sys/class/scsi_host/host5/link_power_management_supported: 1
>> /sys/class/scsi_host/host6/link_power_management_supported: 1
>> /sys/class/scsi_host/host7/link_power_management_supported: 1
> 
> Looks good. My test machine looks exactly like this too.
> I will send out this patch as it is useful anyway regardless of external/hotplug
> ports since not all adapters/drives support LPM.
> 
> When posting this, can I tag it as a solution to the regression ?
> 

Yes sure you can use it.

Adding Imre to the conversation if he has anything to say about this 
change in our test.

Regards

Chaitanya

> And in addition to this patch, I will work on a flexible way of ignoring hotplug.
> 
> 

Re: Regression on linux-next (next-20250708)

[Damien Le Moal]

On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
> so that we can reach deep runtime power states and restore the original policy
> after finishing. [5][6]
> 
> IIUC, the above change is based on spec and not something which can be
> reverted. So as I see it, we have to drop this code path for external ports.
> However I am not sure if we can achieve deep power states without enforcing it
> through the sysfs entry.
> 
> Atleast for the basic-rte subtest, the test passes if we comment out the
> functions controlling the SATA ports. We will need more testing to determine if
> this approach work. Any thoughts on it?
> 
> Also, are there other ways to detect a port is external other than receiving
> EOPNOTSUPP on the sysfs write?

I completely forgot to mention one important thing: please check your test
machine BIOS settings and see if you have "hotplug support" set to enable for
SATA ports. If it is, set that BIOS setting to disable and you will see the
SATA port as a regular one, not as an external port. So LPM support will be
back and your test program will not need changes.

Not all BIOSes have such setting though. Most of the machine I have do have it
though and I checked that it does affect how the ahci driver sees the port
(external or regular with LPM).


-- 
Damien Le Moal
Western Digital Research

Re: Regression on linux-next (next-20250708)

[Borah, Chaitanya Kumar]

On 7/28/2025 11:01 AM, Damien Le Moal wrote:
> On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
>> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
>> so that we can reach deep runtime power states and restore the original policy
>> after finishing. [5][6]
>>
>> IIUC, the above change is based on spec and not something which can be
>> reverted. So as I see it, we have to drop this code path for external ports.
>> However I am not sure if we can achieve deep power states without enforcing it
>> through the sysfs entry.
>>
>> Atleast for the basic-rte subtest, the test passes if we comment out the
>> functions controlling the SATA ports. We will need more testing to determine if
>> this approach work. Any thoughts on it?
>>
>> Also, are there other ways to detect a port is external other than receiving
>> EOPNOTSUPP on the sysfs write?
> 
> I completely forgot to mention one important thing: please check your test
> machine BIOS settings and see if you have "hotplug support" set to enable for
> SATA ports. If it is, set that BIOS setting to disable and you will see the
> SATA port as a regular one, not as an external port. So LPM support will be
> back and your test program will not need changes.
> 
> Not all BIOSes have such setting though. Most of the machine I have do have it
> though and I checked that it does affect how the ahci driver sees the port
> (external or regular with LPM).
> 
> 

Found a "Hot Plug" setting (thanks to Mika!) in our testing device's 
BIOS but it does not seem to have any effect.

We also have an option called "External", toggling that did not help either.

There is another configuration which was *readonly*.

"Configured as eSATA" -> "Hot Plug supported"

Not sure if it is relevant to our discussion.

Regards

Chaitanya

Re: Regression on linux-next (next-20250708)

[Damien Le Moal]

On 7/29/25 01:33, Borah, Chaitanya Kumar wrote:
> 
> 
> On 7/28/2025 11:01 AM, Damien Le Moal wrote:
>> On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
>>> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
>>> so that we can reach deep runtime power states and restore the original policy
>>> after finishing. [5][6]
>>>
>>> IIUC, the above change is based on spec and not something which can be
>>> reverted. So as I see it, we have to drop this code path for external ports.
>>> However I am not sure if we can achieve deep power states without enforcing it
>>> through the sysfs entry.
>>>
>>> Atleast for the basic-rte subtest, the test passes if we comment out the
>>> functions controlling the SATA ports. We will need more testing to determine if
>>> this approach work. Any thoughts on it?
>>>
>>> Also, are there other ways to detect a port is external other than receiving
>>> EOPNOTSUPP on the sysfs write?
>>
>> I completely forgot to mention one important thing: please check your test
>> machine BIOS settings and see if you have "hotplug support" set to enable for
>> SATA ports. If it is, set that BIOS setting to disable and you will see the
>> SATA port as a regular one, not as an external port. So LPM support will be
>> back and your test program will not need changes.
>>
>> Not all BIOSes have such setting though. Most of the machine I have do have it
>> though and I checked that it does affect how the ahci driver sees the port
>> (external or regular with LPM).
>>
>>
> 
> Found a "Hot Plug" setting (thanks to Mika!) in our testing device's 
> BIOS but it does not seem to have any effect.
> 
> We also have an option called "External", toggling that did not help either.
> 
> There is another configuration which was *readonly*.
> 
> "Configured as eSATA" -> "Hot Plug supported"
> 
> Not sure if it is relevant to our discussion.

It is and that probably is the reason why disabling hotplug does nothing on the
port external characteristic. Does this machine really have eSata ports ? Do
they correspond to the 4 ports (out of 8) that you see as external
(link_power_management_supported = 0 ports) ?

Likely, you have the SXS host capability set for this machine because of this
BIOS setup. From the AHCI specifications:

Supports External SATA (SXS): When set to ‘1’, indicates that the HBA has one or
more Serial ATA ports that has a signal only connector that is externally
accessible (e.g. eSATA connector).

Hotplug is reported as a separate bit, but handled in the same way as an
external port as we cannot (easily) support LPM if we want to preserve the
hotplug capability (LPM changes the PHY state constantly, which clashes with hot
plug/unplug PHY changes and is hard to differentiate).

Note that you can see if a port is external in dmesg. Look for:

ata4: SATA max UDMA/133 abar m524288@0xaa500000 port 0xaa500280 irq 112 lpm-pol
1 ext

A regular port will not have the "ext" at the end:

ata5: SATA max UDMA/133 abar m524288@0xaa500000 port 0xaa500300 irq 112 lpm-pol 1



-- 
Damien Le Moal
Western Digital Research

Re: Regression on linux-next (next-20250708)

[Borah, Chaitanya Kumar]

On 7/29/2025 4:00 AM, Damien Le Moal wrote:
> On 7/29/25 01:33, Borah, Chaitanya Kumar wrote:
>>
>>
>> On 7/28/2025 11:01 AM, Damien Le Moal wrote:
>>> On 7/25/25 3:43 PM, Borah, Chaitanya Kumar wrote:
>>>> For some context in our kms_pm_rpm tests, we enable min_power policy for SATA
>>>> so that we can reach deep runtime power states and restore the original policy
>>>> after finishing. [5][6]
>>>>
>>>> IIUC, the above change is based on spec and not something which can be
>>>> reverted. So as I see it, we have to drop this code path for external ports.
>>>> However I am not sure if we can achieve deep power states without enforcing it
>>>> through the sysfs entry.
>>>>
>>>> Atleast for the basic-rte subtest, the test passes if we comment out the
>>>> functions controlling the SATA ports. We will need more testing to determine if
>>>> this approach work. Any thoughts on it?
>>>>
>>>> Also, are there other ways to detect a port is external other than receiving
>>>> EOPNOTSUPP on the sysfs write?
>>>
>>> I completely forgot to mention one important thing: please check your test
>>> machine BIOS settings and see if you have "hotplug support" set to enable for
>>> SATA ports. If it is, set that BIOS setting to disable and you will see the
>>> SATA port as a regular one, not as an external port. So LPM support will be
>>> back and your test program will not need changes.
>>>
>>> Not all BIOSes have such setting though. Most of the machine I have do have it
>>> though and I checked that it does affect how the ahci driver sees the port
>>> (external or regular with LPM).
>>>
>>>
>>
>> Found a "Hot Plug" setting (thanks to Mika!) in our testing device's
>> BIOS but it does not seem to have any effect.
>>
>> We also have an option called "External", toggling that did not help either.
>>
>> There is another configuration which was *readonly*.
>>
>> "Configured as eSATA" -> "Hot Plug supported"
>>
>> Not sure if it is relevant to our discussion.
> 
> It is and that probably is the reason why disabling hotplug does nothing on the
> port external characteristic. Does this machine really have eSata ports ? Do
> they correspond to the 4 ports (out of 8) that you see as external
> (link_power_management_supported = 0 ports) ?
> 

I can see the BIOS setting available for all 8 ports.

> Likely, you have the SXS host capability set for this machine because of this
> BIOS setup. From the AHCI specifications:
> 
> Supports External SATA (SXS): When set to ‘1’, indicates that the HBA has one or
> more Serial ATA ports that has a signal only connector that is externally
> accessible (e.g. eSATA connector).
> 
> Hotplug is reported as a separate bit, but handled in the same way as an
> external port as we cannot (easily) support LPM if we want to preserve the
> hotplug capability (LPM changes the PHY state constantly, which clashes with hot
> plug/unplug PHY changes and is hard to differentiate).
> 
> Note that you can see if a port is external in dmesg. Look for:
> 
> ata4: SATA max UDMA/133 abar m524288@0xaa500000 port 0xaa500280 irq 112 lpm-pol
> 1 ext
> 
> A regular port will not have the "ext" at the end:
> 
> ata5: SATA max UDMA/133 abar m524288@0xaa500000 port 0xaa500300 irq 112 lpm-pol 1

Thanks for the hint. I see the following logs with our default BIOS 
configuration.

[    6.125670] ata1: DUMMY
[    6.125673] ata2: DUMMY
[    6.125676] ata3: DUMMY
[    6.125678] ata4: DUMMY
[    6.125683] ata5: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263300 irq 128 lpm-pol 4
[    6.125693] ata6: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263380 irq 128 lpm-pol 4
[    6.125702] ata7: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263400 irq 128 lpm-pol 4
[    6.125711] ata8: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263480 irq 128 lpm-pol 4

Enabling the "External" option in BIOS has impact on the "non-DUMMY" 
ports but has no affect on the DUMMY ones.

After enabling the External option on port 1 and 7, the dmesg becomes.

[    6.366145] ata1: DUMMY
[    6.366148] ata2: DUMMY
[    6.366151] ata3: DUMMY
[    6.366153] ata4: DUMMY
[    6.366160] ata5: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263300 irq 128 lpm-pol 4
[    6.366168] ata6: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263380 irq 128 lpm-pol 4
[    6.366174] ata7: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263400 irq 128 lpm-pol 1 ext
[    6.366182] ata8: SATA max UDMA/133 abar m2048@0xb8263000 port 
0xb8263480 irq 128 lpm-pol 4

Also enabling "External" hides the "Configured as eSATA" option.

-Chaitanya