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

[Frederick Lawler <fred@fredlawl.com>]

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