On Fri, Jan 06, 2012 at 03:29:34PM -0500, Alan Stern wrote:
> On Thu, 5 Jan 2012, Dmitry Torokhov wrote:
>
> > > > I think pinning driver so that it can't be unregistered (and
> > > > consequently module unload hangs) its a mis-feature.
> > >
:q> > > I suspect that references obtained from get_driver() aren't held very
> > > long. However I haven't checked every case.
> >
> > Unless we stop exporting them we can not make any assumptions on how
> > long they will be held - code is changing constantly.
>
> Something we need to watch out for: get_driver and put_driver are used
> in a bunch of other places, unrelated to driver_attach. Here's what
> I found:
>
> lib/dma-debug.c:173: drv = get_driver(dev->driver);
> lib/dma-debug.c:188: put_driver(drv);
> drivers/pci/xen-pcifront.c:596: if (get_driver(&pdrv->driver)) {
> drivers/pci/xen-pcifront.c:626: put_driver(&pdrv->driver);
> drivers/media/video/s5p-fimc/fimc-mdevice.c:348: put_driver(driver);
> drivers/media/video/s5p-fimc/fimc-mdevice.c:356: put_driver(driver);
> drivers/media/video/ivtv/ivtvfb.c:1296: put_driver(drv);
> drivers/media/video/ivtv/ivtvfb.c:1313: put_driver(drv);
> drivers/media/video/cx18/cx18-alsa-main.c:288: put_driver(drv);
> drivers/media/video/s5p-tv/mixer_video.c:61: put_driver(drv);
> drivers/s390/cio/ccwgroup.c:583: get_driver(&cdriver->driver);
> drivers/s390/cio/ccwgroup.c:595: put_driver(&cdriver->driver);
> drivers/s390/cio/device.c:1681: drv = get_driver(&cdrv->driver);
> drivers/s390/cio/device.c:1687: put_driver(drv);
> drivers/s390/net/smsgiucv_app.c:199: put_driver(smsgiucv_drv);
> drivers/ssb/main.c:146: get_driver(&drv->drv);
> drivers/ssb/main.c:153: put_driver(&drv->drv);
> drivers/net/phy/phy_device.c:934: drv = get_driver(phydev->dev.driver);
> drivers/net/phy/phy_device.c:975: put_driver(dev->driver);
>
> I don't think any of those calls actually accomplish anything, but it's
> hard to be certain. Some of them appear to be futile attempts to
> prevent the driver from being unregistered or unloaded, others are
> there simply to drop the reference taken by driver_find().
>
> In a few of them it's obvious that the driver can't be unregistered
> while the critical section runs, but in the others I can't tell. On
> the other hand, if a critical section can race with unregistration
> then the code is buggy now.
>
> What do you think?
I think we need to audit them and decide on case-by-case basis. For
example drivers/s390/cio/device.c is completely nonsensical: it takes a
reference on a driver that is passed as argument before calling
driver_find_device(). But if passed driver was valid before we called
get_driver it won't become any more valid afterwards and it should not
disappear either.
drivers/s390/cio/ccwgroup.c - calls are useless;
Authors of drivers/net/phy/phy_device.c had their reservations:
/* Make sure the driver is held.
* XXX -- Is this correct? */
drv = get_driver(phydev->dev.driver);
However it is in phydev_probe() and I hope our device core takes care of
not destroying drivers in the middle of binding to a device.
drivers/ssb/main.c seems like needs some protection but does it
incorrectly as we do not wait for drivers to drop all references before
unloading modules.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
