Skip to contentSkip to conversation

usb: gadget: u_serial: Avoid double unlock of serial_port_lock

bug report Dan Carpenter 2 participants 2 messages May 26, 2025 – Jun 17, 2025 USB

lore
Share & tools
lore
#0 lore raw

Hello Prashanth K,

Commit 1c06aff9b82f ("usb: gadget: u_serial: Avoid double unlock of serial_port_lock") from May 6, 2025 (linux-next), leads to the following Smatch static checker warning:

drivers/usb/gadget/function/u_serial.c:1511 gserial_suspend()
warn: duplicate check 'port' (previous on line 1498)

drivers/usb/gadget/function/u_serial.c
1490 void gserial_suspend(struct gserial *gser) 1491 {

  1492          struct gs_port  *port;
  1493          unsigned long   flags;
  1494  
  1495          spin_lock_irqsave(&serial_port_lock, flags);
  1496          port = gser->ioport;
  1497  
  1498          if (!port) {
  1499                  spin_unlock_irqrestore(&serial_port_lock, flags);
  1500                  return;
  1501          }
  1502  
  1503          if (port->write_busy || port->write_started) {
  1504                  /* Wakeup to host if there are ongoing transfers */
  1505                  spin_unlock_irqrestore(&serial_port_lock, flags);
  1506                  if (!gserial_wakeup_host(gser))

1507 return;
1508
1509 /* Check if port is valid after acquiring lock back */

I should have caught this in review but this comment doesn't make sense. "port" is a local variable so it's going to be non-NULL.

  1510                  spin_lock_irqsave(&serial_port_lock, flags);

Maybe we should re-assign it? "port = gser->ioport;" before the NULL check? Otherwise we could just delete the NULL check.

  1511                  if (!port) {
  1512                          spin_unlock_irqrestore(&serial_port_lock, flags);
  1513                          return;
  1514                  }
  1515          }
  1516  
  1517          spin_lock(&port->port_lock);
  1518          spin_unlock(&serial_port_lock);
  1519          port->suspended = true;
  1520          port->start_delayed = true;
  1521          spin_unlock_irqrestore(&port->port_lock, flags);
  1522  }

regards,
dan carpenter

#1 lore raw
Quoted 31 lines · Dan Carpenter

Hello Prashanth K,

Commit 1c06aff9b82f ("usb: gadget: u_serial: Avoid double unlock of serial_port_lock") from May 6, 2025 (linux-next), leads to the following Smatch static checker warning:

drivers/usb/gadget/function/u_serial.c:1511 gserial_suspend()
warn: duplicate check 'port' (previous on line 1498)

drivers/usb/gadget/function/u_serial.c
1490 void gserial_suspend(struct gserial *gser) 1491 {

  1492          struct gs_port  *port;
  1493          unsigned long   flags;
  1494  
  1495          spin_lock_irqsave(&serial_port_lock, flags);
  1496          port = gser->ioport;
  1497  
  1498          if (!port) {
  1499                  spin_unlock_irqrestore(&serial_port_lock, flags);
  1500                  return;
  1501          }
  1502  
  1503          if (port->write_busy || port->write_started) {
  1504                  /* Wakeup to host if there are ongoing transfers */
  1505                  spin_unlock_irqrestore(&serial_port_lock, flags);
  1506                  if (!gserial_wakeup_host(gser))

1507 return;
1508
1509 /* Check if port is valid after acquiring lock back */

Quoted 7 lines

I should have caught this in review but this comment doesn't make sense. "port" is a local variable so it's going to be non-NULL.

  1510                  spin_lock_irqsave(&serial_port_lock, flags);

Maybe we should re-assign it? "port = gser->ioport;" before the NULL check? Otherwise we could just delete the NULL check.

Sorry for missing this. I over-analyzed this one, and since gserial_suspend() doesn't do much except setting few flags, we can remove the check altogether and just keep the spinlock. Can you send the fix?>

Quoted 4 lines
  1511                  if (!port) {
  1512                          spin_unlock_irqrestore(&serial_port_lock, flags);
  1513                          return;
  1514                  }
Quoted 9 lines
  1515          }
  1516  
  1517          spin_lock(&port->port_lock);
  1518          spin_unlock(&serial_port_lock);
  1519          port->suspended = true;
  1520          port->start_delayed = true;
  1521          spin_unlock_irqrestore(&port->port_lock, flags);
  1522  }

Regards,
Prashanth K