static void cm109_stop_traffic(struct cm109_dev *dev)
{
dev->shutdown = 1;
/*
* Make sure other CPUs see this
*/
smp_wmb();
usb_kill_urb(dev->urb_ctl);
usb_kill_urb(dev->urb_irq);
cm109_toggle_buzzer_sync(dev, 0);
dev->shutdown = 0;
smp_wmb();
I don't know anything about this driver, but the placement of the second
smp_wmb() looks odd. Should it really come before the line that sets
dev->shutdown to 0? In general, smp_wmb() is used to separate two sets
of stores; if it comes after all the relevant stores have been performed
then it won't accomplish anything.
}
This driver has a tough job as the two completion
handlers submitted each other's as well as their own
URBs based on the data they get.
That scheme is rather complex, but as far as I can tell correct,
but you need to test that flag everywhere.
However, it's quite noticeable that the code you want to change in
cm109_submit_buzz_toggle() doesn't have any memory barriers to pair with
the smb_wmb()'s above. Shouldn't there at least be an smp_rmb() after
you read dev->shutdown?
As long you're updating the synchronization, it might be a good idea to
also improve the comment describing the memory barriers. "Make sure
other CPUs see this" doesn't mean anything -- of course all the other
CPUs will eventually see the changes made here. The point is that they
should see the new value of dev->shutdown before seeing the final
completion of the two URBs. Also, the comment should say which other
memory barriers pair with the ones here.