Re: [PATCH 5/5] staging: vme: make match() driver specific toimprove non-VME64x support

From: Emilio G. Cota
Date: Sat Aug 13 2011 - 04:50:43 EST


On Fri, Aug 12, 2011 at 12:30:51 +0200, Manohar Vanga wrote:
> +++ b/drivers/staging/vme/vme.c
(snip)
> +static int __vme_register_driver(struct vme_driver *drv, unsigned int ndevs)
> {
> - int i;
> - struct vme_dev *vdev;
> -
> + struct vme_bridge *bridge;
> + int err = 0;
>
> - for (i = 0; i < VME_SLOTS_MAX; i++) {
> - vdev = &bridge->dev[i];
> - device_unregister(&vdev->dev);
> + mutex_lock(&vme_buses_lock);
> + list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> + /*
> + * We increase the refcount of the bridge module here to
> + * prevent it from being removed during driver registration
> + */
> + if (!vme_bridge_get(bridge->num))
> + continue;

hmm have you tested this? It should deadlock, because as in
patch 3 vme_bridge_get() acquires vme_buses_lock.

An alternative is to call here try_module_get() directly on
bridge->owner, which would succeed in preventing it from being
removed (the lock is held

> + mutex_unlock(&vme_buses_lock);
> + err = __vme_register_driver_bus(drv, bridge, ndevs);
> + mutex_lock(&vme_buses_lock);
> + vme_bridge_put(bridge);

This, interestingly, wouldn't deadlock, because we pass the bridge
directly. See my second message to patch 3.

> + if (err)
> + break;
> }
> - vme_remove_bus(bridge);
> + mutex_unlock(&vme_buses_lock);
> + return err;
> }

The whole loop is admittedly complex. IIRC in my original patch
module_get/put were called here directly, and vme_buses_lock
was unlocked before calling __vme_register_driver_bus()
to avoid a deadlock, because within that function the .probe
methods of the driver would likely call vme_bridge_get().

Now that we don't export them, the loop could be simplified to:


> + mutex_lock(&vme_buses_lock);
> + list_for_each_entry(bridge, &vme_bus_list, bus_list) {
> + err = __vme_register_driver_bus(drv, bridge, ndevs);
> + if (err)
> + break;
> }
> + mutex_unlock(&vme_buses_lock);

This cannot race with a bridge being removed. Let's see how:
If the bridge driver is sane, it will call vme_unregister_bridge()
in its .release method. In there vme_remove_bus is called, and
the thread will try to acquire vme_buses_lock, which is already
held by above loop. Coming back to the loop, the try_get_module
call in vme_bus_probe will fail, because the bridge module
is being removed, and as a result all the devices under that
bridge won't be installed--this is what we wanted.

When the loop finishes we unlock vme_buses_lock and the
removal of the bus completes.

That said, I would ONLY take the simplified loop if a comment was
added to explain the above race. And I'd add that comment
near vme_bus_get/put, because if those are exported one
day, the above loop would need be changed accordingly.

Emilio


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/