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.
I think that we can rely on the behavior of cancel_work_sync. So lets
drop the memory barriers and then the patch looks good.
Regards
Marcel
--
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
