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
