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
