Skip to contentSkip to conversation

USB: serial: metro-usb: fix unthrottle race

PATCH New Johan Hovold 1 participant 4 messages Jun 23

Subsystems (2)
USB USB serial
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

cover USB: serial: metro-usb: fix unthrottle race

This series fixes a mostly benign unthrottle race. Included is also preparatory patch replacing an unnecessary atomic allocation and a related variable declaration cleanup.

Johan

Johan Hovold (3):
USB: serial: metro-usb: replace unnecessary atomic allocation
USB: serial: metro-usb: fix unthrottle race
USB: serial: metro-usb: drop redundant initialisations

 drivers/usb/serial/metro-usb.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

PATCH 1/3 USB: serial: metro-usb: replace unnecessary atomic allocation

The unthrottle callback is allowed to sleep so pass the correct GFP flag to usb_submit_urb() to avoid unnecessary atomic allocations.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/metro-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/serial/metro-usb.c +1 -1
index 35473544f1c8..f42ad5dec35e 100644
@@ -329,7 +329,7 @@ static void metrousb_unthrottle(struct tty_struct *tty)
 	spin_unlock_irqrestore(&metro_priv->lock, flags);
 
 	/* Submit the urb to read from the port. */
-	result = usb_submit_urb(port->interrupt_in_urb, GFP_ATOMIC);
+	result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
 	if (result)
 		dev_err(&port->dev,
 			"failed submitting interrupt in urb error code=%d\n",
Signature · 1 line
2.53.0
author maintainer
#2 lore raw

PATCH 2/3 USB: serial: metro-usb: fix unthrottle race

In reply to Johan Hovold

If the completion handler races with unthrottle() both functions may try to resubmit the same interrupt-in urb, but at most one will succeed.

Fix the unthrottle logic using a throttle-requested flag so that only one attempt to resubmit the urb is made to avoid logging an error.

Fixes 43d186fe992d ("USB: serial: add metro-usb driver to the tree")
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/metro-usb.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)
drivers/usb/serial/metro-usb.c +17 -8
index f42ad5dec35e..8458713277f4 100644
@@ -36,6 +36,7 @@
 struct metrousb_private {
 	spinlock_t lock;
 	int throttled;
+	int throttle_req;
 	unsigned long control_state;
 };
 

@@ -143,7 +144,10 @@ static void metrousb_read_int_callback(struct urb *urb)
 
 	/* Set any port variables. */
 	spin_lock_irqsave(&metro_priv->lock, flags);
-	throttled = metro_priv->throttled;
+	if (metro_priv->throttle_req) {
+		metro_priv->throttled = 1;
+		throttled = 1;
+	}
 	spin_unlock_irqrestore(&metro_priv->lock, flags);
 
 	if (throttled)

@@ -175,6 +179,7 @@ static int metrousb_open(struct tty_struct *tty, struct usb_serial_port *port)
 	spin_lock_irqsave(&metro_priv->lock, flags);
 	metro_priv->control_state = 0;
 	metro_priv->throttled = 0;
+	metro_priv->throttle_req = 0;
 	spin_unlock_irqrestore(&metro_priv->lock, flags);
 
 	/* Clear the urb pipe. */

@@ -269,7 +274,7 @@ static void metrousb_throttle(struct tty_struct *tty)
 
 	/* Set the private information for the port to stop reading data. */
 	spin_lock_irqsave(&metro_priv->lock, flags);
-	metro_priv->throttled = 1;
+	metro_priv->throttle_req = 1;
 	spin_unlock_irqrestore(&metro_priv->lock, flags);
 }
 

@@ -321,19 +326,23 @@ static void metrousb_unthrottle(struct tty_struct *tty)
 	struct usb_serial_port *port = tty->driver_data;
 	struct metrousb_private *metro_priv = usb_get_serial_port_data(port);
 	unsigned long flags;
+	int throttled;
 	int result = 0;
 
 	/* Set the private information for the port to resume reading data. */
 	spin_lock_irqsave(&metro_priv->lock, flags);
+	throttled = metro_priv->throttled;
 	metro_priv->throttled = 0;
+	metro_priv->throttle_req = 0;
 	spin_unlock_irqrestore(&metro_priv->lock, flags);
 
-	/* Submit the urb to read from the port. */
-	result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
-	if (result)
-		dev_err(&port->dev,
-			"failed submitting interrupt in urb error code=%d\n",
-			result);
+	if (throttled) {
+		result = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
+		if (result) {
+			dev_err(&port->dev, "failed to submit interrupt in urb: %d\n",
+				result);
+		}
+	}
 }
 
 static struct usb_serial_driver metrousb_device = {
Signature · 1 line
2.53.0
author maintainer
#3 lore raw

PATCH 3/3 USB: serial: metro-usb: drop redundant initialisations

In reply to Johan Hovold

Three functions are initialising their return value variables at declaration only to later assign them unconditionally.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/metro-usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/usb/serial/metro-usb.c +3 -3
index 8458713277f4..22c5f071f116 100644
@@ -109,7 +109,7 @@ static void metrousb_read_int_callback(struct urb *urb)
 	unsigned char *data = urb->transfer_buffer;
 	unsigned long flags;
 	int throttled = 0;
-	int result = 0;
+	int result;
 
 	dev_dbg(&port->dev, "%s\n", __func__);
 

@@ -173,7 +173,7 @@ static int metrousb_open(struct tty_struct *tty, struct usb_serial_port *port)
 	struct usb_serial *serial = port->serial;
 	struct metrousb_private *metro_priv = usb_get_serial_port_data(port);
 	unsigned long flags;
-	int result = 0;
+	int result;
 
 	/* Set the private data information for the port. */
 	spin_lock_irqsave(&metro_priv->lock, flags);

@@ -327,7 +327,7 @@ static void metrousb_unthrottle(struct tty_struct *tty)
 	struct metrousb_private *metro_priv = usb_get_serial_port_data(port);
 	unsigned long flags;
 	int throttled;
-	int result = 0;
+	int result;
 
 	/* Set the private information for the port to resume reading data. */
 	spin_lock_irqsave(&metro_priv->lock, flags);
Signature · 1 line
2.53.0