Re: [PATCH v11 3/4] PCI/DOE: Expose the DOE features via sysfs

[Lukas Wunner <lukas@wunner.de>]

On Fri, Jun 14, 2024 at 10:12:43AM +1000, Alistair Francis wrote:
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -16,6 +16,7 @@
>  #include <linux/kernel.h>
>  #include <linux/sched.h>
>  #include <linux/pci.h>
> +#include <linux/pci-doe.h>
>  #include <linux/stat.h>
>  #include <linux/export.h>
>  #include <linux/topology.h>

I'm not seeing any symbols used here which are defined in pci-doe.h.
Am I missing something?

If not this additional #include can be dropped.


> @@ -1143,6 +1144,9 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
>  {
>  	int i;
>  
> +	if (IS_ENABLED(CONFIG_PCI_DOE))
> +		pci_doe_sysfs_teardown(pdev);
> +

No need to constrain to "if (IS_ENABLED(CONFIG_PCI_DOE))" as you're
defining an empty static inline in the header file.


> @@ -1227,6 +1231,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;
> +	}
> +

Same here.

Note that pci_{create,remove}_resource_files() is not the right place
to dynamically add sysfs attributes.  These functions are called very
late to postpone exposure of ROM resources until they're enumerated.
You want to add your sysfs attributes right after device_add() has been
called and you want to remove them right before device_del() is called.

See here for an example how it's done correctly:

https://lore.kernel.org/all/20240528131940.16924-3-mariusz.tkaczyk@linux.intel.com/

(Search for the call to pci_npem_create() in pci_device_add() and
pci_npem_remove() in pci_destroy_dev().)


> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -189,6 +189,7 @@ extern const struct attribute_group *pci_dev_groups[];
>  extern const struct attribute_group *pci_dev_attr_groups[];
>  extern const struct attribute_group *pcibus_groups[];
>  extern const struct attribute_group *pci_bus_groups[];
> +extern const struct attribute_group pci_doe_sysfs_group;
>  #else
>  static inline int pci_create_sysfs_dev_files(struct pci_dev *pdev) { return 0; }
>  static inline void pci_remove_sysfs_dev_files(struct pci_dev *pdev) { }
> @@ -196,6 +197,7 @@ static inline void pci_remove_sysfs_dev_files(struct pci_dev *pdev) { }
>  #define pci_dev_attr_groups NULL
>  #define pcibus_groups NULL
>  #define pci_bus_groups NULL
> +#define pci_doe_sysfs_group NULL
>  #endif

You only need the "extern const struct ..." definition, not the
NULL definition.  The reason we have these NULL definitions is
because we're referencing the attribute groups in files which
are compiled even if CONFIG_SYSFS=n.  But I believe that's not
the case here.

Thanks,

Lukas