Krzysztof Wilczyński <kw@xxxxxxxxx> wrote:
> Hello,
>
> [...]
> > PCIe devices can significantly impact the overall power consumption of
> > a system. However, obtaining PCIe device power consumption information
> > has traditionally been difficult. This is because the 'lspci' command,
> > which is a standard tool for displaying information about PCI devices,
> > cannot access PBEC information. `lspci` is a standard tool for displaying
> > information about PCI devices.
>
> Will you also be making changes to the pciutils project to expose this
> using the lspci utility?
>
Yes, I'm considering making changes to the pciutils project to expose this
information using the lspci utility.
> That said, we already expose "config" sysfs object, would using it work to
> obtain the data you need?
>
I considered reading from a config file, but since this requires writing to
configuration space, I feel it would be better to implement it in a kernel driver.
> > +static ssize_t pbec_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct pci_dev *pci_dev = to_pci_dev(dev);
> > + size_t len = 0;
> > + int i, pos;
> > + u32 data;
> > +
> > + if (!pci_is_pcie(pci_dev))
> > + return 0;
>
> This is not needed, I believe the "is_visible" callback for this specific
> attributes group checks for this already.
>
> > + pos = pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_PWR);
> > + if (!pos)
> > + return 0;
>
> It would be -EINVAL, customarily. I suppose, the -ENOTSUPP or -ENOSYS
> could
> do too, but most of the other PCI sysfs objects returns -EINVAL back to the
> userspace to indicate that something is wrong.
>
> > + for (i = 0; i < 0xff; i++) {
>
> Does this 0xff need to be documented? Something specific to the PBEC
> feature?
>
This assumes the maximum value of the index, but the maximum value is not
stated in the specifications. Since DSR is an 8-bit register, the maximum value
is assumed to be 0xff and set as the loop count.
Would it be better to define it as a constant?
> > + pci_write_config_byte(pci_dev, pos + PCI_PWR_DSR, (u8)i);
> > + pci_read_config_dword(pci_dev, pos + PCI_PWR_DATA,
> &data);
> > + if (!data)
> > + break;
>
> We should return an error here, something like -EINVAL perhaps.
>
> > + len += sysfs_emit_at(buf, len, "%04x\n", data);
> > + }
> > +
> > + return len;
>
Thank you for your review.
I will fix any errors or unnecessary processing in the next patch.
> How frequently do you think this new sysfs object would be accessed? It's
> not uncommon for the PCI configuration space access to be slow for some
> devices.
>
My current assumption is that it is accessed when lspci is run, so I don't think
it is accessed very often.
> Thank you!
>
> Krzysztof
Thank you!
