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)?
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
