consolidate /sbin/hotplug call for pci and usb
Subsystems (3)
RFC 1/1 consolidate /sbin/hotplug call for pci and usb
Hi all,
Here's a patch against 2.5.38 that does the following:
- adds a call to /sbin/hotplug to the driver core
- removes the call to /sbin/hotplug from the pci and usb cores
- adds a callback to the struct bus_type for the driver core to use to set up bus specific /sbin/hotplug arguments.
One nice side effect of this patch, is that all iterfaces for a USB device generate a call to /sbin/hotplug, so complex devices should now be properly recognized by userspace much easier.
I've successfully tested this with both PCI Hotplug systems, and USB devices. There is still a bug remaining that causes an oops when a USB pci controller is unloaded from the system that I am still tracking down. After I find this problem, and if there are no complaints, I'll send it on to Linus.
Comments appreciated.
thanks,
greg k-h
drivers/base/Makefile +4 -0
@@ -5,6 +5,10 @@ obj-y += fs/ +ifeq ($(CONFIG_HOTPLUG),y) + obj-y += hotplug.o +endif + export-objs := core.o power.o sys.o bus.o driver.o \ class.o intf.o
drivers/base/base.h +10 -0
@@ -50,3 +50,13 @@extern int driver_attach (struct device_driver * drv);extern void driver_detach (struct device_driver * drv); + +#ifdef CONFIG_HOTPLUG +extern int dev_hotplug (struct device *dev,const char *action); +#else +static inline int dev_hotplug (struct device *dev,const char *action) +{ +return 0 ; +} +#endif +
drivers/base/core.c +6 -0
@@ -198,6 +198,9 @@if (platform_notify)platform_notify (dev); +/* notify userspace of device entry */ +dev_hotplug (dev,"add" ); + register_done:if (error) {spin_lock (&device_lock);
@@ -255,6 +258,9 @@device_detach (dev);bus_remove_device (dev); + +/* notify userspace that this device just disappeared */ +dev_hotplug (dev,"remove" );/* remove the driverfs directory */ device_remove_dir (dev);
drivers/base/hotplug.c new file +114 -0
114 lines not on this page · raw message
drivers/pci/hotplug.c +28 -42
96 lines not on this page · raw message
drivers/pci/pci-driver.c +4 -2
@@ -6,6 +6,7 @@#include <linux/pci.h> #include <linux/module.h> #include <linux/init.h> +#include "pci.h" /* * Registration of PCI drivers and handling of hot-pluggable devices.
@@ -199,8 +200,9 @@ }struct bus_type pci_bus_type = { - name:"pci" , - match: pci_bus_match, + name:"pci" , + match: pci_bus_match, + hotplug: pci_hotplug, };static int __init pci_driver_init (void )
drivers/pci/pci.h new file +5 -0
@@ -0,0 +1,5 @@ +/* Functions internal to the PCI core code */ + +extern int pci_hotplug (struct device *dev,char **envp,int num_envp, +char *buffer,int buffer_size); +
drivers/usb/core/usb.c +62 -77
199 lines not on this page · raw message
include/linux/device.h +2 -0
@@ -67,6 +67,8 @@int (*match)(struct device * dev,struct device_driver * drv);struct device * (*add) (struct device * parent,char * bus_id); +int (*hotplug) (struct device *dev,char **envp, +int num_envp,char *buffer,int buffer_size); };
Quoted 4 lines · Greg KH
diff -Nru a/drivers/base/Makefile b/drivers/base/Makefile
drivers/base/Makefile line 14
Quoted 10 lines
drivers/base/Makefile +4
-0@@ -5,6 +5,10 @@ obj-y += fs/ +ifeq ($(CONFIG_HOTPLUG),y) + obj-y += hotplug.o +endif + export-objs := core.o power.o sys.o bus.o driver.o \ class.o intf.o
Why not the simpler
obj-$(CONFIG_HOTPLUG) += hotplug.o
?
--Kai
Quoted 19 lines · Kai Germaschewski
Quoted 4 lines · Greg KH
diff -Nru a/drivers/base/Makefile b/drivers/base/Makefile
drivers/base/Makefile line 14
Quoted 10 lines
drivers/base/Makefile +4
-0@@ -5,6 +5,10 @@ obj-y += fs/ +ifeq ($(CONFIG_HOTPLUG),y) + obj-y += hotplug.o +endif + export-objs := core.o power.o sys.o bus.o driver.o \ class.o intf.oWhy not the simpler
Quoted 1 line
obj-$(CONFIG_HOTPLUG) += hotplug.o
Thanks, Pat also pointed this out to me.
I'll fix it.
thanks,
greg k-h
drivers/pci/hotplug.c line 23
Quoted 8 lines
drivers/pci/hotplug.c +7
-0+/* stuff we want to pass to /sbin/hotplug */ + envp[i++] = scratch; + scratch +=sprintf (scratch,"PCI_CLASS=%04X" , pdev->class) +1 ; + + envp[i++] = scratch; + scratch +=sprintf (scratch,"PCI_ID=%04X:%04X" , + pdev->vendor, pdev->device) +1 ;
And so forth. Use "snprintf" and prevent overrunning those buffers...
- Dave
Quoted 10 lines · David Brownell
drivers/pci/hotplug.c line 23
Quoted 8 lines
drivers/pci/hotplug.c +7
-0+/* stuff we want to pass to /sbin/hotplug */ + envp[i++] = scratch; + scratch +=sprintf (scratch,"PCI_CLASS=%04X" , pdev->class) +1 ; + + envp[i++] = scratch; + scratch +=sprintf (scratch,"PCI_ID=%04X:%04X" , + pdev->vendor, pdev->device) +1 ;And so forth. Use "snprintf" and prevent overrunning those buffers...
Doh, will do.
I also found the unload USB module problem. The driver core was calling hotplug after the device was already removed. Made it a bit difficult to be able to describe the device that way :)
thanks,
greg k-h
Quoted 4 lines
I also found the unload USB module problem. The driver core was calling hotplug after the device was already removed. Made it a bit difficult to be able to describe the device that way :)
On the other hand, since there's no internal "this is an ex-device" state, that's also insurance that nothing could use "usbfs" to try to re-activate the device. I seem to recall oopses going away by reporting the hotplug "remove" events after the usbfs path could no longer be used. Not that I ever liked that consequence, but a fix adding such a "zombie" state would have taken a bit of time.
The real "module unload problem" has a lot to do with not having any way to track how many devices a module is bound to ... that aren't necessarily opened at the moment. (Does Rusty's patch set touch any of that?)
Without having a way to answer that question, today's un-helpful "driver is in active use" refcount would encourage rmmodding drivers that users will expect to still be available. Plug in two devices, look at one, decide to use the other, unplug the first ... and just because you hadn't yet opened the second device, its driver module vanishes. As you start to use it ... huge frustration quotient! :)
- Dave
Quoted 11 lines · David Brownell
Quoted 4 lines
I also found the unload USB module problem. The driver core was calling hotplug after the device was already removed. Made it a bit difficult to be able to describe the device that way :)
On the other hand, since there's no internal "this is an ex-device" state, that's also insurance that nothing could use "usbfs" to try to re-activate the device. I seem to recall oopses going away by reporting the hotplug "remove" events after the usbfs path could no longer be used. Not that I ever liked that consequence, but a fix adding such a "zombie" state would have taken a bit of time.
Yes, Pat and I have talked a lot about the need for a driver "state". I think the current goal was to see how far we can get without needing it. I was certainly cursing the lack of it today when trying to debug this problem, but in the end, having it would have only masked over the real problem that was there.
So personally, I keep going back and forth on if it is really necessary or not to have. Right now, the USB drivers and developers are very used to the fact that when the device disappears from the system, they can not access it anymore, and that this needs to be constantly checked. I think that as time goes on, and more subsystems become "hot-pluggable" either this paranoia will have to spread to the other subsystems, or we will have to create the notion of a device "state" to make things easier on everyone.
Quoted 4 lines
The real "module unload problem" has a lot to do with not having any way to track how many devices a module is bound to ... that aren't necessarily opened at the moment. (Does Rusty's patch set touch any of that?)
As far as I know, it doesn't, but I'm not sure.
Quoted 6 lines
Without having a way to answer that question, today's un-helpful "driver is in active use" refcount would encourage rmmodding drivers that users will expect to still be available. Plug in two devices, look at one, decide to use the other, unplug the first ... and just because you hadn't yet opened the second device, its driver module vanishes. As you start to use it ... huge frustration quotient! :)
Well, that's a driver unload issue, which I think everyone agrees on the fact that it's not ok to do automatic driver unload when a device is removed, because of this very problem.
thanks,
greg k-h
Quoted 6 lines
Yes, Pat and I have talked a lot about the need for a driver "state". I think the current goal was to see how far we can get without needing it. I was certainly cursing the lack of it today when trying to debug this problem, but in the end, having it would have only masked over the real problem that was there.
It'd actually be a "device state", not a "driver state" ...
And I suspect that the specific usbfs issue got fixed by that patch to make its disconnect() method actually do its job, earlier this year. If there's an open issue, I'd likely characterize it as needing to trust device drivers to disconnect() correctly ... without having good ways to verify (later on) they really did so. Unfortunately that's an area where we know drivers like to make mistakes.
Quoted 11 lines
Quoted 6 lines
Without having a way to answer that question, today's un-helpful "driver is in active use" refcount would encourage rmmodding drivers that users will expect to still be available. Plug in two devices, look at one, decide to use the other, unplug the first ... and just because you hadn't yet opened the second device, its driver module vanishes. As you start to use it ... huge frustration quotient! :)
Well, that's a driver unload issue, which I think everyone agrees on the fact that it's not ok to do automatic driver unload when a device is removed, because of this very problem.
I think it _could_ be fine to do such rmmods, if all the module remove races were removed ... and (for this issue) if the primitve were actually "remove if the driver is not (a) in active use, or
(b) bound to any device". Today we have races and (a) ... but it's the lack of (b) that prevents hotplug from even trying to rmmod, on the optimistic assumption there are no races.
- Dave
Quoted 8 lines · David Brownell
Quoted 6 lines
Yes, Pat and I have talked a lot about the need for a driver "state". I think the current goal was to see how far we can get without needing it. I was certainly cursing the lack of it today when trying to debug this problem, but in the end, having it would have only masked over the real problem that was there.
It'd actually be a "device state", not a "driver state" ...
Doh, yes, that's what I meant, sorry.
Quoted 10 lines
Quoted 3 lines
Well, that's a driver unload issue, which I think everyone agrees on the fact that it's not ok to do automatic driver unload when a device is removed, because of this very problem.
I think it _could_ be fine to do such rmmods, if all the module remove races were removed ... and (for this issue) if the primitve were actually "remove if the driver is not (a) in active use, or
(b) bound to any device". Today we have races and (a) ... but it's the lack of (b) that prevents hotplug from even trying to rmmod, on the optimistic assumption there are no races.
But how do we accomplish (b) for devices that we can't remove from the system? Like 99.9% of the pci systems?
I agree it would be "nice", but probably never realistic :)
thanks,
greg k-h
Quoted 13 lines
Quoted 11 lines
Quoted 4 lines
Well, that's a driver unload issue, which I think everyone agrees on the fact that it's not ok to do automatic driver unload when a device is removed, because of this very problem.
I think it _could_ be fine to do such rmmods, if all the module remove races were removed ... and (for this issue) if the primitve were actually "remove if the driver is not (a) in active use, or
(b) bound to any device". Today we have races and (a) ... but it's the lack of (b) that prevents hotplug from even trying to rmmod, on the optimistic assumption there are no races.
Quoted 4 lines
But how do we accomplish (b) for devices that we can't remove from the system? Like 99.9% of the pci systems?
I agree it would be "nice", but probably never realistic :)
There would be two modes for 'rmmod'. One is the "remove if the kernel can tell it can't be used" (as described above), suitable for automation. The other gives the rude end-user failure modes:
"remove this module if it's not in active use", which is all we have today ... a mode suitable only for developers or sysadmins.
Userland needs to make the policy choice, then tell the kernel whether to ignore any unopened (but bound) devices.
Highly realistic, IMO. It's just a question of ensuring some data the kernel already holds (N devices are bound to this driver, even if none are currently opened) is considered by rmmod. I almost put together a patch for it at one time. There seems to be a change in maintainer under way, maybe it'd be worth revisiting this.
- Dave
Quoted 8 lines · Greg KH
Quoted 6 lines · David Brownell
I think it _could_ be fine to do such rmmods, if all the module remove races were removed ... and (for this issue) if the primitve were actually "remove if the driver is not (a) in active use, or
(b) bound to any device". Today we have races and (a) ... but it's the lack of (b) that prevents hotplug from even trying to rmmod, on the optimistic assumption there are no races.
Quoted 4 lines
But how do we accomplish (b) for devices that we can't remove from the system? Like 99.9% of the pci systems?
I agree it would be "nice", but probably never realistic :)
This raises a generally interesting question: When should a driver module be loaded?
Should it be there all the time? -- usually not. Or only when the device is in active use? Also at detection (hotplug) and removal time? As long as the device is installed? What if the device is built into the system and can never be removed? What if the device is not subject to hotplugging itself but is required in order to operate another device that can be hotplugged (e.g., a USB host controller driver)?
I think it's clear that the answer must depend on the particular driver, and that no single scheme involving usage counts (or the equivalent) can suffice for every situation.
Is there a way to let the kernel provide a variety of mechanisms and let the device driver (or even the user) select which one gets used?
Alan Stern
Re: [linux-usb-devel] [RFC] consolidate /sbin/hotplug call for pciand usb
Quoted 3 lines
This raises a generally interesting question: When should a driver module be loaded?
It is an interesting question, but it is not a kernel problem :-)
[..]
Quoted 6 lines
I think it's clear that the answer must depend on the particular driver, and that no single scheme involving usage counts (or the equivalent) can suffice for every situation.
Is there a way to let the kernel provide a variety of mechanisms and let the device driver (or even the user) select which one gets used?
That is more or less what is in place. There's a kernel aspect to the problem, because the kernel needs to implement an unload if nothing is bound to method, as user space cannot do that without a race.
Regards
Oliver
RFC 1/1 consolidate /sbin/hotplug call for pci and usb - take 2
In reply to Greg KHOk, here's an updated version of the previous patch I sent out, that address all of the different comments that I received. It also has the previously mentioned bug fixed. It is against a clean 2.5.38 kernel.
I changed the sprintf() calls to snprintf() and if people would check my math, I would appreciate it :)
This works for me on both PCI Hotplug and USB systems. If there are no complaints about it, I'll send it on to Linus.
Again, comments appreciated.
thanks,
greg k-h
drivers/base/Makefile +2 -0
@@ -5,6 +5,8 @@ obj-y += fs/ +obj-$(CONFIG_HOTPLUG) += hotplug.o + export-objs := core.o power.o sys.o bus.o driver.o \ class.o intf.o
drivers/base/base.h +10 -0
@@ -50,3 +50,13 @@extern int driver_attach (struct device_driver * drv);extern void driver_detach (struct device_driver * drv); + +#ifdef CONFIG_HOTPLUG +extern int dev_hotplug (struct device *dev,const char *action); +#else +static inline int dev_hotplug (struct device *dev,const char *action) +{ +return 0 ; +} +#endif +
drivers/base/core.c +6 -0
@@ -198,6 +198,9 @@if (platform_notify)platform_notify (dev); +/* notify userspace of device entry */ +dev_hotplug (dev,"add" ); + register_done:if (error) {spin_lock (&device_lock);
@@ -252,6 +255,9 @@*/ if (platform_notify_remove)platform_notify_remove (dev); + +/* notify userspace that this device is about to disappear */ +dev_hotplug (dev,"remove" );device_detach (dev);bus_remove_device (dev);
drivers/base/hotplug.c new file +98 -0
98 lines not on this page · raw message
drivers/pci/hotplug.c +54 -42
122 lines not on this page · raw message
drivers/pci/pci-driver.c +4 -2
@@ -6,6 +6,7 @@#include <linux/pci.h> #include <linux/module.h> #include <linux/init.h> +#include "pci.h" /* * Registration of PCI drivers and handling of hot-pluggable devices.
@@ -199,8 +200,9 @@ }struct bus_type pci_bus_type = { - name:"pci" , - match: pci_bus_match, + name:"pci" , + match: pci_bus_match, + hotplug: pci_hotplug, };static int __init pci_driver_init (void )
drivers/pci/pci.h new file +5 -0
@@ -0,0 +1,5 @@ +/* Functions internal to the PCI core code */ + +extern int pci_hotplug (struct device *dev,char **envp,int num_envp, +char *buffer,int buffer_size); +
drivers/usb/core/usb.c +81 -82
220 lines not on this page · raw message
include/linux/device.h +2 -0
@@ -67,6 +67,8 @@int (*match)(struct device * dev,struct device_driver * drv);struct device * (*add) (struct device * parent,char * bus_id); +int (*hotplug) (struct device *dev,char **envp, +int num_envp,char *buffer,int buffer_size); };