[PATCH] PCI: create builtin_pci_driver to avoid registration boilerplate

[PATCH] PCI: create builtin_pci_driver to avoid registration boilerplate

[Paul Gortmaker]

In commit f309d4443130bf814e991f836e919dca22df37ae ("platform_device:
better support builtin boilerplate avoidance") we introduced the
builtin_driver macro.

Here we use that support and extend it to PCI driver registration,
so where a driver is clearly non-modular and builtin-only, we can
register it in a similar fashion.  And existing code that is clearly
non-modular can be updated with the simple mapping of

     module_pci_driver(...)  ---> builtin_pci_driver(...)

We've essentially cloned the former to make the latter, and taken
out the remove/module_exit parts since those never get used in a
non-modular build of the code.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 include/linux/pci.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 88bee285b93d..8da2758e7d0e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1187,6 +1187,17 @@ void pci_unregister_driver(struct pci_driver *dev);
 	module_driver(__pci_driver, pci_register_driver, \
 		       pci_unregister_driver)
 
+/**
+ * builtin_pci_driver() - Helper macro for registering a PCI driver
+ * @__pci_driver: pci_driver struct
+ *
+ * Helper macro for PCI drivers which do not do anything special in their
+ * init code. This eliminates a lot of boilerplate. Each driver may only
+ * use this macro once, and calling it replaces device_initcall(...)
+ */
+#define builtin_pci_driver(__pci_driver) \
+	builtin_driver(__pci_driver, pci_register_driver)
+
 struct pci_driver *pci_dev_driver(const struct pci_dev *dev);
 int pci_add_dynid(struct pci_driver *drv,
 		  unsigned int vendor, unsigned int device,
-- 
2.5.0

Re: [PATCH] PCI: create builtin_pci_driver to avoid registration boilerplate

[Bjorn Helgaas]

Hi Paul,

On Tue, Aug 25, 2015 at 08:25:37PM -0400, Paul Gortmaker wrote:
> In commit f309d4443130bf814e991f836e919dca22df37ae ("platform_device:
> better support builtin boilerplate avoidance") we introduced the
> builtin_driver macro.
> 
> Here we use that support and extend it to PCI driver registration,
> so where a driver is clearly non-modular and builtin-only, we can
> register it in a similar fashion.  And existing code that is clearly
> non-modular can be updated with the simple mapping of
> 
>      module_pci_driver(...)  ---> builtin_pci_driver(...)
> 
> We've essentially cloned the former to make the latter, and taken
> out the remove/module_exit parts since those never get used in a
> non-modular build of the code.

Do you have any estimate of how many potential users of this there
are?  I took a quick look at users of module_pci_driver() (I see
almost 300 of them in v4.3-rc1), and most of them look like legitimate
modules.  But the comment mentions replacing device_initcall() as
well, so maybe there are more there?

If only a couple would be converted to builtin_pci_driver(), I'm not
sure it's worth it, because it does add more things to look at
(builtin_pci_driver() in addition to module_pci_driver()).

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  include/linux/pci.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 88bee285b93d..8da2758e7d0e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1187,6 +1187,17 @@ void pci_unregister_driver(struct pci_driver *dev);
>  	module_driver(__pci_driver, pci_register_driver, \
>  		       pci_unregister_driver)
>  
> +/**
> + * builtin_pci_driver() - Helper macro for registering a PCI driver
> + * @__pci_driver: pci_driver struct
> + *
> + * Helper macro for PCI drivers which do not do anything special in their
> + * init code. This eliminates a lot of boilerplate. Each driver may only
> + * use this macro once, and calling it replaces device_initcall(...)

The builtin_platform_driver() patches I see are single-line patches,
so they don't look like they remove a *lot* of boilerplate.

Bjorn

> + */
> +#define builtin_pci_driver(__pci_driver) \
> +	builtin_driver(__pci_driver, pci_register_driver)
> +
>  struct pci_driver *pci_dev_driver(const struct pci_dev *dev);
>  int pci_add_dynid(struct pci_driver *drv,
>  		  unsigned int vendor, unsigned int device,
> -- 
> 2.5.0
> 

Re: [PATCH] PCI: create builtin_pci_driver to avoid registration boilerplate

[Paul Gortmaker]

[Re: [PATCH] PCI: create builtin_pci_driver to avoid registration boilerplate] On 15/09/2015 (Tue 11:46) Bjorn Helgaas wrote:

> Hi Paul,
> 
> On Tue, Aug 25, 2015 at 08:25:37PM -0400, Paul Gortmaker wrote:
> > In commit f309d4443130bf814e991f836e919dca22df37ae ("platform_device:
> > better support builtin boilerplate avoidance") we introduced the
> > builtin_driver macro.
> > 
> > Here we use that support and extend it to PCI driver registration,
> > so where a driver is clearly non-modular and builtin-only, we can
> > register it in a similar fashion.  And existing code that is clearly
> > non-modular can be updated with the simple mapping of
> > 
> >      module_pci_driver(...)  ---> builtin_pci_driver(...)
> > 
> > We've essentially cloned the former to make the latter, and taken
> > out the remove/module_exit parts since those never get used in a
> > non-modular build of the code.
> 
> Do you have any estimate of how many potential users of this there
> are?  I took a quick look at users of module_pci_driver() (I see
> almost 300 of them in v4.3-rc1), and most of them look like legitimate
> modules.  But the comment mentions replacing device_initcall() as
> well, so maybe there are more there?

It doesn't appear to be a lot; maybe a couple.  When I sent this, I had
not completed much of the treewide audit, nor strayed outside of x86,
but I sent it ahead of time since I was hoping to stage any
infrastructure patches like this ahead of conversions to avoid cross
subsystem dependencies.

> 
> If only a couple would be converted to builtin_pci_driver(), I'm not
> sure it's worth it, because it does add more things to look at
> (builtin_pci_driver() in addition to module_pci_driver()).

So the real goal I am after here, which may not have been clear from
this one stand-alone patch, is to ensure people have what they need to
make non-modular drivers clearly non-modular.  Currently we have about
300+ drivers/files including module.h and using modular registration
functions when they can only be built in.  And along with that is about
5k worth of dead code in exit / unregistration functions that never has
been used.

In which case, the numbers maybe don't matter.  If we have a specific
modular registration function for subsystem X, then we probably should
also have a non-modular/builtin parallel of the same thing.

However, since there are so few in PCI, I guess we could replace
non-modular users with it open coded, if you don't want to add the new
alias/macro, i.e.:

module_pci_driver(xyz) --> builtin_driver(xyz, pci_register_driver)


[...]

> > + * builtin_pci_driver() - Helper macro for registering a PCI driver
> > + * @__pci_driver: pci_driver struct
> > + *
> > + * Helper macro for PCI drivers which do not do anything special in their
> > + * init code. This eliminates a lot of boilerplate. Each driver may only
> > + * use this macro once, and calling it replaces device_initcall(...)
> 
> The builtin_platform_driver() patches I see are single-line patches,
> so they don't look like they remove a *lot* of boilerplate.

The comment applies here as much as it does to the same comment for the
module_pci_driver macro ; I'm assuming it was in reference to what it
would look like if none of the platform driver helper macros were used
and the whole thing was open coded.

Anyway, I'm open to suggestions here, now that you know the whole story.

Paul.
--

> 
> Bjorn
> 
> > + */
> > +#define builtin_pci_driver(__pci_driver) \
> > +	builtin_driver(__pci_driver, pci_register_driver)
> > +
> >  struct pci_driver *pci_dev_driver(const struct pci_dev *dev);
> >  int pci_add_dynid(struct pci_driver *drv,
> >  		  unsigned int vendor, unsigned int device,
> > -- 
> > 2.5.0
> > 

Re: [PATCH] PCI: create builtin_pci_driver to avoid registration boilerplate

[Bjorn Helgaas]

On Tue, Aug 25, 2015 at 08:25:37PM -0400, Paul Gortmaker wrote:
> In commit f309d4443130bf814e991f836e919dca22df37ae ("platform_device:
> better support builtin boilerplate avoidance") we introduced the
> builtin_driver macro.
> 
> Here we use that support and extend it to PCI driver registration,
> so where a driver is clearly non-modular and builtin-only, we can
> register it in a similar fashion.  And existing code that is clearly
> non-modular can be updated with the simple mapping of
> 
>      module_pci_driver(...)  ---> builtin_pci_driver(...)
> 
> We've essentially cloned the former to make the latter, and taken
> out the remove/module_exit parts since those never get used in a
> non-modular build of the code.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Applied to pci/misc for v4.4, thanks, Paul!

> ---
>  include/linux/pci.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 88bee285b93d..8da2758e7d0e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1187,6 +1187,17 @@ void pci_unregister_driver(struct pci_driver *dev);
>  	module_driver(__pci_driver, pci_register_driver, \
>  		       pci_unregister_driver)
>  
> +/**
> + * builtin_pci_driver() - Helper macro for registering a PCI driver
> + * @__pci_driver: pci_driver struct
> + *
> + * Helper macro for PCI drivers which do not do anything special in their
> + * init code. This eliminates a lot of boilerplate. Each driver may only
> + * use this macro once, and calling it replaces device_initcall(...)
> + */
> +#define builtin_pci_driver(__pci_driver) \
> +	builtin_driver(__pci_driver, pci_register_driver)
> +
>  struct pci_driver *pci_dev_driver(const struct pci_dev *dev);
>  int pci_add_dynid(struct pci_driver *drv,
>  		  unsigned int vendor, unsigned int device,
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/