Re: PCI driver module unload race?

Greg KH
2003-03-10T21:44:40Z

On Sat, Mar 08, 2003 at 12:21:01PM -0800, Greg KH wrote:
> On Sat, Mar 08, 2003 at 08:09:43PM +0000, Russell King wrote:
> > On Sat, Mar 08, 2003 at 11:12:37AM -0800, Greg KH wrote:
> > > On Sat, Mar 08, 2003 at 10:47:49AM +0000, Russell King wrote:
> > > > Hi,
> > > >
> > > > What prevents the following scenario from happening? It's purely
> > > > theoretical - I haven't seen this occuring.
> > > >
> > > > - Load PCI driver.
> > > >
> > > > - PCI driver registers using pci_module_init(), and adds itself to sysfs.
> > > >
> > > > - Hot-plugin a PCI device which uses this driver. sysfs matches the PCI
> > > > driver, and calls the PCI drivers probe function.
> > >
> > > Ugh, yes you are correct, I can't believe I missed this before.
> > >
> > > How does this patch look?
> >
> > Hrm, I'm wondering whether this should be part of the device model
> > infrastructure. After all, surely every subsystems device driver
> > which could be a module would need this to prevent unload races?
>
> Very good point, I can see myself duplicating this logic for every
> subsystem :)
>
> I'll look into moving this into the driver core later today.

Ok, how about this patch? It boots for me :)

If no one has any complaints, I'll work on moving the USB core to using
this pointer instead of its own in struct usb_driver.

thanks,

greg k-h

# Driver core: add module owner to struct device_driver

diff -Nru a/drivers/base/bus.c b/drivers/base/bus.c
--- a/drivers/base/bus.c Mon Mar 10 13:52:15 2003
+++ b/drivers/base/bus.c Mon Mar 10 13:52:15 2003
@@ -263,14 +263,25 @@
if (dev->bus->match(dev,drv)) {
dev->driver = drv;
if (drv->probe) {
- if ((error = drv->probe(dev))) {
- dev->driver = NULL;
- return error;
+ if (!try_module_get(drv->owner)) {
+ dev_err(dev,
+ "Can't get a module reference for %s\n",
+ drv->name);
+ goto exit;
}
+
+ if ((error = drv->probe(dev)))
+ dev->driver = NULL;
+
+ module_put(drv->owner);
+
+ if (error)
+ goto exit;
}
device_bind_driver(dev);
error = 0;
}
+exit:
return error;
}

@@ -350,8 +361,16 @@
sysfs_remove_link(&drv->kobj,dev->kobj.name);
list_del_init(&dev->driver_list);
devclass_remove_device(dev);
- if (drv->remove)
- drv->remove(dev);
+ if (drv->remove) {
+ if (!try_module_get(drv->owner)) {
+ dev_err(dev,
+ "Can't get a module reference for %s\n",
+ drv->name);
+ } else {
+ drv->remove(dev);
+ module_put(drv->owner);
+ }
+ }
dev->driver = NULL;
}
}
diff -Nru a/drivers/base/power.c b/drivers/base/power.c
--- a/drivers/base/power.c Mon Mar 10 13:52:15 2003
+++ b/drivers/base/power.c Mon Mar 10 13:52:15 2003
@@ -41,10 +41,17 @@
list_for_each(node,&devices_subsys.kset.list) {
struct device * dev = to_dev(node);
if (dev->driver && dev->driver->suspend) {
- pr_debug("suspending device %s\n",dev->name);
- error = dev->driver->suspend(dev,state,level);
- if (error)
- printk(KERN_ERR "%s: suspend returned %d\n",dev->name,error);
+ if (!try_module_get(dev->driver->owner)) {
+ dev_err(dev,
+ "Can't get a module reference for %s\n",
+ dev->driver->name);
+ } else {
+ pr_debug("suspending device %s\n",dev->name);
+ error = dev->driver->suspend(dev,state,level);
+ if (error)
+ printk(KERN_ERR "%s: suspend returned %d\n",dev->name,error);
+ module_put(dev->driver->owner);
+ }
}
}
up_write(&devices_subsys.rwsem);
@@ -67,8 +74,15 @@
list_for_each_prev(node,&devices_subsys.kset.list) {
struct device * dev = to_dev(node);
if (dev->driver && dev->driver->resume) {
- pr_debug("resuming device %s\n",dev->name);
- dev->driver->resume(dev,level);
+ if (!try_module_get(dev->driver->owner)) {
+ dev_err(dev,
+ "Can't get a module reference for %s\n",
+ dev->driver->name);
+ } else {
+ pr_debug("resuming device %s\n",dev->name);
+ dev->driver->resume(dev,level);
+ module_put(dev->driver->owner);
+ }
}
}
up_write(&devices_subsys.rwsem);
@@ -89,8 +103,15 @@
list_for_each(entry,&devices_subsys.kset.list) {
struct device * dev = to_dev(entry);
if (dev->driver && dev->driver->shutdown) {
- pr_debug("shutting down %s\n",dev->name);
- dev->driver->shutdown(dev);
+ if (!try_module_get(dev->driver->owner)) {
+ dev_err(dev,
+ "Can't get a module reference for %s\n",
+ dev->driver->name);
+ } else {
+ pr_debug("shutting down %s\n",dev->name);
+ dev->driver->shutdown(dev);
+ module_put(dev->driver->owner);
+ }
}
}
up_write(&devices_subsys.rwsem);
diff -Nru a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h Mon Mar 10 13:52:15 2003
+++ b/include/linux/device.h Mon Mar 10 13:52:15 2003
@@ -112,6 +112,7 @@
extern void bus_remove_file(struct bus_type *, struct bus_attribute *);

struct device_driver {
+ struct module * owner;
char * name;
struct bus_type * bus;
struct device_class * devclass;