Re: [RFC PATCH v1] Export PBEC Data register into sysfs

["Krzysztof Wilczyński" <kw@linux.com>]

Hello,

[...]
> PCIe devices can significantly impact the overall power consumption of
> a system. However, obtaining PCIe device power consumption information 
> has traditionally been difficult. This is because the 'lspci' command, 
> which is a standard tool for displaying information about PCI devices, 
> cannot access PBEC information. `lspci` is a standard tool for displaying
> information about PCI devices.

Will you also be making changes to the pciutils project to expose this
using the lspci utility?

That said, we already expose "config" sysfs object, would using it work to
obtain the data you need?

> +static ssize_t pbec_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +	size_t len = 0;
> +	int i, pos;
> +	u32 data;
> +
> +	if (!pci_is_pcie(pci_dev))
> +		return 0;

This is not needed, I believe the "is_visible" callback for this specific
attributes group checks for this already.

> +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> +	if (!pos)
> +		return 0;

It would be -EINVAL, customarily.  I suppose, the -ENOTSUPP or -ENOSYS could
do too, but most of the other PCI sysfs objects returns -EINVAL back to the
userspace to indicate that something is wrong.

> +	for (i = 0; i < 0xff; i++) {

Does this 0xff need to be documented?  Something specific to the PBEC feature?

> +		pci_write_config_byte(pci_dev, pos + PCI_PWR_DSR, (u8)i);
> +		pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
> +		if (!data)
> +			break;

We should return an error here, something like -EINVAL perhaps.

> +		len += sysfs_emit_at(buf, len, "%04x\n", data);
> +	}
> +
> +	return len;

How frequently do you think this new sysfs object would be accessed?  It's
not uncommon for the PCI configuration space access to be slow for some
devices.

Thank you!

	Krzysztof