usb: gadget: u_serial: Implement remote wakeup capability

bug report Dan Carpenter 2 participants 3 messages Apr 30, 2025 USB

Share & tools

Dan Carpenter

author

2025-04-30 08:09 UTC #0 lore raw

Hello Prashanth K,

Commit 3baea29dc0a7 ("usb: gadget: u_serial: Implement remote wakeup capability") from Apr 24, 2025 (linux-next), leads to the following Smatch static checker warning:

drivers/usb/gadget/function/u_serial.c:1511 gserial_suspend() warn: double unlock 'global &serial_port_lock' (orig line 1505) drivers/usb/gadget/function/u_serial.c:1514 gserial_suspend() warn: double unlock 'flags' (orig line 1505)

drivers/usb/gadget/function/u_serial.c

    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);
                                                 ^^^^^^^^^^^^^^^^  ^^^^^

We unlock here.

1506 if (!gserial_wakeup_host(gser))

Assume gserial_wakeup_host() fails so we don't return on the next line.

    1507                         return;
    1508         }
    1509 
    1510         spin_lock(&port->port_lock);
--> 1511         spin_unlock(&serial_port_lock);
                             ^^^^^^^^^^^^^^^^^

Double unlock.

    1512         port->suspended = true;
    1513         port->start_delayed = true;
    1514         spin_unlock_irqrestore(&port->port_lock, flags);
                                                          ^^^^^

IRQs were already enabled. Which is probably fine, I don't know.

    1515 }

regards,
dan carpenter

Prashanth K

2025-04-30 10:36 UTC #1 lore raw

Quoted 34 lines · Dan Carpenter

Hello Prashanth K,
Commit 3baea29dc0a7 ("usb: gadget: u_serial: Implement remote wakeup capability") from Apr 24, 2025 (linux-next), leads to the following Smatch static checker warning:
drivers/usb/gadget/function/u_serial.c:1511 gserial_suspend() warn: double unlock 'global &serial_port_lock' (orig line 1505) drivers/usb/gadget/function/u_serial.c:1514 gserial_suspend() warn: double unlock 'flags' (orig line 1505)
drivers/usb/gadget/function/u_serial.c
    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);
                                                 ^^^^^^^^^^^^^^^^  ^^^^^
We unlock here.
1506 if (!gserial_wakeup_host(gser))
Assume gserial_wakeup_host() fails so we don't return on the next line.
    1507                         return;
    1508         }
    1509 
    1510         spin_lock(&port->port_lock);
--> 1511         spin_unlock(&serial_port_lock);
                             ^^^^^^^^^^^^^^^^^
Double unlock.
    1512         port->suspended = true;
    1513         port->start_delayed = true;
    1514         spin_unlock_irqrestore(&port->port_lock, flags);
                                                          ^^^^^

Quoted 6 lines

IRQs were already enabled. Which is probably fine, I don't know.
    1515 }
regards,
dan carpenter

Can you send a patch, or should i fix it? Let me know. I think we can rearrange the locks also, something like this,

(quoted diff) +0 -0

b/drivers/usb/gadget/function/u_serial.c
index 41dee7c8cc7c..1f182fce071a 100644

drivers/usb/gadget/function/u_serial.c +4 -3

@@ -1500,15 +1500,16 @@ void gserial_suspend(struct gserial *gser)
                return;
        }
 
+       spin_lock(&port->port_lock);
+       spin_unlock(&serial_port_lock);
        if (port->write_busy || port->write_started) {
                /* Wakeup to host if there are ongoing transfers */
-               spin_unlock_irqrestore(&serial_port_lock, flags);
+               spin_unlock_irqrestore(&port->port_lock, flags);
                if (!gserial_wakeup_host(gser))
                        return;
+               spin_lock_irqsave(&port->port_lock, flags);
        }
 
-       spin_lock(&port->port_lock);
-       spin_unlock(&serial_port_lock);
        port->suspended = true;
        port->start_delayed = true;
        spin_unlock_irqrestore(&port->port_lock, flags);

Regards,
Prashanth K

Dan Carpenter

author

2025-04-30 10:43 UTC #2 lore raw

Quoted 39 lines · Prashanth K

Quoted 36 lines · Dan Carpenter
Hello Prashanth K,
Commit 3baea29dc0a7 ("usb: gadget: u_serial: Implement remote wakeup capability") from Apr 24, 2025 (linux-next), leads to the following Smatch static checker warning:
drivers/usb/gadget/function/u_serial.c:1511 gserial_suspend() warn: double unlock 'global &serial_port_lock' (orig line 1505) drivers/usb/gadget/function/u_serial.c:1514 gserial_suspend() warn: double unlock 'flags' (orig line 1505)
drivers/usb/gadget/function/u_serial.c
    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);
                                                 ^^^^^^^^^^^^^^^^  ^^^^^
We unlock here.
1506 if (!gserial_wakeup_host(gser))
Assume gserial_wakeup_host() fails so we don't return on the next line.
    1507                         return;
    1508         }
    1509 
    1510         spin_lock(&port->port_lock);
--> 1511         spin_unlock(&serial_port_lock);
                             ^^^^^^^^^^^^^^^^^
Double unlock.
    1512         port->suspended = true;
    1513         port->start_delayed = true;
    1514         spin_unlock_irqrestore(&port->port_lock, flags);
                                                          ^^^^^
IRQs were already enabled. Which is probably fine, I don't know.

Quoted 6 lines

Quoted 4 lines
    1515 }
regards,
dan carpenter
Can you send a patch, or should i fix it? Let me know.

Could you could send the patch? Thanks!

regards,
dan carpenter