fix hotplug for legacy platform drivers
Subsystems (3)
PATCH 1/2 fix hotplug for legacy platform drivers
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.
include/linux/device.h +2 -2
@@ -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); };
drivers/base/platform.c +18 -0
@@ -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)
drivers/pcmcia/pxa2xx_sharpsl.c +2 -0
@@ -261,6 +261,8 @@ static int __init sharpsl_pcmcia_init(voif (!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;
drivers/pcmcia/pxa2xx_mainstone.c +2 -0
@@ -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);
Quoted 5 lines · David Brownell
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 :)
Quoted 9 lines · Cornelia Huck
Quoted 5 lines · David Brownell
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".
Quoted 4 lines
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
Quoted 4 lines · David Brownell
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.
Quoted 3 lines
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.
drivers/pcmcia/pxa2xx_mainstone.c static int __init mst_pcmcia_init(void) · line 179
Quoted 8 lines · David Brownell
drivers/pcmcia/pxa2xx_mainstone.c +2
-0@@ -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.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 13 lines · Russell King
drivers/pcmcia/pxa2xx_mainstone.c static int __init mst_pcmcia_init(void) · line 179
Quoted 8 lines · David Brownell
drivers/pcmcia/pxa2xx_mainstone.c +2
-0@@ -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.
Quoted 5 lines
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. :)
drivers/base/platform.c EXPORT_SYMBOL_GPL(platform_device_unregi · line 371
Quoted 13 lines · David Brownell
drivers/base/platform.c +7
-0@@ -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.
Signature · 1 line
Dmitry
Quoted 10 lines · Dmitry Torokhov
Quoted 9 lines · David Brownell
drivers/base/platform.c +5
-0@@ -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
Quoted 8 lines
drivers/base/platform.c EXPORT_SYMBOL_GPL(platform_device_unregi · line 371
Quoted 4 lines
drivers/base/platform.c +2
-0+* 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