On Wed, Dec 18, 2019 at 08:22:47PM +0100, Andrey Konovalov wrote:
> > > > > +static void gadget_unbind(struct usb_gadget *gadget)
> > > > > +{
> > > > > + struct raw_dev *dev = get_gadget_data(gadget);
> > > > > + unsigned long flags;
> > > > > +
> > > > > + spin_lock_irqsave(&dev->lock, flags);
> > > > > + set_gadget_data(gadget, NULL);
> > > > > + spin_unlock_irqrestore(&dev->lock, flags);
> > > > > + /* Matches kref_get() in gadget_bind(). */
> > > > > + kref_put(&dev->count, dev_free);
> > > >
> > > > What protects the kref from being called 'put' twice on the same
> > > > pointer at the same time? There should be some lock somewhere, right?
> > >
> > > Hm, kref_put() does refcount_dec_and_test(), which in turns calls
> > > atomic_dec_and_test(), so this is protected against concurrent puts
> > > (which is the whole idea of kref?), and no locking is needed. Unless I
> > > misunderstand something.
> >
> > It's late, but there should be some lock somewhere to prevent a race
> > around this type of thing. That's why we have kref_put_mutex() and
> > kref_put_lock().
> >
> > Odds are you are fine here, but just something to be aware of...
>
> Ah, I see. So AFAIU kref_put_lock/mutex() are meant to be used in
> cases when there might be a concurrent user that doesn't have the
> reference counter incremented, but holds the lock? We don't do this
> kind of stuff here.
Ok, as long as there is a lock somewhere preventing this type of thing
from happening. Last time I looked at this, it took me and 2 grad
students an hour with a whiteboard to work it all out. Which is why the
lock variants are there now :)
thanks,
greg k-h
