On Mon, Apr 6, 2020 at 4:07 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Mon, Apr 06, 2020 at 03:06:12PM +0200, Andrey Konovalov wrote:
> > On Mon, Apr 6, 2020 at 12:55 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > >
> > > Hello Andrey Konovalov,
> > >
> > > The patch f2c2e717642c: "usb: gadget: add raw-gadget interface" from
> > > Feb 24, 2020, leads to the following static checker warning:
> > >
> > > drivers/usb/gadget/legacy/raw_gadget.c:102 raw_event_queue_fetch() warn: inconsistent returns 'queue->sema'.
> > > Locked on : 96,102
> > > Unlocked on: 93
> > >
> > > drivers/usb/gadget/legacy/raw_gadget.c
> > > 81 static struct usb_raw_event *raw_event_queue_fetch(
> > > 82 struct raw_event_queue *queue)
> > > 83 {
> > > 84 unsigned long flags;
> > > 85 struct usb_raw_event *event;
> > > 86
> > > 87 /*
> > > 88 * This function can be called concurrently. We first check that
> > > 89 * there's at least one event queued by decrementing the semaphore,
> > > 90 * and then take the lock to protect queue struct fields.
> > > 91 */
> > > 92 if (down_interruptible(&queue->sema))
> > > 93 return NULL;
> > > 94 spin_lock_irqsave(&queue->lock, flags);
> > > 95 if (WARN_ON(!queue->size))
> > > 96 return NULL;
> > >
> > > I'm going to have investigate to see why Smatch doesn't complain that
> > > we have disabled IRQs on this path... Anyway, this doesn't seem like it
> > > can be correct. I get that this is a WARN_ON() path, but we're leaving
> > > the computer in a very screwed up state we don't at least enable IRQs.
> >
> > Hi Dan,
> >
> > Oh, right, I'll send a patch to add spin_lock_irqsave() there.
> >
> > I don't really get the warning about queue->sema though, is there
> > something wrong with it, or is it actually a warning about
> > queue->lock?
>
> The rule here is that we return NULL here and on line 93, so how does
> the caller know if we took that "queue->sema" lock?
OK, I see, will send a fix soon, thanks!
