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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

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