Re: WARNING in cm109_urb_irq_callback/usb_submit_urb

[Oliver Neukum <oneukum@suse.com>]

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