[RFC PATCH v1] Export PBEC Data register into sysfs

[RFC PATCH v1] Export PBEC Data register into sysfs

[Kobayashi,Daisuke]

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.

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.

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;
+
+	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);
+
 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

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

[Daisuke Kobayashi (Fujitsu)]

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.
> 
> 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.
> 
> 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;
> +
> +	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);
> +
>  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

Dear reviews.

Could you please take a look at the patch when you have time?
Thank you for your time and consideration.

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

[Krzysztof Wilczyński]

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

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

[Krzysztof Wilczyński]

Hello,

[...]
> Could you please take a look at the patch when you have time?
> Thank you for your time and consideration.

Kobayashi-san, thank you for chasing after this, and sorry for the delay.

I left some comments.  Have a look.

	Krzysztof

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

[Bjorn Helgaas]

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
> 

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

[Daisuke Kobayashi (Fujitsu)]

Krzysztof Wilczyński <kw@linux.com> wrote:
> 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?
> 
Yes, I'm considering making changes to the pciutils project to expose this 
information using the lspci utility.

> That said, we already expose "config" sysfs object, would using it work to
> obtain the data you need?
> 
I considered reading from a config file, but since this requires writing to
configuration space, I feel it would be better to implement it in a kernel driver.

> > +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?
> 
This assumes the maximum value of the index, but the maximum value is not
stated in the specifications. Since DSR is an 8-bit register, the maximum value
is assumed to be 0xff and set as the loop count.
Would it be better to define it as a constant?

> > +		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;
> 
Thank you for your review.
I will fix any errors or unnecessary processing in the next patch.

> 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.
> 
My current assumption is that it is accessed when lspci is run, so I don't think
it is accessed very often.

> Thank you!
> 
> 	Krzysztof

Thank you!

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

[Daisuke Kobayashi (Fujitsu)]

Hello, I hope this email finds you well.

Please let me know if there is anything I should do to get this feature merged.
I'd appreciate it if you could take a look when you have a chance. 
I'm happy to answer any questions you might have.

Thanks.

Kobayashi Daisuke wrote:
> Krzysztof Wilczyński <kw@linux.com> wrote:
> > 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?
> >
> Yes, I'm considering making changes to the pciutils project to expose this
> information using the lspci utility.
> 
> > That said, we already expose "config" sysfs object, would using it
> > work to obtain the data you need?
> >
> I considered reading from a config file, but since this requires writing to
> configuration space, I feel it would be better to implement it in a kernel driver.
> 
> > > +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?
> >
> This assumes the maximum value of the index, but the maximum value is not
> stated in the specifications. Since DSR is an 8-bit register, the maximum value
> is assumed to be 0xff and set as the loop count.
> Would it be better to define it as a constant?
> 
> > > +		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;
> >
> Thank you for your review.
> I will fix any errors or unnecessary processing in the next patch.
> 
> > 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.
> >
> My current assumption is that it is accessed when lspci is run, so I don't think
> it is accessed very often.
> 
> > Thank you!
> >
> > 	Krzysztof
> 
> Thank you!

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

['Krzysztof Wilczyński']

Kobayashi-san,

[...]
> Please let me know if there is anything I should do to get this feature merged.
> I'd appreciate it if you could take a look when you have a chance. 
> I'm happy to answer any questions you might have.

To get your change merged, what you need to do, would be to:

  - Address review comments from me and from Bjorn
  - Send a new patch that is not an RFC one

Once you send a new version, we need to allow others a little bit of time
to review your patch.

Meanwhile, let us know if there is anything else you would need help with.

	Krzysztof

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

[Daisuke Kobayashi (Fujitsu)]

Krzysztof Wilczyński wrote
> Kobayashi-san,
> 
> [...]
> > Please let me know if there is anything I should do to get this feature merged.
> > I'd appreciate it if you could take a look when you have a chance.
> > I'm happy to answer any questions you might have.
> 
> To get your change merged, what you need to do, would be to:
> 
>   - Address review comments from me and from Bjorn
>   - Send a new patch that is not an RFC one
> 
> Once you send a new version, we need to allow others a little bit of time to
> review your patch.
> 
> Meanwhile, let us know if there is anything else you would need help with.
> 
> 	Krzysztof

Thank you for your detailed explanation of the situation.
I will create a new version of the patch as per your suggestions.