Messages in this thread | | | Date | Thu, 10 Aug 2023 07:05:12 +0200 | From | Greg KH <> | Subject | Re: [PATCH v3] PCI/DOE: Expose the DOE protocols via sysfs |
| |
On Wed, Aug 09, 2023 at 07:28:51PM -0400, Alistair Francis wrote: > The PCIe 6 specification added support for the Data Object Exchange (DOE). > When DOE is supported the Discovery Data Object Protocol must be > implemented. The protocol allows a requester to obtain information about > the other DOE protocols supported by the device. > > The kernel is already querying the DOE protocols supported and cacheing > the values. This patch exposes the values via sysfs. This will allow > userspace to determine which DOE protocols are supported by the PCIe > device. > > By exposing the information to userspace tools like lspci can relay the > information to users. By listing all of the supported protocols we can > allow userspace to parse and support the list, which might include > vendor specific protocols as well as yet to be supported protocols. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > v3: > - Expose each DOE feature as a separate file
But you don't actually have anything in the sysfs files, why not?
> --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -56,6 +56,10 @@ struct pci_doe_mb { > wait_queue_head_t wq; > struct workqueue_struct *work_queue; > unsigned long flags; > + > +#ifdef CONFIG_SYSFS > + struct device_attribute *sysfs_attrs; > +#endif
Please don't put #ifdefs in .c files if you can prevent it. I think this will work just fine if you don't have the #ifdef. And who would be using pci without sysfs?
> + attrs[i].attr.mode = 0444; > + attrs[i].show = NULL;
Why set to NULL something that is already NULL? Did you just forget to actually set the proper show callback here?
> +#ifdef CONFIG_PCI_DOE > + retval = doe_sysfs_init(pdev); > + if (retval) > + return retval; > +#endif
Again, no #ifdef in .c files please, put this in the .h file like normal.
thanks,
greg k-h
| |