RFC: AMBA bus discardable probe() function [Kernel]

Prev: [PATCH -staging] slicoss: Remove explicit arch dependencies
Next: Attempted summary of suspend-blockers LKML thread, take two

From: Greg KH on 4 Aug 2010 15:50

On Wed, Aug 04, 2010 at 02:39:03PM +0200, Linus Walleij wrote:
> Fighting a compilation warning when using __init on probe():s in
> the AMBA (PrimeCell) bus abstraction, the intended effect of
> discarding AMBA probe():s is not achieveable without first adding
> the amba_driver_probe() function akin to platform_driver_probe().
>
> The latter does some extensive checks which I believe are
> necessary to replicate, and leads to this nasty hack,
> dereferencing structs from base/base.h like the platform bus does.

Ick, no, don't do that :(

> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index d31590e..2a4c88f 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -18,6 +18,9 @@
> #include <asm/irq.h>
> #include <asm/sizes.h>
>
> +/* Cross referencing the private driver core like the platform bus does */
> +#include "../base/base.h"

Nope, sorry, not allowed. This should be your first flag that something
is wrong here.

The platform bus does this because it is part of the driver core, and it
does other fun things. This should never be needed for any other bus.
Note how no other bus needs to do this, so that should be a hint that
it's wrong.

> +static int amba_driver_probe_fail(struct device *_dev)
> +{
> + return -ENXIO;
> +}
> +
> +
> +/**
> + * amba_driver_probe - register AMBA driver for non-hotpluggable device
> + * @drv: platform driver structure
> + * @probe: the driver probe routine, probably from an __init section
> + *
> + * Use this instead of amba_driver_register() when you know the device
> + * is not hotpluggable and has already been registered, and you want to
> + * remove its run-once probe() infrastructure from memory after the driver
> + * has bound to the device.

Is that _really_ worth it? Come on, how much memory are you saving
here?

> + * One typical use for this would be with drivers for controllers integrated
> + * into system-on-chip processors, where the controller devices have been
> + * configured as part of board setup.
> + *
> + * Returns zero if the driver registered and bound to a device, else returns
> + * a negative error code and with the driver not registered.
> + */
> +int __init_or_module amba_driver_probe(struct amba_driver *drv,
> + int (*probe)(struct amba_device *,
> + struct amba_id *))
> +{
> + int retval, code;
> +
> + /* make sure driver won't have bind/unbind attributes */
> + drv->drv.suppress_bind_attrs = true;
> +
> + /* temporary section violation during probe() */
> + drv->probe = probe;
> + retval = code = amba_driver_register(drv);
> +
> + /*
> + * Fixup that section violation, being paranoid about code scanning
> + * the list of drivers in order to probe new devices. Check to see
> + * if the probe was successful, and make sure any forced probes of
> + * new devices fail.
> + */
> + spin_lock(&amba_bustype.p->klist_drivers.k_lock);

Ick, nope, you can't do this, sorry. That's a "private" structure for a
reason.

So what's the real driving force here, just saving a few hundred bytes
after booting? Or something else?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Russell King - ARM Linux on 4 Aug 2010 18:30

From: Linus WALLEIJ on 5 Aug 2010 02:30

[Russell]

> which gives a total of 5188 bytes for all the above probe functions.
> However, in order to free this, we're adding 184 bytes of code and
> literal pool to achieve this:
> (...)
> The overall kernel size is 3877020 bytes, which means we're potentially
> allowing for 0.13% of the kernel to be freed at run time - which may
> equate to one or at most two additional pages.

We have the PL022 as well, and the PL08x is being added but it will
likely stay in that ballpark figure.

But overall that does not seem to be worth it, so let's drop it
for the time being. CC:ing the linux-embedded folks that
often worry about footprint size to see if someone oppose. Also
CC Viresh who works on a platform for e.g. set-top boxes using
these PrimeCells which I think may be memory-constrained.

Yours,
Linus Walleij

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Linus WALLEIJ on 5 Aug 2010 02:30

[Greg]
> [Me]
> > + spin_lock(&amba_bustype.p->klist_drivers.k_lock);
>
> Ick, nope, you can't do this, sorry. That's a "private" structure for
> a reason.

Yeah I get it, but in the platform bus case what's that traversal of
the klists actually for? I didn't get it, and was guessing that it
was considering the case where devices spawn new devices.

> So what's the real driving force here, just saving a few hundred bytes
> after booting? Or something else?

A few thousand actually according to Russells measurements.
And no I don't think it's worth it unless someone else challenge this...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Dmitry Torokhov on 5 Aug 2010 02:50

On Wednesday, August 04, 2010 11:26:00 pm Linus WALLEIJ wrote:
> [Greg]
>
> > [Me]
> >
> > > + spin_lock(&amba_bustype.p->klist_drivers.k_lock);
> >
> > Ick, nope, you can't do this, sorry. That's a "private" structure for
> > a reason.
>
> Yeah I get it, but in the platform bus case what's that traversal of
> the klists actually for? I didn't get it, and was guessing that it
> was considering the case where devices spawn new devices.

It is to check if the driver actually bound to any devices and fail driver
registration if it did not - then, in case of modular build, entire driver
module might get unloaded from memory as well.

--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

|
Pages: 1
Prev: [PATCH -staging] slicoss: Remove explicit arch dependencies
Next: Attempted summary of suspend-blockers LKML thread, take two