On Thu, Mar 20, 2025 at 04:42:33PM +0100, Oliver Neukum wrote:
> On 20.03.25 15:25, Alan Stern wrote:
>
> > This test must itself be subject to the same race, right? There needs
> > to be some kind of synchronization between the two tasks (i.e., a mutex,
> > spinlock, or something similar).
>
> Hi,
>
> there is:
>
> 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.
Alan Stern
