Re: WARNING in cm109_urb_irq_callback/usb_submit_urb

From: Oliver Neukum
Date: Thu Mar 27 2025 - 07:46:35 EST


Hi,

On 20.03.25 18:25, Alan Stern wrote:

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

Indeed. This driver is not written for comprehension. As far as I can tell
it is not necessary at all. You need to set shutdown to zero before you
resubmit the URBs. But I don't see how the barrier helps with that.

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.

Don't we guarantee an interaction between smp_wmb() and taking a spinlock?

}

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?

I think this driver assumes that the ctl_submit_lock spinlock makes
it safe.

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.

Before the completion? AFAICT they need to see it before they try
to submit an URB.

Regards
Oliver