[BUG] Deferred probing in driver model is racy, resulting in lost probes

On Thu, Sep 27, 2012 at 10:03 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Thu, Sep 27, 2012 at 02:58:09PM +0100, Russell King - ARM Linux wrote:

quoted

On Thu, Sep 27, 2012 at 07:47:46AM +0800, Ming Lei wrote:

quoted

On Thu, Sep 27, 2012 at 4:23 AM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

To be honest, I've not bothered to test the above patch, and now when I
look at it, I notice it's broken - in that on error it will corrupt the
driver list.  Take a look at the error path.

priv is drv->p.  We add priv->knode_bus to the driver list.  If
driver_attach() returns an error, then we go to out_unregister, which

In fact, driver_attach() always returns zero, so it does __not__ affect
your test.

It may not affect my test, but the fact is, that patch lays down the
foundations for bugs to be introduced.  Now, while I can test it (and
have done) I don't think it's suitable for mainline because of _that_.

If driver_attach() always returns zero, there's no point in it returning
a value - it might as well return void and stop leading people to
believe that it might return an error.  So I suggest this alternative
patch instead, which gets rid of what is effectively dead code, and
probably a few warnings about not checking the return value from a
__must_check function (see usb-serial.c).

With this patch, no one is lead into a false sense of security that,
after your patch, if driver_attach() ever returns an error, proper
cleanup will happen - it's blatently obvious to anyone modifying it
that if they do want it to return an error, they have to audit all the
users of it, which IMHO is a good thing.

Sorry, old version of that patch.  Here's the right one.

IMO, it is better to take the one line change first since it is really
correct and easy to be backported to previous stable releases.


Thanks,
-- 
Ming Lei