[PATCH v5 0/2] Export PBEC Data register into sysfs

[PATCH v5 0/2] 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.

Changes:
v4->v5
https://lore.kernel.org/linux-pci/20240911012053.345286-1-kobayashi.da-06@fujitsu.com/
- All register values were output to a single file. Now, a dedicated directory
 is created, and individual values are output to separate files within this directory.
- Multiple helper functions are added to handle the output of individual register
 values to their respective files.

Kobayashi,Daisuke (2):
  Add helper functions for Power Budgeting Extended Capability.
  Export Power Budgeting Extended Capability into pci-bus-sysfs

 Documentation/ABI/testing/sysfs-bus-pci |  62 ++++++++
 drivers/pci/pci-sysfs.c                 | 179 ++++++++++++++++++++++++
 drivers/pci/pci.h                       |   3 +
 drivers/pci/probe.c                     |  66 +++++++++
 include/uapi/linux/pci_regs.h           |   3 +-
 5 files changed, 312 insertions(+), 1 deletion(-)

-- 
2.45.0

[PATCH v5 1/2] Add helper functions for Power Budgeting Extended Capability

[Kobayashi,Daisuke]

Add functions to return a text description of the supplied
power_budget scale/base power/rail.
Export these functions so they can be used by modules.

Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 drivers/pci/pci.h             |  3 ++
 drivers/pci/probe.c           | 66 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/pci_regs.h |  3 +-
 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 14d00ce45bfa..967b53996694 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -374,6 +374,9 @@ static inline int pcie_dev_speed_mbps(enum pci_bus_speed speed)
 }
 
 const char *pci_speed_string(enum pci_bus_speed speed);
+const char *pci_power_budget_scale_string(u8 num);
+const char *pci_power_budget_alt_encode_string(u8 num);
+const char *pci_power_budget_rail_string(u8 num);
 enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev);
 enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev);
 void __pcie_print_link_status(struct pci_dev *dev, bool verbose);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index f1615805f5b0..18a920527f69 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -748,6 +748,72 @@ void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
 }
 EXPORT_SYMBOL_GPL(pcie_update_link_speed);
 
+const char *pci_power_budget_rail_string(u8 num)
+{
+	/* Indexed by the rail number */
+	static const char *rail_strings[] = {
+	    "Power(12V)",		/* 0x00 */
+	    "Power(3.3V)",		/* 0x01 */
+	    "Power(1.5Vor1.8V)",		/* 0x02 */
+	    "Power(48V)",		/* 0x03 */
+	    "Power(5V)",		/* 0x04 */
+	    "Thermal",			/* 0x05 */
+	};
+
+	if (num < ARRAY_SIZE(rail_strings))
+		return rail_strings[num];
+	return "Unknown";
+}
+EXPORT_SYMBOL_GPL(pci_power_budget_rail_string);
+
+const char *pci_power_budget_scale_string(u8 num)
+{
+	/* Indexed by the scale number */
+	static const char *scale_strings[] = {
+	    "x1.0",		/* 0x00 */
+	    "x0.1",		/* 0x01 */
+	    "x0.01",		/* 0x02 */
+	    "x0.001",		/* 0x03 */
+	    "x10",		/* 0x04 */
+	    "x100",			/* 0x05 */
+	};
+
+	if (num < ARRAY_SIZE(scale_strings))
+		return scale_strings[num];
+	return "Unknown";
+}
+EXPORT_SYMBOL_GPL(pci_power_budget_scale_string);
+
+const char *pci_power_budget_alt_encode_string(u8 num)
+{
+	u8 n;
+	n = num & 0x0f;
+	/* Indexed by the Base Power number */
+	static const char *Power_strings[] = {
+	    "> 239 W and ≤ 250 W Slot Power Limit",		/* 0xF0 */
+	    "> 250 W and ≤ 275 W Slot Power Limit",		/* 0xF1 */
+	    "> 275 W and ≤ 300 W Slot Power Limit",		/* 0xF2 */
+	    "> 300 W and ≤ 325 W Slot Power Limit",		/* 0xF3 */
+	    "> 325 W and ≤ 350 W Slot Power Limit",		/* 0xF4 */
+	    "> 350 W and ≤ 375 W Slot Power Limit",		/* 0xF5 */
+	    "> 375 W and ≤ 400 W Slot Power Limit",		/* 0xF6 */
+	    "> 400 W and ≤ 425 W Slot Power Limit",		/* 0xF7 */
+	    "> 425 W and ≤ 450 W Slot Power Limit",		/* 0xF8 */
+	    "> 450 W and ≤ 475 W Slot Power Limit",		/* 0xF9 */
+	    "> 475 W and ≤ 500 W Slot Power Limit",		/* 0xFA */
+	    "> 500 W and ≤ 525 W Slot Power Limit",		/* 0xFB */
+	    "> 525 W and ≤ 550 W Slot Power Limit",		/* 0xFC */
+	    "> 550 W and ≤ 575 W Slot Power Limit",		/* 0xFD */
+	    "> 575 W and ≤ 600 W Slot Power Limit",		/* 0xFE */
+	    "Greater than 600 W",		/* 0xFF */
+	};
+
+	if (n < ARRAY_SIZE(Power_strings))
+		return Power_strings[n];
+	return "Unknown";
+}
+EXPORT_SYMBOL_GPL(pci_power_budget_alt_encode_string);
+
 static unsigned char agp_speeds[] = {
 	AGP_UNKNOWN,
 	AGP_1X,
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 12323b3334a9..3a5e238b98d8 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -842,11 +842,12 @@
 #define PCI_PWR_DSR		0x04	/* Data Select Register */
 #define PCI_PWR_DATA		0x08	/* Data Register */
 #define  PCI_PWR_DATA_BASE(x)	((x) & 0xff)	    /* Base Power */
-#define  PCI_PWR_DATA_SCALE(x)	(((x) >> 8) & 3)    /* Data Scale */
+#define  PCI_PWR_DATA_SCALE(x)	(((x) >> 8) & 3)    /* Data Scale[1:0] */
 #define  PCI_PWR_DATA_PM_SUB(x)	(((x) >> 10) & 7)   /* PM Sub State */
 #define  PCI_PWR_DATA_PM_STATE(x) (((x) >> 13) & 3) /* PM State */
 #define  PCI_PWR_DATA_TYPE(x)	(((x) >> 15) & 7)   /* Type */
 #define  PCI_PWR_DATA_RAIL(x)	(((x) >> 18) & 7)   /* Power Rail */
+#define  PCI_PWR_DATA_SCALE_UP(x)	(((x) >> 21) & 1)    /* Data Scale[2] */
 #define PCI_PWR_CAP		0x0c	/* Capability */
 #define  PCI_PWR_CAP_BUDGET(x)	((x) & 1)	/* Included in system budget */
 #define PCI_EXT_CAP_PWR_SIZEOF	0x10
-- 
2.45.0

Re: [PATCH v5 1/2] Add helper functions for Power Budgeting Extended Capability

[Jonathan Cameron]

On Tue, 10 Dec 2024 13:08:20 +0900
"Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:

> Add functions to return a text description of the supplied
> power_budget scale/base power/rail.
> Export these functions so they can be used by modules.
> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>

Hi.  A few comments inline,

Thanks,

Jonathan

> ---
>  drivers/pci/pci.h             |  3 ++
>  drivers/pci/probe.c           | 66 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/pci_regs.h |  3 +-
>  3 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 14d00ce45bfa..967b53996694 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -374,6 +374,9 @@ static inline int pcie_dev_speed_mbps(enum pci_bus_speed speed)
>  }
>  
>  const char *pci_speed_string(enum pci_bus_speed speed);
> +const char *pci_power_budget_scale_string(u8 num);
> +const char *pci_power_budget_alt_encode_string(u8 num);
> +const char *pci_power_budget_rail_string(u8 num);
>  enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev);
>  enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev);
>  void __pcie_print_link_status(struct pci_dev *dev, bool verbose);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f1615805f5b0..18a920527f69 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -748,6 +748,72 @@ void pcie_update_link_speed(struct pci_bus *bus, u16 linksta)
>  }
>  EXPORT_SYMBOL_GPL(pcie_update_link_speed);
>  
> +const char *pci_power_budget_rail_string(u8 num)
> +{
> +	/* Indexed by the rail number */
> +	static const char *rail_strings[] = {
> +	    "Power(12V)",		/* 0x00 */

For these you could do
	[0x00] = "Power(12V"), rather than the comment?
Makes the code self documenting and keeps it easy to compare
with the specification.


> +	    "Power(3.3V)",		/* 0x01 */
> +	    "Power(1.5Vor1.8V)",		/* 0x02 */
> +	    "Power(48V)",		/* 0x03 */
> +	    "Power(5V)",		/* 0x04 */
> +	    "Thermal",			/* 0x05 */
> +	};
> +
> +	if (num < ARRAY_SIZE(rail_strings))
> +		return rail_strings[num];
> +	return "Unknown";
> +}
> +EXPORT_SYMBOL_GPL(pci_power_budget_rail_string);
> +
> +const char *pci_power_budget_scale_string(u8 num)
> +{
> +	/* Indexed by the scale number */
> +	static const char *scale_strings[] = {
As above.

> +	    "x1.0",		/* 0x00 */
> +	    "x0.1",		/* 0x01 */
> +	    "x0.01",		/* 0x02 */
> +	    "x0.001",		/* 0x03 */
> +	    "x10",		/* 0x04 */
> +	    "x100",			/* 0x05 */
> +	};
> +
> +	if (num < ARRAY_SIZE(scale_strings))
> +		return scale_strings[num];
> +	return "Unknown";
> +}
> +EXPORT_SYMBOL_GPL(pci_power_budget_scale_string);
> +
> +const char *pci_power_budget_alt_encode_string(u8 num)
> +{
> +	u8 n;
> +	n = num & 0x0f;

	u8 n = num & 0x0f;

> +	/* Indexed by the Base Power number */
> +	static const char *Power_strings[] = {
> +	    "> 239 W and ≤ 250 W Slot Power Limit",		/* 0xF0 */
> +	    "> 250 W and ≤ 275 W Slot Power Limit",		/* 0xF1 */
> +	    "> 275 W and ≤ 300 W Slot Power Limit",		/* 0xF2 */
> +	    "> 300 W and ≤ 325 W Slot Power Limit",		/* 0xF3 */
> +	    "> 325 W and ≤ 350 W Slot Power Limit",		/* 0xF4 */
> +	    "> 350 W and ≤ 375 W Slot Power Limit",		/* 0xF5 */
> +	    "> 375 W and ≤ 400 W Slot Power Limit",		/* 0xF6 */
> +	    "> 400 W and ≤ 425 W Slot Power Limit",		/* 0xF7 */
> +	    "> 425 W and ≤ 450 W Slot Power Limit",		/* 0xF8 */
> +	    "> 450 W and ≤ 475 W Slot Power Limit",		/* 0xF9 */
> +	    "> 475 W and ≤ 500 W Slot Power Limit",		/* 0xFA */
> +	    "> 500 W and ≤ 525 W Slot Power Limit",		/* 0xFB */
> +	    "> 525 W and ≤ 550 W Slot Power Limit",		/* 0xFC */
> +	    "> 550 W and ≤ 575 W Slot Power Limit",		/* 0xFD */
> +	    "> 575 W and ≤ 600 W Slot Power Limit",		/* 0xFE */
> +	    "Greater than 600 W",		/* 0xFF */

Technically the spec says it's "Reserved for values greater than 600 W", but
which I assume they mean a new interface will be needed if you see 0xFF.
I guess your text works for that though.

> +	};
> +
> +	if (n < ARRAY_SIZE(Power_strings))
> +		return Power_strings[n];
> +	return "Unknown";
> +}
> +EXPORT_SYMBOL_GPL(pci_power_budget_alt_encode_string);

Why do you need the export? Aren't all the users in the core PCI code?

> +
>  static unsigned char agp_speeds[] = {
>  	AGP_UNKNOWN,
>  	AGP_1X,
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 12323b3334a9..3a5e238b98d8 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -842,11 +842,12 @@
>  #define PCI_PWR_DSR		0x04	/* Data Select Register */
>  #define PCI_PWR_DATA		0x08	/* Data Register */
>  #define  PCI_PWR_DATA_BASE(x)	((x) & 0xff)	    /* Base Power */
> -#define  PCI_PWR_DATA_SCALE(x)	(((x) >> 8) & 3)    /* Data Scale */
> +#define  PCI_PWR_DATA_SCALE(x)	(((x) >> 8) & 3)    /* Data Scale[1:0] */
>  #define  PCI_PWR_DATA_PM_SUB(x)	(((x) >> 10) & 7)   /* PM Sub State */
>  #define  PCI_PWR_DATA_PM_STATE(x) (((x) >> 13) & 3) /* PM State */
>  #define  PCI_PWR_DATA_TYPE(x)	(((x) >> 15) & 7)   /* Type */
>  #define  PCI_PWR_DATA_RAIL(x)	(((x) >> 18) & 7)   /* Power Rail */
> +#define  PCI_PWR_DATA_SCALE_UP(x)	(((x) >> 21) & 1)    /* Data Scale[2] */
>  #define PCI_PWR_CAP		0x0c	/* Capability */
>  #define  PCI_PWR_CAP_BUDGET(x)	((x) & 1)	/* Included in system budget */
>  #define PCI_EXT_CAP_PWR_SIZEOF	0x10

[PATCH v5 2/2] Export Power Budgeting Extended Capability into pci-bus-sysfs

[Kobayashi,Daisuke]

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.

Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 Documentation/ABI/testing/sysfs-bus-pci |  62 ++++++++
 drivers/pci/pci-sysfs.c                 | 179 ++++++++++++++++++++++++
 2 files changed, 241 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 7f63c7e97773..ec417ae20bc1 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -572,3 +572,65 @@ Description:
 		enclosure-specific indications "specific0" to "specific7",
 		hence the corresponding led class devices are unavailable if
 		the DSM interface is used.
+
+What:		/sys/bus/pci/devices/.../power_budget
+Date:		December 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.
+
+What:		/sys/bus/pci/devices/.../power_budget/power_budget_data_select
+Date:		December 2024
+Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
+Description:
+		This is an 8-bit read/write register that selects the DWORD of 
+		power budgeting data that will be displayed in the
+		Power Budgeting Data. The value starts at zero and incrementing
+		the index value selects the next DWORD.
+
+What:		/sys/bus/pci/devices/.../power_budget/power_budget_power
+Date:		December 2024
+Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
+Description:
+		This file provides the power consumption calculated by
+		multiplying the base power by the data scale.
+
+
+What:		/sys/bus/pci/devices/.../power_budget/power_budget_pm_state
+Date:		December 2024
+Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
+Description:
+		This file specifies the power management state of the operating
+		condition.
+
+What:		/sys/bus/pci/devices/.../power_budget/power_budget_pm_substate
+Date:		December 2024
+Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
+Description:
+		This file specifies the power management sub state of the
+		operating condition.
+
+What:		/sys/bus/pci/devices/.../power_budget/power_budget_type
+Date:		December 2024
+Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
+Description:
+		This file specifies the type of the operating condition.
+
+
+What:		/sys/bus/pci/devices/.../power_budget/power_budget_rail
+Date:		December 2024
+Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
+Description:
+		This file Specifies the thermal load or power rail of the
+		operating condition.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5d0f4db1cab7..89909633ad02 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -238,6 +238,155 @@ static ssize_t current_link_width_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(current_link_width);
 
+static ssize_t power_budget_data_select_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	int pos;
+	u8 val;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	if (kstrtou8(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	pci_write_config_byte(pci_dev, pos + PCI_PWR_DSR, val);
+
+	return count;
+}
+
+static ssize_t power_budget_data_select_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	int pos, err;
+	u8 data;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	err = pci_read_config_byte(pci_dev, pos + PCI_PWR_DSR, &data);
+	if (err)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%u\n", data);
+}
+
+static DEVICE_ATTR_RW(power_budget_data_select);
+
+static ssize_t power_budget_power_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	int pos, err;
+	u32 data;
+	u8 base, scale_up, scale_low, scale;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
+	if (err)
+		return -EINVAL;
+
+	base = PCI_PWR_DATA_BASE(data);
+	scale_up = PCI_PWR_DATA_SCALE_UP(data);
+	scale_low = PCI_PWR_DATA_SCALE(data);
+	scale = scale_up << 2 | scale_low;
+	if (scale == 0 && base >= 0xF0)
+		return sysfs_emit(buf, "%s\n",pci_power_budget_alt_encode_string(data));
+
+	return sysfs_emit(buf, "%u%s\n", base, pci_power_budget_scale_string(scale));
+}
+static DEVICE_ATTR_RO(power_budget_power);
+
+static ssize_t power_budget_pm_state_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	int pos, err;
+	u32 data;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
+	if (err)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "D%u\n", PCI_PWR_DATA_PM_STATE(data));
+}
+static DEVICE_ATTR_RO(power_budget_pm_state);
+
+static ssize_t power_budget_pm_substate_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	int pos, err;
+	u8 substate;
+	u32 data;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
+	if (err)
+		return -EINVAL;
+	
+	substate = PCI_PWR_DATA_PM_SUB(data);
+	if (substate == 0)
+		return sysfs_emit(buf, "Default Sub State\n");
+
+	return sysfs_emit(buf, "Device Specific Sub State\n");
+}
+static DEVICE_ATTR_RO(power_budget_pm_substate);
+
+static ssize_t power_budget_type_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	int pos, err;
+	u32 data;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
+	if (err)
+		return -EINVAL;
+	
+	return sysfs_emit(buf, "%u\n", PCI_PWR_DATA_TYPE(data));
+}
+static DEVICE_ATTR_RO(power_budget_type);
+
+static ssize_t power_budget_rail_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	int pos, err;
+	u32 data;
+
+	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
+	if (!pos)
+		return -EINVAL;
+
+	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
+	if (err)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%s\n", 
+				pci_power_budget_rail_string(PCI_PWR_DATA_RAIL(data)));
+}
+static DEVICE_ATTR_RO(power_budget_rail);
+
 static ssize_t secondary_bus_number_show(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
@@ -636,6 +785,16 @@ static struct attribute *pcie_dev_attrs[] = {
 	NULL,
 };
 
+static struct attribute *pcie_pbec_attrs[] = {
+	&dev_attr_power_budget_data_select.attr,
+	&dev_attr_power_budget_power.attr,
+	&dev_attr_power_budget_pm_state.attr,
+	&dev_attr_power_budget_pm_substate.attr,
+	&dev_attr_power_budget_rail.attr,
+	&dev_attr_power_budget_type.attr,
+	NULL,
+};
+
 static struct attribute *pcibus_attrs[] = {
 	&dev_attr_bus_rescan.attr,
 	&dev_attr_cpuaffinity.attr,
@@ -1610,6 +1769,19 @@ static umode_t pcie_dev_attrs_are_visible(struct kobject *kobj,
 	return 0;
 }
 
+static umode_t pcie_pbec_attrs_are_visible(struct kobject *kobj,
+					  struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (pci_is_pcie(pdev) && 
+		pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PWR))
+		return a->mode;
+
+	return 0;
+}
+
 static const struct attribute_group pci_dev_group = {
 	.attrs = pci_dev_attrs,
 };
@@ -1652,6 +1824,12 @@ static const struct attribute_group pcie_dev_attr_group = {
 	.is_visible = pcie_dev_attrs_are_visible,
 };
 
+static const struct attribute_group pcie_pbec_attr_group = {
+	.name = "power_budget",
+	.attrs = pcie_pbec_attrs,
+	.is_visible = pcie_pbec_attrs_are_visible,
+};
+
 const struct attribute_group *pci_dev_attr_groups[] = {
 	&pci_dev_attr_group,
 	&pci_dev_hp_attr_group,
@@ -1661,6 +1839,7 @@ const struct attribute_group *pci_dev_attr_groups[] = {
 #endif
 	&pci_bridge_attr_group,
 	&pcie_dev_attr_group,
+	&pcie_pbec_attr_group,
 #ifdef CONFIG_PCIEAER
 	&aer_stats_attr_group,
 #endif
-- 
2.45.0

Re: [PATCH v5 2/2] Export Power Budgeting Extended Capability into pci-bus-sysfs

[Jonathan Cameron]

On Tue, 10 Dec 2024 13:08:21 +0900
"Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:

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

I'm curious why you chose to drive the interface from sysfs as opposed to just
querying how many there are boot and providing separate files for each data
register, potentially a set of files for each one for better readability.

Slightly irritating is that there is no 'count' register so you'd have to
walk up or bisect just to find out how many there actually readable.
Then use is_visible() to hide the remainder.

Look like it is an 8 bit data select register, so maybe 256 max?
That would give you things like 
power_budget0_power
power_budget0_pm_state 
etc
Or maybe drop the leading power given where we are so
budget0_power etc

Mailboxes via sysfs are always a bit messy as there isn't really any
locking etc.

Sorry I'm coming in rather late with this query given you are already at v5!

Jonathan

> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  62 ++++++++
>  drivers/pci/pci-sysfs.c                 | 179 ++++++++++++++++++++++++
>  2 files changed, 241 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 7f63c7e97773..ec417ae20bc1 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -572,3 +572,65 @@ Description:
>  		enclosure-specific indications "specific0" to "specific7",
>  		hence the corresponding led class devices are unavailable if
>  		the DSM interface is used.
> +
> +What:		/sys/bus/pci/devices/.../power_budget
> +Date:		December 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.
> +
> +What:		/sys/bus/pci/devices/.../power_budget/power_budget_data_select
> +Date:		December 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This is an 8-bit read/write register that selects the DWORD of 
> +		power budgeting data that will be displayed in the
> +		Power Budgeting Data. The value starts at zero and incrementing
> +		the index value selects the next DWORD.
> +
> +What:		/sys/bus/pci/devices/.../power_budget/power_budget_power
> +Date:		December 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file provides the power consumption calculated by
> +		multiplying the base power by the data scale.
> +
> +
> +What:		/sys/bus/pci/devices/.../power_budget/power_budget_pm_state
> +Date:		December 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file specifies the power management state of the operating
> +		condition.
> +
> +What:		/sys/bus/pci/devices/.../power_budget/power_budget_pm_substate
> +Date:		December 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file specifies the power management sub state of the
> +		operating condition.
> +
> +What:		/sys/bus/pci/devices/.../power_budget/power_budget_type
> +Date:		December 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file specifies the type of the operating condition.
> +
> +
> +What:		/sys/bus/pci/devices/.../power_budget/power_budget_rail
> +Date:		December 2024
> +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> +Description:
> +		This file Specifies the thermal load or power rail of the
> +		operating condition.
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5d0f4db1cab7..89909633ad02 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -238,6 +238,155 @@ static ssize_t current_link_width_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(current_link_width);

> +
> +static ssize_t power_budget_rail_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +	int pos, err;
> +	u32 data;
> +
> +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);

IIRC, this is not a cheap operation. I'd suggest storing it like we do for aer_cap
etc.

> +	if (!pos)
> +		return -EINVAL;
> +
> +	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
> +	if (err)
> +		return -EINVAL;
> +
> +	return sysfs_emit(buf, "%s\n", 
> +				pci_power_budget_rail_string(PCI_PWR_DATA_RAIL(data)));

Unusual indenting. Align it below b

> +}
> +static DEVICE_ATTR_RO(power_budget_rail);
> +
>  static ssize_t secondary_bus_number_show(struct device *dev,
>  					 struct device_attribute *attr,
>  					 char *buf)
> @@ -636,6 +785,16 @@ static struct attribute *pcie_dev_attrs[] = {
>  	NULL,
>  };
>  
> +static struct attribute *pcie_pbec_attrs[] = {
> +	&dev_attr_power_budget_data_select.attr,
> +	&dev_attr_power_budget_power.attr,
> +	&dev_attr_power_budget_pm_state.attr,
> +	&dev_attr_power_budget_pm_substate.attr,
> +	&dev_attr_power_budget_rail.attr,
> +	&dev_attr_power_budget_type.attr,
> +	NULL,
No need for comma on terminating entries as we will never add anything after them.

> +};

RE: [PATCH v5 2/2] Export Power Budgeting Extended Capability into pci-bus-sysfs

[Daisuke Kobayashi (Fujitsu)]

Jonathan Cameron wrote:
> On Tue, 10 Dec 2024 13:08:21 +0900
> "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:
> 
> > 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.
> 
> I'm curious why you chose to drive the interface from sysfs as opposed to just
> querying how many there are boot and providing separate files for each data
> register, potentially a set of files for each one for better readability.
> 
> Slightly irritating is that there is no 'count' register so you'd have to walk up or
> bisect just to find out how many there actually readable.
> Then use is_visible() to hide the remainder.
> 
> Look like it is an 8 bit data select register, so maybe 256 max?
> That would give you things like
> power_budget0_power
> power_budget0_pm_state
> etc
> Or maybe drop the leading power given where we are so budget0_power etc
> 
> Mailboxes via sysfs are always a bit messy as there isn't really any locking etc.
> 
> Sorry I'm coming in rather late with this query given you are already at v5!
> 
> Jonathan
> 
Thank you for your valuable feedback and for taking the time to review my patch.

Regarding your proposed file structure:

budget0_power
budget0_pm_state
...
budget1_power
budget1_pm_state
...
budget2_xxx
...

Is this the correct format?

In my understanding, implementing this would require 256 attributes corresponding to
the DataSelectRegister, which seems overly redundant. Additionally, I was concerned
about the mechanism of changing the DataSelectRegister each time a file is read.

Therefore, I proposed this patch implementation, even though it might not be ideal.

If you have a suitable implementation to achieve your proposed structure, 
I would greatly appreciate it if you could share it.

Upon further reflection, the issue of needing to change the DataSelectRegister could 
potentially be avoided by initially retrieving and storing the register values for all indices.

Thank you for pointing out the coding style issue, I'll fix it in the next patch.

> >
> > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-pci |  62 ++++++++
> >  drivers/pci/pci-sysfs.c                 | 179
> ++++++++++++++++++++++++
> >  2 files changed, 241 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> > b/Documentation/ABI/testing/sysfs-bus-pci
> > index 7f63c7e97773..ec417ae20bc1 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -572,3 +572,65 @@ Description:
> >  		enclosure-specific indications "specific0" to "specific7",
> >  		hence the corresponding led class devices are unavailable if
> >  		the DSM interface is used.
> > +
> > +What:		/sys/bus/pci/devices/.../power_budget
> > +Date:		December 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.
> > +
> > +What:
> 	/sys/bus/pci/devices/.../power_budget/power_budget_data_select
> > +Date:		December 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This is an 8-bit read/write register that selects the DWORD of
> > +		power budgeting data that will be displayed in the
> > +		Power Budgeting Data. The value starts at zero and
> incrementing
> > +		the index value selects the next DWORD.
> > +
> > +What:
> 	/sys/bus/pci/devices/.../power_budget/power_budget_power
> > +Date:		December 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This file provides the power consumption calculated by
> > +		multiplying the base power by the data scale.
> > +
> > +
> > +What:
> 	/sys/bus/pci/devices/.../power_budget/power_budget_pm_state
> > +Date:		December 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This file specifies the power management state of the
> operating
> > +		condition.
> > +
> > +What:
> 	/sys/bus/pci/devices/.../power_budget/power_budget_pm_substat
> e
> > +Date:		December 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This file specifies the power management sub state of the
> > +		operating condition.
> > +
> > +What:
> 	/sys/bus/pci/devices/.../power_budget/power_budget_type
> > +Date:		December 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This file specifies the type of the operating condition.
> > +
> > +
> > +What:
> 	/sys/bus/pci/devices/.../power_budget/power_budget_rail
> > +Date:		December 2024
> > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > +Description:
> > +		This file Specifies the thermal load or power rail of the
> > +		operating condition.
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index
> > 5d0f4db1cab7..89909633ad02 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -238,6 +238,155 @@ static ssize_t current_link_width_show(struct
> > device *dev,  }  static DEVICE_ATTR_RO(current_link_width);
> 
> > +
> > +static ssize_t power_budget_rail_show(struct device *dev,
> > +				       struct device_attribute *attr, char
> *buf) {
> > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > +	int pos, err;
> > +	u32 data;
> > +
> > +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> 
> IIRC, this is not a cheap operation. I'd suggest storing it like we do for aer_cap
> etc.
> 
> > +	if (!pos)
> > +		return -EINVAL;
> > +
> > +	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
> > +	if (err)
> > +		return -EINVAL;
> > +
> > +	return sysfs_emit(buf, "%s\n",
> > +
> 	pci_power_budget_rail_string(PCI_PWR_DATA_RAIL(data)));
> 
> Unusual indenting. Align it below b
> 
> > +}
> > +static DEVICE_ATTR_RO(power_budget_rail);
> > +
> >  static ssize_t secondary_bus_number_show(struct device *dev,
> >  					 struct device_attribute *attr,
> >  					 char *buf)
> > @@ -636,6 +785,16 @@ static struct attribute *pcie_dev_attrs[] = {
> >  	NULL,
> >  };
> >
> > +static struct attribute *pcie_pbec_attrs[] = {
> > +	&dev_attr_power_budget_data_select.attr,
> > +	&dev_attr_power_budget_power.attr,
> > +	&dev_attr_power_budget_pm_state.attr,
> > +	&dev_attr_power_budget_pm_substate.attr,
> > +	&dev_attr_power_budget_rail.attr,
> > +	&dev_attr_power_budget_type.attr,
> > +	NULL,
> No need for comma on terminating entries as we will never add anything after
> them.
> 
> > +};

Re: [PATCH v5 2/2] Export Power Budgeting Extended Capability into pci-bus-sysfs

[Jonathan Cameron]

On Thu, 12 Dec 2024 09:08:54 +0000
"Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@fujitsu.com> wrote:

> Jonathan Cameron wrote:
> > On Tue, 10 Dec 2024 13:08:21 +0900
> > "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:
> >   
> > > 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.  
> > 
> > I'm curious why you chose to drive the interface from sysfs as opposed to just
> > querying how many there are boot and providing separate files for each data
> > register, potentially a set of files for each one for better readability.
> > 
> > Slightly irritating is that there is no 'count' register so you'd have to walk up or
> > bisect just to find out how many there actually readable.
> > Then use is_visible() to hide the remainder.
> > 
> > Look like it is an 8 bit data select register, so maybe 256 max?
> > That would give you things like
> > power_budget0_power
> > power_budget0_pm_state
> > etc
> > Or maybe drop the leading power given where we are so budget0_power etc
> > 
> > Mailboxes via sysfs are always a bit messy as there isn't really any locking etc.
> > 
> > Sorry I'm coming in rather late with this query given you are already at v5!
> > 
> > Jonathan
> >   
> Thank you for your valuable feedback and for taking the time to review my patch.
> 
> Regarding your proposed file structure:
> 
> budget0_power
> budget0_pm_state
> ...
> budget1_power
> budget1_pm_state
> ...
> budget2_xxx
> ...
> 
> Is this the correct format?

Yes. That is what I had mind.

> 
> In my understanding, implementing this would require 256 attributes corresponding to
> the DataSelectRegister, which seems overly redundant. Additionally, I was concerned
> about the mechanism of changing the DataSelectRegister each time a file is read.

I agree it is quite a large number of files, but there are larger sysfs interfaces
so I'm not that concerned. Do you have any data on how many entries are typically used?
We could use an is_visible() callback and register all 256, or we could do dynamic
allocation though that would require walking to find out what is there.

The implementation should cache the current dataselectregister value and only write it
if the value needs to change.  Sensible userspace software would just read all
the elements of each budgetX_* before moving on to the next one.

> 
> Therefore, I proposed this patch implementation, even though it might not be ideal.
> 
> If you have a suitable implementation to achieve your proposed structure, 
> I would greatly appreciate it if you could share it.
> 
> Upon further reflection, the issue of needing to change the DataSelectRegister could 
> potentially be avoided by initially retrieving and storing the register values for all indices.

Cacheing these is definitely an option to explore.  I'm not sure we would want to pay that cost
at boot, but if not we could do it on first touch.  The 0 value is reserved anyway so
if cached copy is 0, read it from the device and fill the cache.
We would need to read some at boot to figure out which should be visible, but
that would be logN of max entries at worse, rather than all 256.  If there are only
a few it would make sense to just read them all at boot.

Annoying there isn't a count register for these!

Jonathan




> 
> Thank you for pointing out the coding style issue, I'll fix it in the next patch.
> 
> > >
> > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-pci |  62 ++++++++
> > >  drivers/pci/pci-sysfs.c                 | 179  
> > ++++++++++++++++++++++++  
> > >  2 files changed, 241 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci
> > > b/Documentation/ABI/testing/sysfs-bus-pci
> > > index 7f63c7e97773..ec417ae20bc1 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > > @@ -572,3 +572,65 @@ Description:
> > >  		enclosure-specific indications "specific0" to "specific7",
> > >  		hence the corresponding led class devices are unavailable if
> > >  		the DSM interface is used.
> > > +
> > > +What:		/sys/bus/pci/devices/.../power_budget
> > > +Date:		December 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.
> > > +
> > > +What:  
> > 	/sys/bus/pci/devices/.../power_budget/power_budget_data_select  
> > > +Date:		December 2024
> > > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > > +Description:
> > > +		This is an 8-bit read/write register that selects the DWORD of
> > > +		power budgeting data that will be displayed in the
> > > +		Power Budgeting Data. The value starts at zero and  
> > incrementing  
> > > +		the index value selects the next DWORD.
> > > +
> > > +What:  
> > 	/sys/bus/pci/devices/.../power_budget/power_budget_power  
> > > +Date:		December 2024
> > > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > > +Description:
> > > +		This file provides the power consumption calculated by
> > > +		multiplying the base power by the data scale.
> > > +
> > > +
> > > +What:  
> > 	/sys/bus/pci/devices/.../power_budget/power_budget_pm_state  
> > > +Date:		December 2024
> > > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > > +Description:
> > > +		This file specifies the power management state of the  
> > operating  
> > > +		condition.
> > > +
> > > +What:  
> > 	/sys/bus/pci/devices/.../power_budget/power_budget_pm_substat
> > e  
> > > +Date:		December 2024
> > > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > > +Description:
> > > +		This file specifies the power management sub state of the
> > > +		operating condition.
> > > +
> > > +What:  
> > 	/sys/bus/pci/devices/.../power_budget/power_budget_type  
> > > +Date:		December 2024
> > > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > > +Description:
> > > +		This file specifies the type of the operating condition.
> > > +
> > > +
> > > +What:  
> > 	/sys/bus/pci/devices/.../power_budget/power_budget_rail  
> > > +Date:		December 2024
> > > +Contact:	Kobayashi Daisuke <kobayashi.da-06@fujitsu.com>
> > > +Description:
> > > +		This file Specifies the thermal load or power rail of the
> > > +		operating condition.
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index
> > > 5d0f4db1cab7..89909633ad02 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -238,6 +238,155 @@ static ssize_t current_link_width_show(struct
> > > device *dev,  }  static DEVICE_ATTR_RO(current_link_width);  
> >   
> > > +
> > > +static ssize_t power_budget_rail_show(struct device *dev,
> > > +				       struct device_attribute *attr, char  
> > *buf) {  
> > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > +	int pos, err;
> > > +	u32 data;
> > > +
> > > +	pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);  
> > 
> > IIRC, this is not a cheap operation. I'd suggest storing it like we do for aer_cap
> > etc.
> >   
> > > +	if (!pos)
> > > +		return -EINVAL;
> > > +
> > > +	err = pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA, &data);
> > > +	if (err)
> > > +		return -EINVAL;
> > > +
> > > +	return sysfs_emit(buf, "%s\n",
> > > +  
> > 	pci_power_budget_rail_string(PCI_PWR_DATA_RAIL(data)));
> > 
> > Unusual indenting. Align it below b
> >   
> > > +}
> > > +static DEVICE_ATTR_RO(power_budget_rail);
> > > +
> > >  static ssize_t secondary_bus_number_show(struct device *dev,
> > >  					 struct device_attribute *attr,
> > >  					 char *buf)
> > > @@ -636,6 +785,16 @@ static struct attribute *pcie_dev_attrs[] = {
> > >  	NULL,
> > >  };
> > >
> > > +static struct attribute *pcie_pbec_attrs[] = {
> > > +	&dev_attr_power_budget_data_select.attr,
> > > +	&dev_attr_power_budget_power.attr,
> > > +	&dev_attr_power_budget_pm_state.attr,
> > > +	&dev_attr_power_budget_pm_substate.attr,
> > > +	&dev_attr_power_budget_rail.attr,
> > > +	&dev_attr_power_budget_type.attr,
> > > +	NULL,  
> > No need for comma on terminating entries as we will never add anything after
> > them.
> >   
> > > +};  
> 

RE: [PATCH v5 2/2] Export Power Budgeting Extended Capability into pci-bus-sysfs

[Daisuke Kobayashi (Fujitsu)]

Thank you for your detailed feedback and suggestions. Based on your comments,
I have a few thoughts and questions to ensure we move forward effectively.

Jonathan Cameron wrote:
> On Thu, 12 Dec 2024 09:08:54 +0000
> "Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@fujitsu.com> wrote:
> 
> > Jonathan Cameron wrote:
> > > On Tue, 10 Dec 2024 13:08:21 +0900
> > > "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:

... snip ...
> > Regarding your proposed file structure:
> >
> > budget0_power
> > budget0_pm_state
> > ...
> > budget1_power
> > budget1_pm_state
> > ...
> > budget2_xxx
> > ...
> >
> > Is this the correct format?
> 
> Yes. That is what I had mind.
> 

Thanks, I see.

> >
> > In my understanding, implementing this would require 256 attributes
> > corresponding to the DataSelectRegister, which seems overly redundant.
> > Additionally, I was concerned about the mechanism of changing the
> DataSelectRegister each time a file is read.
> 
> I agree it is quite a large number of files, but there are larger sysfs interfaces so
> I'm not that concerned. Do you have any data on how many entries are typically
> used?
> We could use an is_visible() callback and register all 256, or we could do
> dynamic allocation though that would require walking to find out what is there.
> 
> The implementation should cache the current dataselectregister value and only
> write it if the value needs to change.  Sensible userspace software would just
> read all the elements of each budgetX_* before moving on to the next one.
> 

Unfortunately, I don't have specific data on the typical number of entries used.
However, I estimate that it would be within 10 at most.
Could you please provide examples of large sysfs interfaces and where they are
implemented? I would like to refer to them for implementation.
In pci-sysfs.c, there is an attribute called resourceX_resize, and I believe its basic
concept could be useful. However, the PowerBudget implementation is more complex
because it involves multiple sysfs outputs for each index.
If there are more reference implementations, I could propose a more mature implementation.

> >
> > Therefore, I proposed this patch implementation, even though it might not be
> ideal.
> >
> > If you have a suitable implementation to achieve your proposed
> > structure, I would greatly appreciate it if you could share it.
> >
> > Upon further reflection, the issue of needing to change the
> > DataSelectRegister could potentially be avoided by initially retrieving and
> storing the register values for all indices.
> 
> Cacheing these is definitely an option to explore.  I'm not sure we would want
> to pay that cost at boot, but if not we could do it on first touch.  The 0 value is
> reserved anyway so if cached copy is 0, read it from the device and fill the
> cache.
> We would need to read some at boot to figure out which should be visible, but
> that would be logN of max entries at worse, rather than all 256.  If there are only
> a few it would make sense to just read them all at boot.
> 
Although the exact number of indices is uncertain, I believe it is sufficiently small. 
Therefore, I plan to adopt a method where all values are read and cached at boot time. 
I intend to create an array in the struct pci_dev and initialize it in the probe function.
If there are any other suitable places or methods, please let me know.

> Annoying there isn't a count register for these!

I also feel that. Thank you.
> 
> Jonathan
> 
> 
> 
> 
> >
> > Thank you for pointing out the coding style issue, I'll fix it in the next patch.
> >
> > > >
> > > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
... snip ...

RE: [PATCH v5 2/2] Export Power Budgeting Extended Capability into pci-bus-sysfs

[Daisuke Kobayashi (Fujitsu)]

Gentle ping. Could you check this patch?

> Thank you for your detailed feedback and suggestions. Based on your
> comments, I have a few thoughts and questions to ensure we move forward
> effectively.
> 
> Jonathan Cameron wrote:
> > On Thu, 12 Dec 2024 09:08:54 +0000
> > "Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@fujitsu.com> wrote:
> >
> > > Jonathan Cameron wrote:
> > > > On Tue, 10 Dec 2024 13:08:21 +0900 "Kobayashi,Daisuke"
> > > > <kobayashi.da-06@fujitsu.com> wrote:
> 
> ... snip ...
> > > Regarding your proposed file structure:
> > >
> > > budget0_power
> > > budget0_pm_state
> > > ...
> > > budget1_power
> > > budget1_pm_state
> > > ...
> > > budget2_xxx
> > > ...
> > >
> > > Is this the correct format?
> >
> > Yes. That is what I had mind.
> >
> 
> Thanks, I see.
> 
> > >
> > > In my understanding, implementing this would require 256 attributes
> > > corresponding to the DataSelectRegister, which seems overly redundant.
> > > Additionally, I was concerned about the mechanism of changing the
> > DataSelectRegister each time a file is read.
> >
> > I agree it is quite a large number of files, but there are larger
> > sysfs interfaces so I'm not that concerned. Do you have any data on
> > how many entries are typically used?
> > We could use an is_visible() callback and register all 256, or we
> > could do dynamic allocation though that would require walking to find out
> what is there.
> >
> > The implementation should cache the current dataselectregister value
> > and only write it if the value needs to change.  Sensible userspace
> > software would just read all the elements of each budgetX_* before moving
> on to the next one.
> >
> 
> Unfortunately, I don't have specific data on the typical number of entries used.
> However, I estimate that it would be within 10 at most.
> Could you please provide examples of large sysfs interfaces and where they are
> implemented? I would like to refer to them for implementation.
> In pci-sysfs.c, there is an attribute called resourceX_resize, and I believe its
> basic concept could be useful. However, the PowerBudget implementation is
> more complex because it involves multiple sysfs outputs for each index.
> If there are more reference implementations, I could propose a more mature
> implementation.
> 
> > >
> > > Therefore, I proposed this patch implementation, even though it
> > > might not be
> > ideal.
> > >
> > > If you have a suitable implementation to achieve your proposed
> > > structure, I would greatly appreciate it if you could share it.
> > >
> > > Upon further reflection, the issue of needing to change the
> > > DataSelectRegister could potentially be avoided by initially
> > > retrieving and
> > storing the register values for all indices.
> >
> > Cacheing these is definitely an option to explore.  I'm not sure we
> > would want to pay that cost at boot, but if not we could do it on
> > first touch.  The 0 value is reserved anyway so if cached copy is 0,
> > read it from the device and fill the cache.
> > We would need to read some at boot to figure out which should be
> > visible, but that would be logN of max entries at worse, rather than
> > all 256.  If there are only a few it would make sense to just read them all at
> boot.
> >
> Although the exact number of indices is uncertain, I believe it is sufficiently
> small.
> Therefore, I plan to adopt a method where all values are read and cached at
> boot time.
> I intend to create an array in the struct pci_dev and initialize it in the probe
> function.
> If there are any other suitable places or methods, please let me know.
> 
> > Annoying there isn't a count register for these!
> 
> I also feel that. Thank you.
> >
> > Jonathan
> >
> >
> >
> >
> > >
> > > Thank you for pointing out the coding style issue, I'll fix it in the next patch.
> > >
> > > > >
> > > > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ... snip ...