usb: gadget: u_serial: Avoid double unlock of serial_port_lock
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
Quoted 31 lines · Dan Carpenter
Hello Prashanth K,
Commit
1c06aff9b82f("usb: gadget:u_serial: Avoid double unlock ofserial_port_lock") from May 6, 2025 (linux-next), leads to the following Smatch static checker warning:
drivers/usb/gadget/function/u_serial.c:1511gserial_suspend()
warn: duplicate check 'port' (previous on line 1498)
drivers/usb/gadget/function/u_serial.c
1490 voidgserial_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