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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 12.05.2019 03:02, Frederick Lawler wrote:
> Evening,
> 
> Heiner Kallweit wrote on 5/11/19 10:33 AM:> +static ssize_t aspm_disable_link_state_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_disable & st->disable_mask)
>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "[%s] ",
>> +                     st->name);
>> +        else
>> +            len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
>> +                     st->name);
>> +    }
>> +
>> +    if (link->clkpm_disable)
>> +        len += scnprintf(buf + len, PAGE_SIZE - len, "[CLKPM] ");
>> +    else
>> +        len += scnprintf(buf + len, PAGE_SIZE - len, "CLKPM ");
>> +
>> +    mutex_unlock(&aspm_lock);
>> +
>> +    len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
>> +
>> +    return len;
>> +}
> 
> I think it would be better to model the output to something similar to what lspci would do:
> 
> L1.1- L1.2- L0S+ ClockPM+, etc...
> 
> This better conveys the message that these states are enabled vs disabled.
> 
> I'd be interested to know if the use of [STATE]/STATE pattern is used elsewhere in the kernel. If so, then I'm cool with it :)
> 
This pattern is used in several places in sysfs. At first for "1 of n" selections like
in /sys/module/pcie_aspm/parameters/policy. But also for "m of n" selections,
examples would be LED triggers or active remote control protocols (drivers/media/rc).

>> +
>> +static ssize_t aspm_disable_link_state_store(struct device *dev,
>> +                         struct device_attribute *attr,
>> +                         const char *buf, size_t len)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    struct pcie_link_state *link;
>> +    char *buftmp = (char *)buf, *tok;
>> +    unsigned int disable_aspm, disable_clkpm;
>> +    bool first = true, add;
>> +    int err = 0, i;
>> +
>> +    if (aspm_disabled)
>> +        return -EPERM;
>> +
>> +    link = aspm_get_parent_link(pdev);
>> +    if (!link)
>> +        return -EOPNOTSUPP;
>> +
>> +    down_read(&pci_bus_sem);
>> +    mutex_lock(&aspm_lock);
>> +
>> +    disable_aspm = link->aspm_disable;
>> +    disable_clkpm = link->clkpm_disable;
>> +
>> +    while ((tok = strsep(&buftmp, " \n")) != NULL) {
>> +        bool found = false;
>> +
>> +        if (!*tok)
>> +            continue;
>> +
>> +        if (first) {
>> +            if (!strcasecmp(tok, "none")) {
>> +                disable_aspm = 0;
>> +                disable_clkpm = 0;
>> +                break;
>> +            }
>> +            if (!strcasecmp(tok, "all")) {
>> +                disable_aspm = ASPM_STATE_ALL;
>> +                disable_clkpm = 1;
>> +                break;
>> +            }
>> +            first = false;
>> +        }
>> +
>> +        if (*tok != '+' && *tok != '-') {
>> +            err = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        add = *tok++ == '+';
>> +
>> +        for (i = 0; i < ARRAY_SIZE(aspm_sysfs_states); i++) {
>> +            const struct aspm_sysfs_state *st =
>> +                        aspm_sysfs_states + i;
>> +
>> +            if (!strcasecmp(tok, st->name)) {
>> +                if (add)
>> +                    disable_aspm |= st->disable_mask;
>> +                else
>> +                    disable_aspm &= ~st->disable_mask;
>> +                found = true;
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (!found && !strcasecmp(tok, "clkpm")) {
>> +            disable_clkpm = add ? 1 : 0;
>> +            found = true;
>> +        }
>> +
>> +        if (!found) {
>> +            err = -EINVAL;
>> +            goto out;
>> +        }
>> +    }
>> +
>> +    if (disable_aspm & ASPM_STATE_L1)
>> +        disable_aspm |= ASPM_STATE_L1SS;
>> +
>> +    link->aspm_disable = disable_aspm;
>> +    link->clkpm_disable = disable_clkpm;
>> +
>> +    pcie_config_aspm_link(link, policy_to_aspm_state(link));
>> +    pcie_set_clkpm(link, policy_to_clkpm_state(link));
>> +out:
>> +    mutex_unlock(&aspm_lock);
>> +    up_read(&pci_bus_sem);
>> +
>> +    return err ?: len;
>> +}
>> +
>> +static DEVICE_ATTR_RW(aspm_disable_link_state);
>>   
> 
> Since we're introducing a new sysfs interface, would it be more appropriate to rename the sysfs files to aspm_set_link_state (or something to that effect)?
> 
> The syntax as it stands, means that to enable a state, a double negative must be used:
> 
> echo "-L1.1" > ./aspm_disable_link_state"
> vs
> echo "+L1.1" > ./aspm_set_link_state
> 
> 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.
> 
I think this is a valid point. Let me check and come up with a proposal.

> Thanks,
> Frederick Lawler
> 
> 
Heiner