LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
@ 2007-03-31 21:55 David Brownell
  2007-04-02 10:49 ` Cornelia Huck
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: David Brownell @ 2007-03-31 21:55 UTC (permalink / raw)
  To: Greg KH, Linux Kernel list; +Cc: Andres Salomon

We've had various reports of some legacy "probe the hardware" style platform
drivers having nasty problems with hotplug support.

The core issue is that those legacy drivers don't fully conform to the driver
model.  They assume a role that should be the responsibility of infrastructure
code: creating device nodes.

The "modprobe" step in hotplugging relies on drivers to have split those
roles into different modules.  The lack of this split causes the problems.
When a driver creates nodes for devices that don't exist (sending a hotplug
event), then exits (aborting one modprobe) before the "modprobe $MODALIAS"
step completes (by failing, since it's in the middle of a modprobe), the
result can be an endless loop of modprobe invocations ... badness.

This fix uses the newish per-device flag controlling issuance of "add" events.
(A previous version of this patch used a per-device "driver can hotplug" flag,
which only scrubbed $MODALIAS from the environment rather than suppressing the
entire hotplug event.)  It also shrinks that flag to one bit, saving a word in
"struct device".

So the net of this patch is removing some nasty failures with legacy drivers,
while retaining hotplug capability for the majority of platform drivers.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>


--- g26.orig/include/linux/device.h	2007-03-30 16:44:04.000000000 -0700
+++ g26/include/linux/device.h	2007-03-31 14:39:35.000000000 -0700
@@ -395,12 +395,13 @@ struct device {
 	struct klist_node	knode_parent;		/* node in sibling list */
 	struct klist_node	knode_driver;
 	struct klist_node	knode_bus;
-	struct device 	* parent;
+	struct device		*parent;
 
 	struct kobject kobj;
 	char	bus_id[BUS_ID_SIZE];	/* position on parent bus */
 	struct device_type	*type;
 	unsigned		is_registered:1;
+	unsigned		uevent_suppress:1;
 	struct device_attribute uevent_attr;
 	struct device_attribute *devt_attr;
 
@@ -441,7 +442,6 @@ struct device {
 	struct class		*class;
 	dev_t			devt;		/* dev_t, creates the sysfs "dev" */
 	struct attribute_group	**groups;	/* optional groups */
-	int			uevent_suppress;
 
 	void	(*release)(struct device * dev);
 };
--- g26.orig/drivers/base/platform.c	2007-03-30 16:44:04.000000000 -0700
+++ g26/drivers/base/platform.c	2007-03-31 14:23:02.000000000 -0700
@@ -160,6 +160,11 @@ static void platform_device_release(stru
  *
  *	Create a platform device object which can have other objects attached
  *	to it, and which will have attached objects freed when it is released.
+ *
+ *	This device will be marked as not supporting hotpluggable drivers; in
+ *	the unusual case that the device isn't being dynamically allocated as
+ *	of a legacy "probe the hardware" driver, infrastructure code should
+ *	reverse this marking.
  */
 struct platform_device *platform_device_alloc(const char *name, unsigned int id)
 {
@@ -172,6 +177,12 @@ struct platform_device *platform_device_
 		pa->pdev.id = id;
 		device_initialize(&pa->pdev.dev);
 		pa->pdev.dev.release = platform_device_release;
+
+		/* prevent hotplug "modprobe $(MODALIAS)" from causing trouble in
+		 * legacy probe-the-hardware drivers, which don't properly split
+		 * out enumeration logic from drivers.
+		 */
+		pa->pdev.dev.uevent_suppress = 1;
 	}
 
 	return pa ? &pa->pdev : NULL;
@@ -349,6 +360,13 @@ EXPORT_SYMBOL_GPL(platform_device_unregi
  *	memory allocated for the device allows drivers using such devices
  *	to be unloaded iwithout waiting for the last reference to the device
  *	to be dropped.
+ *
+ *	This interface is primarily intended for use with legacy drivers
+ *	which probe hardware directly.  Because such drivers create device
+ *	nodes themselves, rather than letting system infrastructure handle
+ *	such device enumeration tasks, they don't fully conform to the Linux
+ *	driver model.  In particular, when such drivers are built as modules,
+ *	they can't be "hotplugged".
  */
 struct platform_device *platform_device_register_simple(char *name, unsigned int id,
 							struct resource *res, unsigned int num)
--- g26.orig/drivers/pcmcia/pxa2xx_sharpsl.c	2007-03-30 16:44:04.000000000 -0700
+++ g26/drivers/pcmcia/pxa2xx_sharpsl.c	2007-03-31 14:26:04.000000000 -0700
@@ -261,6 +261,8 @@ static int __init sharpsl_pcmcia_init(vo
 	if (!sharpsl_pcmcia_device)
 		return -ENOMEM;
 
+	/* REVISIT just statically allocate the device */
+	sharpsl_pcmcia_device->dev.uevent_suppress = 0;
 	sharpsl_pcmcia_device->dev.platform_data = &sharpsl_pcmcia_ops;
 	sharpsl_pcmcia_device->dev.parent = platform_scoop_config->devs[0].dev;
 
--- g26.orig/drivers/pcmcia/pxa2xx_mainstone.c	2007-03-30 16:44:04.000000000 -0700
+++ g26/drivers/pcmcia/pxa2xx_mainstone.c	2007-03-31 14:23:56.000000000 -0700
@@ -175,6 +175,8 @@ static int __init mst_pcmcia_init(void)
 	if (!mst_pcmcia_device)
 		return -ENOMEM;
 
+	/* REVISIT just statically allocate the device */
+	mst_pcmcia_device->dev.uevent_suppress = 0;
 	mst_pcmcia_device->dev.platform_data = &mst_pcmcia_ops;
 
 	ret = platform_device_add(mst_pcmcia_device);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
  2007-03-31 21:55 [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers David Brownell
@ 2007-04-02 10:49 ` Cornelia Huck
  2007-04-02 16:59   ` David Brownell
  2007-04-02 10:55 ` Russell King
  2007-04-02 20:10 ` Dmitry Torokhov
  2 siblings, 1 reply; 8+ messages in thread
From: Cornelia Huck @ 2007-04-02 10:49 UTC (permalink / raw)
  To: David Brownell; +Cc: Greg KH, Linux Kernel list, Andres Salomon

On Sat, 31 Mar 2007 14:55:38 -0700,
David Brownell <david-b@pacbell.net> wrote:

> This fix uses the newish per-device flag controlling issuance of "add" events.
> (A previous version of this patch used a per-device "driver can hotplug" flag,
> which only scrubbed $MODALIAS from the environment rather than suppressing the
> entire hotplug event.)  It also shrinks that flag to one bit, saving a word in
> "struct device".

Would this still work on top of
driver-core-suppress-uevents-via-filter.patch (in -mm), which
suppresses all uevents (not just the add event)? I'd think yes, but I'm
not that familiar with platform devices :)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
  2007-03-31 21:55 [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers David Brownell
  2007-04-02 10:49 ` Cornelia Huck
@ 2007-04-02 10:55 ` Russell King
  2007-04-02 16:39   ` David Brownell
  2007-04-02 20:10 ` Dmitry Torokhov
  2 siblings, 1 reply; 8+ messages in thread
From: Russell King @ 2007-04-02 10:55 UTC (permalink / raw)
  To: David Brownell; +Cc: Greg KH, Linux Kernel list, Andres Salomon

On Sat, Mar 31, 2007 at 02:55:38PM -0700, David Brownell wrote:
> --- g26.orig/drivers/pcmcia/pxa2xx_mainstone.c	2007-03-30 16:44:04.000000000 -0700
> +++ g26/drivers/pcmcia/pxa2xx_mainstone.c	2007-03-31 14:23:56.000000000 -0700
> @@ -175,6 +175,8 @@ static int __init mst_pcmcia_init(void)
>  	if (!mst_pcmcia_device)
>  		return -ENOMEM;
>  
> +	/* REVISIT just statically allocate the device */
> +	mst_pcmcia_device->dev.uevent_suppress = 0;

Such a comment indicates that you clearly do not understand why these
platform devices are dynamically allocated.

These are modules.  If they were statically allocated, then you have a
potential oops waiting to happen if you have the right ordering of user
accesses to sysfs coupled with an inopportune unload of such a driver -
the memory backing the platform device will be unexpectedly released
resulting in an access to freed memory.

The dynamic allocation interfaces for platform devices is there to
allow drivers to properly conform to the sysfs lifetime rules.

Any module statically allocating a device structure is just simply buggy.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
  2007-04-02 10:55 ` Russell King
@ 2007-04-02 16:39   ` David Brownell
  0 siblings, 0 replies; 8+ messages in thread
From: David Brownell @ 2007-04-02 16:39 UTC (permalink / raw)
  To: Russell King; +Cc: Greg KH, Linux Kernel list, Andres Salomon

On Monday 02 April 2007 3:55 am, Russell King wrote:
> On Sat, Mar 31, 2007 at 02:55:38PM -0700, David Brownell wrote:
> > --- g26.orig/drivers/pcmcia/pxa2xx_mainstone.c	2007-03-30 16:44:04.000000000 -0700
> > +++ g26/drivers/pcmcia/pxa2xx_mainstone.c	2007-03-31 14:23:56.000000000 -0700
> > @@ -175,6 +175,8 @@ static int __init mst_pcmcia_init(void)
> >  	if (!mst_pcmcia_device)
> >  		return -ENOMEM;
> >  
> > +	/* REVISIT just statically allocate the device */
> > +	mst_pcmcia_device->dev.uevent_suppress = 0;
> 
> Such a comment indicates that you clearly do not understand why these
> platform devices are dynamically allocated.
> 
> These are modules.  If they were statically allocated, then you have a
> potential oops waiting to happen if you have the right ordering of user
> accesses to sysfs coupled with an inopportune unload of such a driver -
> the memory backing the platform device will be unexpectedly released
> resulting in an access to freed memory.

OK, so I consider that issue sufficiently revisited.  :)


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
  2007-04-02 10:49 ` Cornelia Huck
@ 2007-04-02 16:59   ` David Brownell
  2007-04-03  8:52     ` Cornelia Huck
  0 siblings, 1 reply; 8+ messages in thread
From: David Brownell @ 2007-04-02 16:59 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: Greg KH, Linux Kernel list, Andres Salomon

On Monday 02 April 2007 3:49 am, Cornelia Huck wrote:
> On Sat, 31 Mar 2007 14:55:38 -0700,
> David Brownell <david-b@pacbell.net> wrote:
> 
> > This fix uses the newish per-device flag controlling issuance of "add" events.
> > (A previous version of this patch used a per-device "driver can hotplug" flag,
> > which only scrubbed $MODALIAS from the environment rather than suppressing the
> > entire hotplug event.)  It also shrinks that flag to one bit, saving a word in
> > "struct device".
> 
> Would this still work on top of
> driver-core-suppress-uevents-via-filter.patch (in -mm), which
> suppresses all uevents (not just the add event)? I'd think yes, but I'm
> not that familiar with platform devices :)

It depends what "yes" means.  ;)

Yes, as far as I can tell the only additional change (today) from
that "supress all events" patch would be that "remove" events would
also go away ... last time I checked, only add/remove events were
issued for those devices.

But long term, I wonder.  Isn't "no kevents issued" an extremely
blunt tool, which could cause lots of damage?  It might be better
to have selective filters, one per event family:  core (add/remove),
online/offline, mount/unmount, etc.

Specifically with respect to legacy drivers, it might make sense
that managing the devices they create not be confusable with managing
"real" devices, so I don't immediately suspect that suppressing ALL
the hotplug events might be troublesome.

But in general it's worth thinking about.  The comments on that
"suppress all kevents" patch didn't include any motivation at all.
Why do you want to prevent all kevents, rather than just a subset?

- Dave

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
  2007-03-31 21:55 [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers David Brownell
  2007-04-02 10:49 ` Cornelia Huck
  2007-04-02 10:55 ` Russell King
@ 2007-04-02 20:10 ` Dmitry Torokhov
  2007-04-02 20:17   ` David Brownell
  2 siblings, 1 reply; 8+ messages in thread
From: Dmitry Torokhov @ 2007-04-02 20:10 UTC (permalink / raw)
  To: David Brownell; +Cc: Greg KH, Linux Kernel list, Andres Salomon

On 3/31/07, David Brownell <david-b@pacbell.net> wrote:
> @@ -349,6 +360,13 @@ EXPORT_SYMBOL_GPL(platform_device_unregi
>  *     memory allocated for the device allows drivers using such devices
>  *     to be unloaded iwithout waiting for the last reference to the device
>  *     to be dropped.
> + *
> + *     This interface is primarily intended for use with legacy drivers
> + *     which probe hardware directly.  Because such drivers create device
> + *     nodes themselves, rather than letting system infrastructure handle
> + *     such device enumeration tasks, they don't fully conform to the Linux
> + *     driver model.  In particular, when such drivers are built as modules,
> + *     they can't be "hotplugged".
>  */
>  struct platform_device *platform_device_register_simple(char *name, unsigned int id,

I find this comment misleading. Many of these drivers do not create
any devices (as in /dev/xxx) but rather create underlying hardware
abstraction objects.


-- 
Dmitry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
  2007-04-02 20:10 ` Dmitry Torokhov
@ 2007-04-02 20:17   ` David Brownell
  0 siblings, 0 replies; 8+ messages in thread
From: David Brownell @ 2007-04-02 20:17 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Greg KH, Linux Kernel list, Andres Salomon

On Monday 02 April 2007 1:10 pm, Dmitry Torokhov wrote:
> On 3/31/07, David Brownell <david-b@pacbell.net> wrote:
> > @@ -349,6 +360,13 @@ EXPORT_SYMBOL_GPL(platform_device_unregi
> >  *     memory allocated for the device allows drivers using such devices
> >  *     to be unloaded iwithout waiting for the last reference to the device
> >  *     to be dropped.
> > + *
> > + *     This interface is primarily intended for use with legacy drivers
> > + *     which probe hardware directly.  Because such drivers create device
> > + *     nodes themselves, rather than letting system infrastructure handle
> > + *     such device enumeration tasks, they don't fully conform to the Linux
> > + *     driver model.  In particular, when such drivers are built as modules,
> > + *     they can't be "hotplugged".
> >  */
> >  struct platform_device *platform_device_register_simple(char *name, unsigned int id,
> 
> I find this comment misleading. Many of these drivers do not create
> any devices (as in /dev/xxx) but rather create underlying hardware
> abstraction objects.

When I refresh this patch, I'll make it say "create sysfs device nodes" to
make clear which type of device node (not the /dev/ kind) it's referring to.

- Dave

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers
  2007-04-02 16:59   ` David Brownell
@ 2007-04-03  8:52     ` Cornelia Huck
  0 siblings, 0 replies; 8+ messages in thread
From: Cornelia Huck @ 2007-04-03  8:52 UTC (permalink / raw)
  To: David Brownell; +Cc: Greg KH, Linux Kernel list, Andres Salomon

On Mon, 2 Apr 2007 09:59:28 -0700,
David Brownell <david-b@pacbell.net> wrote:

> But long term, I wonder.  Isn't "no kevents issued" an extremely
> blunt tool, which could cause lots of damage?  It might be better
> to have selective filters, one per event family:  core (add/remove),
> online/offline, mount/unmount, etc.

It all depends on your use case :) But yes, we could introduce some
flags that selectively filter out some groups of uevents.

> But in general it's worth thinking about.  The comments on that
> "suppress all kevents" patch didn't include any motivation at all.
> Why do you want to prevent all kevents, rather than just a subset?

Currently, some drivers (like firmware_class) return an error code in
their uevent function in order to suppress uevents until they did some
setup. It seemed cleaner to use uevent_suppress and filter until they
were ready. For s390, we have the problem that we get a storm of
uevents for devices which aren't useable after all; we can use
uevent_suppress to make sure that userspace doesn't know anything about
those devices until we're really sure we want to keep them.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-04-03  8:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-03-31 21:55 [patch 2.6.21-rc5-git 1/2] fix hotplug for legacy platform drivers David Brownell
2007-04-02 10:49 ` Cornelia Huck
2007-04-02 16:59   ` David Brownell
2007-04-03  8:52     ` Cornelia Huck
2007-04-02 10:55 ` Russell King
2007-04-02 16:39   ` David Brownell
2007-04-02 20:10 ` Dmitry Torokhov
2007-04-02 20:17   ` David Brownell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).