On Tue, Oct 17, 2023 at 6:34 PM Lukas Wunner <lukas@xxxxxxxxx> wrote:
>
> On Fri, Oct 13, 2023 at 01:41:57PM +1000, Alistair Francis wrote:
> > +#ifdef CONFIG_SYSFS
> > +static umode_t pci_doe_sysfs_attr_is_visible(struct kobject *kobj,
> > + struct attribute *a, int n)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> > + struct pci_doe_mb *doe_mb;
> > + unsigned long index, j;
> > + void *entry;
> > +
> > + xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > + xa_for_each(&doe_mb->feats, j, entry)
> > + return a->mode;
> > + }
> > +
> > + return 0;
> > +}
>
> Out of curiosity: Does this method prevent creation of a
> "doe_features" directory for devices which don't have any
> DOE mailboxes?
It does once this patch (or something similar) is applied:
https://lkml.org/lkml/2022/8/24/607
GKH and I are working on getting a patch like that working and merged
Alistair
>
> (If it does, a code comment explaining that might be helpful.)
>
>
> > +const struct attribute_group pci_dev_doe_feature_group = {
> > + .name = "doe_features",
> > + .attrs = pci_dev_doe_feature_attrs,
> > + .is_visible = pci_doe_sysfs_attr_is_visible,
> > +};
>
> Nit: Wondering why the "=" is aligned for .name and .attrs
> but not for .is_visible?
>
>
> > +static void pci_doe_sysfs_feature_remove(struct pci_dev *pdev,
> > + struct pci_doe_mb *doe_mb)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_attribute *attrs = doe_mb->sysfs_attrs;
> > + unsigned long i;
> > + void *entry;
>
> Nit: Inverse Christmas tree?
>
>
> > +static int pci_doe_sysfs_feature_populate(struct pci_dev *pdev,
> > + struct pci_doe_mb *doe_mb)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_attribute *attrs;
> > + unsigned long num_features = 0;
> > + unsigned long vid, type;
> > + unsigned long i;
> > + void *entry;
> > + int ret;
> > +
> > + xa_for_each(&doe_mb->feats, i, entry)
> > + num_features++;
> > +
> > + attrs = kcalloc(num_features, sizeof(*attrs), GFP_KERNEL);
> > + if (!attrs)
> > + return -ENOMEM;
> > +
> > + doe_mb->sysfs_attrs = 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,
> > + "0x%04lX:%02lX", vid, type);
>
> Nit: Capital X conversion specifier will result in upper case hex
> characters in filename and contents, whereas existing sysfs attributes
> such as "vendor", "device" contain lower case hex characters.
>
> Might want to consider lower case x for consistency.
>
>
> > +void pci_doe_sysfs_teardown(struct pci_dev *pdev)
> > +{
> > + struct pci_doe_mb *doe_mb;
> > + unsigned long index;
> > +
> > + xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > + pci_doe_sysfs_feature_remove(pdev, doe_mb);
> > + }
>
> Nit: Curly braces not necessary.
>
>
> > @@ -1153,6 +1154,10 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
> > {
> > int i;
> >
> > + if (IS_ENABLED(CONFIG_PCI_DOE)) {
> > + pci_doe_sysfs_teardown(pdev);
> > + }
>
> Nit: Curly braces not necessary.
>
>
> > @@ -1230,6 +1235,12 @@ static int pci_create_resource_files(struct pci_dev *pdev)
> > int i;
> > int retval;
> >
> > + if (IS_ENABLED(CONFIG_PCI_DOE)) {
> > + retval = pci_doe_sysfs_init(pdev);
> > + if (retval)
> > + return retval;
> > + }
> > +
> > /* Expose the PCI resources from this device as files */
> > for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>
> I think this needs to be added to pci_create_sysfs_dev_files() instead.
>
> pci_create_resource_files() only deals with creation of resource files,
> as the name implies, which is unrelated to DOE features.
>
> Worse, pci_create_resource_files() is also called from
> pci_dev_resource_resize_attr(), i.e. every time user space
> writes to the "resource##n##_resize" attributes.
>
> Similarly, the call to pci_doe_sysfs_teardown() belongs in
> pci_remove_sysfs_dev_files().
>
> Thanks,
>
> Lukas
