On Tue, Jul 2, 2024 at 11:58 PM Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote: > > On Tue, 2 Jul 2024 16:04:17 +1000 > Alistair Francis <alistair23@xxxxxxxxx> wrote: > > > The PCIe 6 specification added support for the Data Object > > Exchange (DOE). > > When DOE is supported the DOE Discovery Feature must be implemented per > > PCIe r6.1 sec 6.30.1.1. The protocol allows a requester to obtain > > information about the other DOE features supported by the device. > > > > The kernel is already querying the DOE features supported and cacheing > > the values. Expose the values in sysfs to allow user space to > > determine which DOE features 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 features we can > > allow userspace to parse the list, which might include > > vendor specific features as well as yet to be supported features. > > > > After this patch is supported you can see something like this when > > attaching a DOE device > > > > $ ls /sys/devices/pci0000:00/0000:00:02.0//doe* > > 0001:00 0001:01 0001:02 > > > > Signed-off-by: Alistair Francis <alistair.francis@xxxxxxx> > > --- > > v13: > > - Drop pci_doe_sysfs_init() and use pci_doe_sysfs_group > > - As discussed in https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/ > > we can just modify pci_doe_sysfs_group at the DOE init and let > > Can't do that as it is global so you expose the same DOE features for > all DOEs. > > Also, I think that this is only processing features on last doe_mb found > for a given device. Fix that and the duplicates problem resurfaces. > > > > device_add() handle the sysfs attributes. > > > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > > index defc4be81bd4..e7b702afce88 100644 > > --- a/drivers/pci/doe.c > > +++ b/drivers/pci/doe.c > > > + > > static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout) > > { > > if (wait_event_timeout(doe_mb->wq, > > @@ -687,6 +747,12 @@ void pci_doe_init(struct pci_dev *pdev) > > { > > struct pci_doe_mb *doe_mb; > > u16 offset = 0; > > + struct attribute **sysfs_attrs; > > + struct device_attribute *attrs; > > + unsigned long num_features = 0; > > + unsigned long i; > > + unsigned long vid, type; > > + void *entry; > > int rc; > > > > xa_init(&pdev->doe_mbs); > > @@ -707,6 +773,45 @@ void pci_doe_init(struct pci_dev *pdev) > > pci_doe_destroy_mb(doe_mb); > > } > > } > > The above is looping over multiple DOEs but this just considers last one. > That doesn't look right... Yeah... That isn't > > I think this needs to be in the loop and having done that > the duplicate handing may be an issue. I'm not sure what happens > in that path with a presupplied set of attributes. > > > + > > + if (doe_mb) { > > + xa_for_each(&doe_mb->feats, i, entry) > > + num_features++; > > + > > + sysfs_attrs = kcalloc(num_features + 1, sizeof(*sysfs_attrs), GFP_KERNEL); > > + if (!sysfs_attrs) > > + return; > > + > > + attrs = kcalloc(num_features, sizeof(*attrs), GFP_KERNEL); > > + if (!attrs) { > > + kfree(sysfs_attrs); > > + return; > > + } > > + > > + doe_mb->device_attrs = attrs; > > + doe_mb->sysfs_attrs = sysfs_attrs; > > + > > + xa_for_each(&doe_mb->feats, i, entry) { > > + sysfs_attr_init(&attrs[i].attr); > > + > > + vid = xa_to_value(entry) >> 8; > > + type = xa_to_value(entry) & 0xFF; > > + > > + attrs[i].attr.name = kasprintf(GFP_KERNEL, "%04lx:%02lx", vid, type); > > + if (!attrs[i].attr.name) { > > + pci_doe_sysfs_feature_remove(pdev, doe_mb); > > + return; > > + } > > + attrs[i].attr.mode = 0444; > > + attrs[i].show = pci_doe_sysfs_feature_show; > > + > > + sysfs_attrs[i] = &attrs[i].attr; > > + } > > + > > + sysfs_attrs[num_features] = NULL; > > + > > + pci_doe_sysfs_group.attrs = sysfs_attrs; > Hmm. Isn't this global? What if you have multiple devices. Any input from a PCI maintainer here? There are basically two approaches. 1. We can have a pci_doe_sysfs_init() function that is called where we dynamically add the entries, like in v12 2. We can go down the dev->groups and device_add() path, like this patch and discussed at https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/ For the second we will have to create a global pci_doe_sysfs_group that contains all possible DOE entries on the system and then have the show functions determine if they should be displayed for that device. Everytime we call pci_doe_init() we can check for any missing entries in pci_doe_sysfs_group.attrs and then realloc pci_doe_sysfs_group.attrs to add them. Untested, but that should work even for hot-plugged devices. pci_doe_sysfs_group.attrs would just grow forever though as I don't think we have an easy way to deallocate anything as we aren't sure if we are the only entry. Thoughts? Alistair
