[PATCH v4] Export PBEC Data register into sysfs

[PATCH v4] Export PBEC Data register into sysfs

[Kobayashi,Daisuke]

This proposal aims to add a feature that outputs PCIe device power 
consumption information to sysfs.

Add support for PBEC (Power Budgeting Extended Capability) output 
to the PCIe driver. PBEC is defined in the 
PCIe specification(PCIe r6.0, sec 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. 

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>
---
 Documentation/ABI/testing/sysfs-bus-pci | 17 +++++++++++++++
 drivers/pci/pci-sysfs.c                 | 28 +++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index ecf47559f495..be1911d948ef 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -500,3 +500,20 @@ Description:
 		console drivers from the device.  Raw users of pci-sysfs
 		resourceN attributes must be terminated prior to resizing.
 		Success of the resizing operation is not guaranteed.
+
+What:		/sys/bus/pci/devices/.../power_budget
+Date:		September 2024
+Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
+Description:
+		This file provides information about the PCIe power budget
+		for the device. It is a read-only file that outputs the values
+		of the Power Budgeting Data Register for each power state as a
+		series of 32-bit hexadecimal values. Each line represents a
+		single Power Budgeting Data register entry, containing the
+		power budget for a specific power state.
+
+		The specific interpretation of these values depends on the
+		device and the PCIe specification. Refer to the PCIe
+		specification for detailed information about the Power
+		Budgeting Data register, including the encoding	of power
+		states and the interpretation of Base Power and Data Scale.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 2321fdfefd7d..c52814a33597 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 power_budget_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;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	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) {
+			if (len == 0)
+				return -EINVAL;
+			break;
+		}
+		len += sysfs_emit_at(buf, len, "%04x\n", data);
+	}
+
+	return len;
+}
+static DEVICE_ATTR_RO(power_budget);
+
 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_power_budget.attr,
 	NULL,
 };
 
-- 
2.45.0

RE: [PATCH v4] Export PBEC Data register into sysfs

[Daisuke Kobayashi (Fujitsu)]

Dear reviews.

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

Kobayashi Daisuke wrote:
> Subject: [PATCH v4] Export PBEC Data register into sysfs
> 
> This proposal aims to add a feature that outputs PCIe device power
> consumption information to sysfs.
> 
> Add support for PBEC (Power Budgeting Extended Capability) output to the
> PCIe driver. PBEC is defined in the PCIe specification(PCIe r6.0, sec 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.
> 
> 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>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci | 17 +++++++++++++++
>  drivers/pci/pci-sysfs.c                 | 28
> +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> b/Documentation/ABI/testing/sysfs-bus-pci
> index ecf47559f495..be1911d948ef 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -500,3 +500,20 @@ Description:
>  		console drivers from the device.  Raw users of pci-sysfs
>  		resourceN attributes must be terminated prior to resizing.
>  		Success of the resizing operation is not guaranteed.
> +
> +What:		/sys/bus/pci/devices/.../power_budget
> +Date:		September 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file provides information about the PCIe power budget
> +		for the device. It is a read-only file that outputs the values
> +		of the Power Budgeting Data Register for each power state as
> a
> +		series of 32-bit hexadecimal values. Each line represents a
> +		single Power Budgeting Data register entry, containing the
> +		power budget for a specific power state.
> +
> +		The specific interpretation of these values depends on the
> +		device and the PCIe specification. Refer to the PCIe
> +		specification for detailed information about the Power
> +		Budgeting Data register, including the encoding	of power
> +		states and the interpretation of Base Power and Data Scale.
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index
> 2321fdfefd7d..c52814a33597 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 power_budget_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;
> +
> +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> +	if (!pos)
> +		return -EINVAL;
> +
> +	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) {
> +			if (len == 0)
> +				return -EINVAL;
> +			break;
> +		}
> +		len += sysfs_emit_at(buf, len, "%04x\n", data);
> +	}
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RO(power_budget);
> +
>  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_power_budget.attr,
>  	NULL,
>  };
> 
> --
> 2.45.0

RE: [PATCH v4] 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.
> 
> Add support for PBEC (Power Budgeting Extended Capability) output
> to the PCIe driver. PBEC is defined in the
> PCIe specification(PCIe r6.0, sec 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.
> 
> 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>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci | 17 +++++++++++++++
>  drivers/pci/pci-sysfs.c                 | 28
> +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> b/Documentation/ABI/testing/sysfs-bus-pci
> index ecf47559f495..be1911d948ef 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -500,3 +500,20 @@ Description:
>  		console drivers from the device.  Raw users of pci-sysfs
>  		resourceN attributes must be terminated prior to resizing.
>  		Success of the resizing operation is not guaranteed.
> +
> +What:		/sys/bus/pci/devices/.../power_budget
> +Date:		September 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file provides information about the PCIe power budget
> +		for the device. It is a read-only file that outputs the values
> +		of the Power Budgeting Data Register for each power state as
> a
> +		series of 32-bit hexadecimal values. Each line represents a
> +		single Power Budgeting Data register entry, containing the
> +		power budget for a specific power state.
> +
> +		The specific interpretation of these values depends on the
> +		device and the PCIe specification. Refer to the PCIe
> +		specification for detailed information about the Power
> +		Budgeting Data register, including the encoding	of power
> +		states and the interpretation of Base Power and Data Scale.
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 2321fdfefd7d..c52814a33597 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 power_budget_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;
> +
> +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> +	if (!pos)
> +		return -EINVAL;
> +
> +	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) {
> +			if (len == 0)
> +				return -EINVAL;
> +			break;
> +		}
> +		len += sysfs_emit_at(buf, len, "%04x\n", data);
> +	}
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RO(power_budget);
> +
>  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_power_budget.attr,
>  	NULL,
>  };
> 
> --
> 2.45.0

Dear reviews.

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

RE: [PATCH v4] Export PBEC Data register into sysfs

[Daisuke Kobayashi (Fujitsu)]

Gentle ping. Could you check this patch?

> Kobayashi, Daisuke wrote:
> > This proposal aims to add a feature that outputs PCIe device power
> > consumption information to sysfs.
> >
> > Add support for PBEC (Power Budgeting Extended Capability) output to
> > the PCIe driver. PBEC is defined in the PCIe specification(PCIe r6.0,
> > sec 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.
> >
> > 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>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-pci | 17 +++++++++++++++
> >  drivers/pci/pci-sysfs.c                 | 28
> > +++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> > b/Documentation/ABI/testing/sysfs-bus-pci
> > index ecf47559f495..be1911d948ef 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -500,3 +500,20 @@ Description:
> >  		console drivers from the device.  Raw users of pci-sysfs
> >  		resourceN attributes must be terminated prior to resizing.
> >  		Success of the resizing operation is not guaranteed.
> > +
> > +What:		/sys/bus/pci/devices/.../power_budget
> > +Date:		September 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This file provides information about the PCIe power budget
> > +		for the device. It is a read-only file that outputs the values
> > +		of the Power Budgeting Data Register for each power state as
> > a
> > +		series of 32-bit hexadecimal values. Each line represents a
> > +		single Power Budgeting Data register entry, containing the
> > +		power budget for a specific power state.
> > +
> > +		The specific interpretation of these values depends on the
> > +		device and the PCIe specification. Refer to the PCIe
> > +		specification for detailed information about the Power
> > +		Budgeting Data register, including the encoding	of power
> > +		states and the interpretation of Base Power and Data Scale.
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index
> > 2321fdfefd7d..c52814a33597 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 power_budget_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;
> > +
> > +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> > +	if (!pos)
> > +		return -EINVAL;
> > +
> > +	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) {
> > +			if (len == 0)
> > +				return -EINVAL;
> > +			break;
> > +		}
> > +		len += sysfs_emit_at(buf, len, "%04x\n", data);
> > +	}
> > +
> > +	return len;
> > +}
> > +static DEVICE_ATTR_RO(power_budget);
> > +
> >  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_power_budget.attr,
> >  	NULL,
> >  };
> >
> > --
> > 2.45.0
> 
> Dear reviews.
> 
> Could you please take a look at the patch when you have time?
> Thank you for your time and consideration.

Re: [PATCH v4] Export PBEC Data register into sysfs

[Jonathan Cameron]

On Wed, 11 Sep 2024 10:20:53 +0900
"Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:

> This proposal aims to add a feature that outputs PCIe device power 
> consumption information to sysfs.
> 
> Add support for PBEC (Power Budgeting Extended Capability) output 
> to the PCIe driver. PBEC is defined in the 
> PCIe specification(PCIe r6.0, sec 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. 
> 
> 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>
Hi,

> ---
>  Documentation/ABI/testing/sysfs-bus-pci | 17 +++++++++++++++
>  drivers/pci/pci-sysfs.c                 | 28 +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ecf47559f495..be1911d948ef 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -500,3 +500,20 @@ Description:
>  		console drivers from the device.  Raw users of pci-sysfs
>  		resourceN attributes must be terminated prior to resizing.
>  		Success of the resizing operation is not guaranteed.
> +
> +What:		/sys/bus/pci/devices/.../power_budget
> +Date:		September 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file provides information about the PCIe power budget
> +		for the device. It is a read-only file that outputs the values
> +		of the Power Budgeting Data Register for each power state as a
> +		series of 32-bit hexadecimal values. Each line represents a
> +		single Power Budgeting Data register entry, containing the
> +		power budget for a specific power state.
> +
> +		The specific interpretation of these values depends on the
> +		device and the PCIe specification. Refer to the PCIe
> +		specification for detailed information about the Power
> +		Budgeting Data register, including the encoding	of power
> +		states and the interpretation of Base Power and Data Scale.

Is there precedence for similar register values just being available via
sysfs?  This definitely isn't in keeping with general desirable sysfs
interface characteristics.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 2321fdfefd7d..c52814a33597 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 power_budget_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;
> +
> +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> +	if (!pos)
> +		return -EINVAL;
> +
> +	for (i = 0; i < 0xff; i++) {
> +		pci_write_config_byte(pci_dev, pos + PCI_PWR_DSR, (u8)i);

Why not make i a u8? Would remove need for cast and otherwise make no
difference that I can see.

> +		pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
> +		if (!data) {
> +			if (len == 0)
> +				return -EINVAL;
> +			break;
> +		}
> +		len += sysfs_emit_at(buf, len, "%04x\n", data);

It's not user friendly to just output the register content, and this
is breaking the one thing per sysfs file ABI rules.

Various possible sysfs structures may make more sense.

1) Directory with files for each entry found. Each file
   is one thing so 
   power_budget/X_power - maths done to take base power and apply the data scale.
                X_pm_state
                X_pm_substate
                X_type - potentially with nice strings for each type.
                X_rail  - 12V, 3,3V , 1.5V/1.8V, 48V, 5V, thermal
                X_connector - 
	        X_connector_type

With the stuff in the extended bit only visible if flag in bit 31 is set.

> +	}
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RO(power_budget);
> +
>  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_power_budget.attr,
>  	NULL,
>  };
>  

RE: [PATCH v4] Export PBEC Data register into sysfs

[Daisuke Kobayashi (Fujitsu)]

Jonathan Cameron wrote:
> On Wed, 11 Sep 2024 10:20:53 +0900
> "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:
> 
> > This proposal aims to add a feature that outputs PCIe device power
> > consumption information to sysfs.
> >
> > Add support for PBEC (Power Budgeting Extended Capability) output to
> > the PCIe driver. PBEC is defined in the PCIe specification(PCIe r6.0,
> > sec 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.
> >
> > 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>
> Hi,
> 
> > ---
> >  Documentation/ABI/testing/sysfs-bus-pci | 17 +++++++++++++++
> >  drivers/pci/pci-sysfs.c                 | 28
> +++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> > b/Documentation/ABI/testing/sysfs-bus-pci
> > index ecf47559f495..be1911d948ef 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -500,3 +500,20 @@ Description:
> >  		console drivers from the device.  Raw users of pci-sysfs
> >  		resourceN attributes must be terminated prior to resizing.
> >  		Success of the resizing operation is not guaranteed.
> > +
> > +What:		/sys/bus/pci/devices/.../power_budget
> > +Date:		September 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This file provides information about the PCIe power budget
> > +		for the device. It is a read-only file that outputs the values
> > +		of the Power Budgeting Data Register for each power state as
> a
> > +		series of 32-bit hexadecimal values. Each line represents a
> > +		single Power Budgeting Data register entry, containing the
> > +		power budget for a specific power state.
> > +
> > +		The specific interpretation of these values depends on the
> > +		device and the PCIe specification. Refer to the PCIe
> > +		specification for detailed information about the Power
> > +		Budgeting Data register, including the encoding	of power
> > +		states and the interpretation of Base Power and Data Scale.
> 
> Is there precedence for similar register values just being available via sysfs?
> This definitely isn't in keeping with general desirable sysfs interface
> characteristics.
> 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index
> > 2321fdfefd7d..c52814a33597 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 power_budget_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;
> > +
> > +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> > +	if (!pos)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < 0xff; i++) {
> > +		pci_write_config_byte(pci_dev, pos + PCI_PWR_DSR, (u8)i);
> 
> Why not make i a u8? Would remove need for cast and otherwise make no
> difference that I can see.
> 
> > +		pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA,
> &data);
> > +		if (!data) {
> > +			if (len == 0)
> > +				return -EINVAL;
> > +			break;
> > +		}
> > +		len += sysfs_emit_at(buf, len, "%04x\n", data);
> 
> It's not user friendly to just output the register content, and this is breaking the
> one thing per sysfs file ABI rules.
> 
> Various possible sysfs structures may make more sense.
> 
> 1) Directory with files for each entry found. Each file
>    is one thing so
>    power_budget/X_power - maths done to take base power and apply the
> data scale.
>                 X_pm_state
>                 X_pm_substate
>                 X_type - potentially with nice strings for each type.
>                 X_rail  - 12V, 3,3V , 1.5V/1.8V, 48V, 5V, thermal
>                 X_connector -
> 	        X_connector_type
> 
> With the stuff in the extended bit only visible if flag in bit 31 is set.
> 
Thank you for your comment.
We will modify the implementation to follow that "one thing per sysfs file" rules.
> > +	}
> > +
> > +	return len;
> > +}
> > +static DEVICE_ATTR_RO(power_budget);
> > +
> >  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_power_budget.attr,
> >  	NULL,
> >  };
> >

Re: [PATCH v4] Export PBEC Data register into sysfs

[Krzysztof Wilczyński]

Hello,

[...]
> We will modify the implementation to follow that "one thing per sysfs file" rules.

A favour to ask of you: if possible, when sending another version of this
patch, if you could include a changelog as the comment, then it would be
great.  This way it would be easy to follow-up on what changes were made
since last version.

Thank you!

	Krzysztof

RE: [PATCH v4] Export PBEC Data register into sysfs

[Daisuke Kobayashi (Fujitsu)]

Jonathan Cameron wrote:
> "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:
> 
[...]
> It's not user friendly to just output the register content, and this is breaking the
> one thing per sysfs file ABI rules.
> 
> Various possible sysfs structures may make more sense.
> 
> 1) Directory with files for each entry found. Each file
>    is one thing so
>    power_budget/X_power - maths done to take base power and apply the
> data scale.
>                 X_pm_state
>                 X_pm_substate
>                 X_type - potentially with nice strings for each type.
>                 X_rail  - 12V, 3,3V , 1.5V/1.8V, 48V, 5V, thermal
>                 X_connector -
> 	        X_connector_type
> 
> With the stuff in the extended bit only visible if flag in bit 31 is set.

Following the ABI rules, I propose the following directory structure, 
based on the suggestion provided. 
Please review it and let me know if you have any concerns. 
If there are no objections, I will implement this and release it as the next v5 patch.

power_budget
    ├── 0   - The index number to be set in the Data Select Register
    │   ├── power - Value considering base power and data scale
    │   ├── pm_state - D0, D1, D2, D3
    │   ├── pm_substate
    │   ├── type
    │   ├── rail - 12V, 3.3V, 1.5V/1.8V, 48V, 5V, Thermal
    │   ├── connector
    │   └── connector_type
    ├── 1
    │   ├── power
    │   ├── pm_state
    │   ├── 

RE: [PATCH v4] Export PBEC Data register into sysfs

[Daisuke Kobayashi (Fujitsu)]

> Jonathan Cameron wrote:
> > "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:
> >
> [...]
> > It's not user friendly to just output the register content, and this
> > is breaking the one thing per sysfs file ABI rules.
> >
> > Various possible sysfs structures may make more sense.
> >
> > 1) Directory with files for each entry found. Each file
> >    is one thing so
> >    power_budget/X_power - maths done to take base power and apply the
> > data scale.
> >                 X_pm_state
> >                 X_pm_substate
> >                 X_type - potentially with nice strings for each type.
> >                 X_rail  - 12V, 3,3V , 1.5V/1.8V, 48V, 5V, thermal
> >                 X_connector -
> > 	        X_connector_type
> >
> > With the stuff in the extended bit only visible if flag in bit 31 is set.
> 
> Following the ABI rules, I propose the following directory structure, based on
> the suggestion provided.
> Please review it and let me know if you have any concerns.
> If there are no objections, I will implement this and release it as the next v5
> patch.
> 
> power_budget
>     ├── 0   - The index number to be set in the Data Select Register
>     │   ├── power - Value considering base power and data scale
>     │   ├── pm_state - D0, D1, D2, D3
>     │   ├── pm_substate
>     │   ├── type
>     │   ├── rail - 12V, 3.3V, 1.5V/1.8V, 48V, 5V, Thermal
>     │   ├── connector
>     │   └── connector_type
>     ├── 1
>     │   ├── power
>     │   ├── pm_state
>     │   ├──

After reviewing the existing pci-sysfs implementation, I've found that 
implementing the directory structure that I proposed cleanly is proving difficult. 
Therefore, I'm proposing a second option with the following structure.

power_budget
    ├── data_select - Read-Write value selecting the power budget to be displayed.
    ├── power - Value considering base power and data scale
    ├── pm_state - D0, D1, D2, D3
    ├── pm_substate
    ├── type
    ├── rail - 12V, 3.3V, 1.5V/1.8V, 48V, 5V, Thermal
    ├── connector
    └── connector_type