Re: [PATCH 1/7] platform_device: better support builtin boilerplate avoidance

[Paul Gortmaker <paul.gortmaker@windriver.com>]

[Re: [PATCH 1/7] platform_device: better support builtin boilerplate avoidance] On 12/05/2015 (Tue 13:46) Linus Walleij wrote:

> On Sun, May 10, 2015 at 9:49 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> 
> > We have macros that help reduce the boilerplate for modules
> > that register with no extra init/exit complexity other than the
> > most standard use case.  However we see an increasing number of
> > non-modular drivers using these modular_driver() type register
> > functions.
> >
> > There are several downsides to this:
> > 1) The code can appear modular to a reader of the code, and they
> >    won't know if the code really is modular without checking the
> >    Makefile and Kconfig to see if compilation is governed by a
> >    bool or tristate.
> > 2) Coders of drivers may be tempted to code up an __exit function
> >    that is never used, just in order to satisfy the required three
> >    args of the modular registration function.
> > 3) Non-modular code ends up including the <module.h> which increases
> >    CPP overhead that they don't need.
> > 4) It hinders us from performing better separation of the module
> >    init code and the generic init code.
> >
> > Here we introduce similar macros, with the mapping from module_driver
> > to builtin_driver and similar, so that simple changes of:
> >
> >   module_platform_driver()       --->  builtin_platform_driver()
> >   module_platform_driver_probe() --->  builtin_platform_driver_probe().
> >
> > can help us avoid #3 above, without having to code up the same
> > __init functions and device_initcall() boilerplate.
> >
> > For non modular code, module_init becomes __initcall.  But direct use
> > of __initcall is discouraged, vs. one of the priority categorized
> > subgroups.  As __initcall gets mapped onto device_initcall, our
> > use of device_initcall directly in this change means that the
> > runtime impact is zero -- drivers will remain at level 6 in the
> > initcall ordering.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> 
> This does not inhibit probe() and remove() to be
> triggered from sysfs does it?
> 
> What is needed on builtin drivers is to set
> .suppress_bind_attrs = true on the struct device_driver
> so that we inhibit the creation of sysfs files to probe
> and remove the driver by operator intervention.

Is this needed?  I think we will break existing use cases if we do this.

For example, I have IGB as built-in, but I can still unbind one of the
four devices and make it available for PCI pass through to KVM with:

echo "0000:0a:00.1" > /sys/bus/pci/drivers/igb/unbind
echo "0000:0a:00.1" > /sys/bus/pci/drivers/pci-stub/bind

> 
> I don't know if there is a simple way do address
> this though since you don't seem to operate on
> the struct device_driver, just pass it on.
> 
> Maybe it's possible to inhibit compilation of
> builtin_platform_driver's if .suppress_bind_attrs == 0?

If we wanted to do this, I think we could simply do something like:

int __platform_driver_register(struct platform_driver *drv,
                                struct module *owner)
{
        drv->driver.owner = owner;
        drv->driver.bus = &platform_bus_type;
+       if (!owner)	/* built in */
+               drv->driver.suppress_bind_attrs = true;
        if (drv->probe)
                drv->driver.probe = platform_drv_probe;
        if (drv->remove)

...but again, I'm thinking that will break things for people, unless I'm
missing something here.

Paul.
--

> 
> Yours,
> Linus Walleij