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?
Thanks for the report!
>
> 97 event = queue->events[0];
> 98 queue->size--;
> 99 memmove(&queue->events[0], &queue->events[1],
> 100 queue->size * sizeof(queue->events[0]));
> 101 spin_unlock_irqrestore(&queue->lock, flags);
> 102 return event;
> 103 }
