usb: gadget: add raw-gadget interface

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.

    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  }

Quoted 28 lines · Dan Carpenter

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;

Quoted 4 lines

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?

Quoted 36 lines · Andrey Konovalov

Quoted 28 lines · Dan Carpenter

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;

Quoted 4 lines

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,

Quoted 5 lines

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?