Jonathan Cameron wrote:
> On Wed, 11 Sep 2024 10:20:53 +0900
> "Kobayashi,Daisuke" <kobayashi.da-06@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> 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@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.
>
> 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,
> > };
> >
