Re: Problems with get_driver() and driver_attach() (and new_id too)

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Jan 09, 2012 at 11:37:57AM -0500, Alan Stern wrote:
> On Mon, 9 Jan 2012, Dmitry Torokhov wrote:
> 
> > > 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.
> 
> Yes, it does.  That one looks like a misunderstanding.  It calls
> get_driver during phy_probe and put_driver during phy_remove, which
> accomplishes nothing.
> 
> > 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.
> 
> Possibly it needs to be replaced with try_module_get.  I'll send out an 
> email to the maintainers of these drivers to see what they think.

Looking at SSB some more they seem to be releasing devices to make sure
nobody accesses them while SPROM is being written, but while they do
release all devices I do not see how they prevent new binding from
taking place. It all should probably replaced by doing
mutex_trylock(&bus->sprom_lock) in their probe() and using device core
iterators to unbind/rebind the drivers (do we even need to do the
bookkeeping of devices previously bound/not bound?).

Thanks.

-- 
Dmitry