Re: WARNING in cm109_urb_irq_callback/usb_submit_urb

Oliver Neukum
2025-03-27T11:42:44Z

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