[bug report] usb: gadget: u_serial: Avoid double unlock of serial_port_lock

[bug report] usb: gadget: u_serial: Avoid double unlock of serial_port_lock

[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 */

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

Re: [bug report] usb: gadget: u_serial: Avoid double unlock of serial_port_lock

[Prashanth K]

On 5/26/2025 4:42 PM, Dan Carpenter wrote:
> 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.
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?>
>   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,
Prashanth K