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 :)
>
>> +
>> +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)?
>
I changed the attribute to aspm_link_states. The "set" isn't needed IMO. It's a RW attribute and that should make
clear it can be used to change the state.
> 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.
>
> Thanks,
> Frederick Lawler
>
>
Heiner
