Hello, I hope this email finds you well.
Please let me know if there is anything I should do to get this feature merged.
I'd appreciate it if you could take a look when you have a chance.
I'm happy to answer any questions you might have.
Thanks.
Kobayashi Daisuke wrote:
> 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!
