On Tue, 10 Dec 2024 13:08:21 +0900
"Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
> +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@xxxxxxxxxxx>
> +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@xxxxxxxxxxx>
> +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@xxxxxxxxxxx>
> +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@xxxxxxxxxxx>
> +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@xxxxxxxxxxx>
> +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@xxxxxxxxxxx>
> +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.
> +};
