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

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

[Linus Walleij]

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.

Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
I'm not sure about the proper way around this, Russell, David,
Dmitry, Greg et al, please indicate whether this is:

1) Desirable on the AMBA bus (I think so, actually all the AMBA
   devices that exist should be able to have their probe functions
   as __init() functions AFAICT, they're always embedded.)

2) Possible to do without the klist traversals, I'm not quite
   following under what circumstances this is really necessary.
   If this is just when device drivers spawn new devices then we
   might be able to do without (though in theory it'd be needed).

3) An indication that this private core stuff should somehow be
   accessible by derivative busses anyhow?

Yours,
Linus Walleij
---
 drivers/amba/bus.c       |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/spi/amba-pl022.c |    7 ++---
 include/linux/amba/bus.h |    3 ++
 3 files changed, 63 insertions(+), 4 deletions(-)

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"
+
 #define to_amba_device(d)	container_of(d, struct amba_device, dev)
 #define to_amba_driver(d)	container_of(d, struct amba_driver, drv)
 
@@ -223,6 +226,59 @@ void amba_driver_unregister(struct amba_driver *drv)
 	driver_unregister(&drv->drv);
 }
 
+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.
+ *
+ * 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);
+	drv->probe = NULL;
+	if (code == 0 && list_empty(&drv->drv.p->klist_devices.k_list))
+		retval = -ENODEV;
+	drv->drv.probe = amba_driver_probe_fail;
+	spin_unlock(&amba_bustype.p->klist_drivers.k_lock);
+
+	if (code != retval)
+		amba_driver_unregister(drv);
+	return retval;
+}
 
 static void amba_device_release(struct device *dev)
 {
@@ -442,6 +498,7 @@ void amba_release_regions(struct amba_device *dev)
 
 EXPORT_SYMBOL(amba_driver_register);
 EXPORT_SYMBOL(amba_driver_unregister);
+EXPORT_SYMBOL(amba_driver_probe);
 EXPORT_SYMBOL(amba_device_register);
 EXPORT_SYMBOL(amba_device_unregister);
 EXPORT_SYMBOL(amba_find_device);
diff --git a/drivers/spi/amba-pl022.c b/drivers/spi/amba-pl022.c
index f0a1418..28dd364 100644
--- a/drivers/spi/amba-pl022.c
+++ b/drivers/spi/amba-pl022.c
@@ -1969,16 +1969,15 @@ static struct amba_driver pl022_driver = {
 		.name	= "ssp-pl022",
 	},
 	.id_table	= pl022_ids,
-	.probe		= pl022_probe,
 	.remove		= __exit_p(pl022_remove),
-	.suspend        = pl022_suspend,
-	.resume         = pl022_resume,
+	.suspend	= pl022_suspend,
+	.resume		= pl022_resume,
 };
 
 
 static int __init pl022_init(void)
 {
-	return amba_driver_register(&pl022_driver);
+	return amba_driver_probe(&pl022_driver, pl022_probe);
 }
 
 module_init(pl022_init);
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index b0c1740..0d3d55f 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -58,6 +58,9 @@ enum amba_vendor {
 
 int amba_driver_register(struct amba_driver *);
 void amba_driver_unregister(struct amba_driver *);
+int amba_driver_probe(struct amba_driver *adrv,
+		      int (*probe)(struct amba_device *,
+				   struct amba_id *));
 int amba_device_register(struct amba_device *, struct resource *);
 void amba_device_unregister(struct amba_device *);
 struct amba_device *amba_find_device(const char *, struct device *, unsigned int, unsigned int);
-- 
1.6.3.3

Re: [PATCH] RFC: AMBA bus discardable probe() function

[Greg KH]

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

Re: [PATCH] RFC: AMBA bus discardable probe() function

[Russell King - ARM Linux]

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().

Well, I've decided to investigate what kind of savings we're looking
at.  Going by my most recently built Realview kernel, I see in
System.map:

c01754c4 t pl061_probe
c01757b0 t pl061_irq_handler
	=> 748 bytes

c01842dc t clcdfb_probe
c0184658 t clcdfb_check_var
	=> 892 bytes

c01a3860 t pl011_probe
c01a39e4 T dev_driver_string
	=> 388 bytes

c01ee0c4 t pl030_probe
c01ee1dc t pl031_alarm_irq_enable
	=> 280 bytes

c01ee7e8 t pl031_probe
c01ee950 t pl031_interrupt
	=> 360 bytes

c02b43dc t amba_kmi_probe
c02b453c t ds1307_probe
	=> 352 bytes

c02b4b84 t mmci_probe
c02b4f74 t aaci_probe
c02b53fc t smc_drv_remove
	=> 2168 bytes

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:

c01847b8 t amba_driver_probe_fail
c01847cc t resource_show
	=> 20 bytes

c0184e40 T amba_driver_probe
c0184ee4 W unxlate_dev_mem_ptr
	=> 164 bytes

This reduces the saving to 5004 bytes.

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.

RE: [PATCH] RFC: AMBA bus discardable probe() function

[Linus WALLEIJ]

[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

RE: [PATCH] RFC: AMBA bus discardable probe() function

[Linus WALLEIJ]

[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

Re: [PATCH] RFC: AMBA bus discardable probe() function

[Dmitry Torokhov]

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