On Wed, Oct 26, 2011 at 10:57 AM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote: > Hi David, > >> When disconnecting a bcm203x device we kill and destroy the usb-urb, however, >> there might still be a pending work-structure which resubmits the now invalid >> urb. To avoid this race condition, we simply set a shutdown-flag and >> synchronously kill the worker first. >> >> This also adds a comment to all schedule_work()s, as it is really not clear >> that they are used as replacement for short timers (which can be seen in the git >> history). >> >> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxxxxxxx> >> --- >> drivers/bluetooth/bcm203x.c | 12 ++++++++++++ >> 1 files changed, 12 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/bluetooth/bcm203x.c b/drivers/bluetooth/bcm203x.c >> index 8b1b643..6b42732 100644 >> --- a/drivers/bluetooth/bcm203x.c >> +++ b/drivers/bluetooth/bcm203x.c >> @@ -24,6 +24,7 @@ >> >> #include <linux/module.h> >> >> +#include <linux/atomic.h> >> #include <linux/kernel.h> >> #include <linux/init.h> >> #include <linux/slab.h> >> @@ -65,6 +66,7 @@ struct bcm203x_data { >> unsigned long state; >> >> struct work_struct work; >> + atomic_t shutdown; >> >> struct urb *urb; >> unsigned char *buffer; >> @@ -97,6 +99,7 @@ static void bcm203x_complete(struct urb *urb) >> >> data->state = BCM203X_SELECT_MEMORY; >> >> + /* use workqueue to have a small delay */ >> schedule_work(&data->work); >> break; >> >> @@ -155,6 +158,10 @@ static void bcm203x_work(struct work_struct *work) >> struct bcm203x_data *data = >> container_of(work, struct bcm203x_data, work); >> >> + smp_rmb(); >> + if (atomic_read(&data->shutdown)) >> + return; >> + >> if (usb_submit_urb(data->urb, GFP_ATOMIC) < 0) >> BT_ERR("Can't submit URB"); >> } >> @@ -243,6 +250,7 @@ static int bcm203x_probe(struct usb_interface *intf, const struct usb_device_id >> >> usb_set_intfdata(intf, data); >> >> + /* use workqueue to have a small delay */ >> schedule_work(&data->work); >> >> return 0; >> @@ -254,6 +262,10 @@ static void bcm203x_disconnect(struct usb_interface *intf) >> >> BT_DBG("intf %p", intf); >> >> + atomic_inc(&data->shutdown); >> + smp_wmb(); >> + cancel_work_sync(&data->work); >> + > > can you quickly explain to me why we need the memory barrier here. atomic_inc() and atomic_read() do not imply memory barriers, hence, cancel_work_sync() may finish before atomic_inc() is visible to the worker. Then the worker could restart and resend the urb without seeing the shutdown-flag and we still have the same race. However, I think cancel_work_sync() uses spinlocks or some other kind of lock and implies a memory barrier so both smp_wmb() and smp_rmb() may be dropped. I am not sure whether it is valid to rely on cancel_work_sync() to behave that way. > Regards > > Marcel Regards David -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html