Re: [PATCH RFC v3 3/3] PCI/ASPM: add sysfs attribute for controlling ASPM

[Heiner Kallweit <hkallweit1@gmail.com>]

On 17.05.2019 02:12, Frederick Lawler wrote:
> Heiner,
> 
> Heiner Kallweit wrote on 5/12/19 8:54 AM:> [snip]
>> +static ssize_t aspm_link_states_show(struct device *dev,
>> +                     struct device_attribute *attr, char *buf)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct pcie_link_state *link;
>> +    int len = 0, i;
>> +
>> +    link = aspm_get_parent_link(pdev);
>> +    if (!link)
>> +        return -EOPNOTSUPP;
>> +
>> +    mutex_lock(&aspm_lock);
>> +
>> +    for (i = 0; i < ARRAY_SIZE(aspm_sysfs_states); i++) {
>> +        const struct aspm_sysfs_state *st = aspm_sysfs_states + i;
>> +
>> +        if (link->aspm_enabled & st->mask)
>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "[%s] ",
>> +                     st->name);
>> +        else
>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
>> +                     st->name);
>> +    }
> 
>   # echo "-L1" > aspm_link_states
>   # cat aspm_link_states
>   L0S L1 L1.1 L1.2 [CLKPM]
> 
>   # echo "+L1" > aspm_link_states
>   # cat aspm_link_states
>   L0S [L1] L1.1 L1.2 [CLKPM]
> 
> In v1 & v2 [STATE] was used to show that a state was disabled, now [STATE] is used to show enabled. Is that intended?
> 
Yes, based on a review comment from you from May 12th:
"If we avoid the double negative, the documentation about to be written will be more clear and use of the sysfs file will be more intuitive."

>> +
>> +    if (link->clkpm_enabled)
>> +        len += scnprintf(buf + len, PAGE_SIZE - len, "[CLKPM] ");
>> +    else
>> +        len += scnprintf(buf + len, PAGE_SIZE - len, "CLKPM ");
>> +
> 
> Same as above...
> 
>> +    mutex_unlock(&aspm_lock);
>> +
>> +    len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
>> +
>> +    return len;
>> +}
> 
> Other than the above, v3 works as expected on my machine. Good work :)
> 
Great, thanks for the feedback!

OK, then let's see what still needs to be done to finish the patch series:
The new sysfs attribute needs to be documented. Anything else?

> Frederick Lawler
> 
> 
Heiner