Re: WARNING in cm109_urb_irq_callback/usb_submit_urb

[Alan Stern]

On Thu, Mar 27, 2025 at 12:42:41PM +0100, Oliver Neukum wrote:
> 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?

There's no special interaction between them.  Just the usual ordering 
requirement between the smp_wmb() memory barrier and the write part of 
a spin_lock() or spin_unlock().

> > > }
> > > 
> > > 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.

I haven't looked at the code, but it sounds like a quick audit might be 
in order.

> > 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.

My point was that the memory barrier doesn't "make sure other CPUs will 
see this", as the command says.  Rather, it provides ordering: It 
ensures that other CPUs will see the writes preceding the memory barrier 
before they see the writes following the memory barrier.

Hence the comment should be updated, so that it provides information 
that actually is important for someone reading the code to know.

Alan Stern