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

[Bjorn Helgaas <helgaas@kernel.org>]

Note subject line convention for drivers/pci (use "git log --oneline
drivers/pci/pci-sysfs.c")

On Wed, Jun 26, 2024 at 01:43:30PM +0900, Kobayashi,Daisuke wrote:
> This proposal aims to add a feature that outputs PCIe device power 
> consumption information to sysfs.
> 
> This feature can be implemented by adding support for PBEC (Power 
> Budgeting Extended Capability) output to the PCIe driver. PBEC is 
> defined in the PCIe specification(7.8.1) and is a standard method for
> obtaining device power consumption information.

s/This feature can be implemented by adding support/Add support/

(To change this to imperative mood and say what the patch actually
*does*, as opposed to what *could* be done.)

Include spec revision, e.g., "PCIe r6.0, sec 7.8.1"

> 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.

lspci detail isn't really necessary here.  Maybe lspci *could* be
extended to display PBEC information, but that's a separate project.
It looks like the Data/Data Select issue might mean lspci would have
to get the info from the kernel, e.g., from this sysfs file, to avoid
synchronization issues.

> The PBEC Data register changes depending on the value of the PBEC Data 
> Select register. To obtain all PBEC Data register values defined in the 
> device, obtain the value of the PBEC Data register while changing the 
> value of the PBEC Data Select register.
> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/pci/pci-sysfs.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 2321fdfefd7d..b13d30da38a1 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -182,6 +182,33 @@ static ssize_t resource_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(resource);
>  
> +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;

Unnecessary.  Already covered by pcie_dev_attrs_are_visible().

> +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> +	if (!pos)
> +		return 0;
> +
> +	for (i = 0; i < 0xff; i++) {
> +		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;
> +		len += sysfs_emit_at(buf, len, "%04x\n", data);
> +	}
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RO(pbec);

Needs a more descriptive name, I think.  Also some documentation in
Documentation/ABI/.

>  static ssize_t max_link_speed_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
> @@ -629,6 +656,7 @@ static struct attribute *pcie_dev_attrs[] = {
>  	&dev_attr_current_link_width.attr,
>  	&dev_attr_max_link_width.attr,
>  	&dev_attr_max_link_speed.attr,
> +	&dev_attr_pbec.attr,
>  	NULL,
>  };
>  
> -- 
> 2.44.0
>