[PATCH 0/3] USB: serial: metro-usb: fix unthrottle race

[PATCH 0/3] USB: serial: metro-usb: fix unthrottle race

[Johan Hovold]

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(-)

-- 
2.53.0

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

[Johan Hovold]

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 <johan@kernel.org>
---
 drivers/usb/serial/metro-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/metro-usb.c b/drivers/usb/serial/metro-usb.c
index 35473544f1c8..f42ad5dec35e 100644
--- a/drivers/usb/serial/metro-usb.c
+++ b/drivers/usb/serial/metro-usb.c
@@ -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",
-- 
2.53.0

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

[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 <johan@kernel.org>
---
 drivers/usb/serial/metro-usb.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/serial/metro-usb.c b/drivers/usb/serial/metro-usb.c
index f42ad5dec35e..8458713277f4 100644
--- a/drivers/usb/serial/metro-usb.c
+++ b/drivers/usb/serial/metro-usb.c
@@ -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 = {
-- 
2.53.0

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

[Johan Hovold]

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

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/metro-usb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/metro-usb.c b/drivers/usb/serial/metro-usb.c
index 8458713277f4..22c5f071f116 100644
--- a/drivers/usb/serial/metro-usb.c
+++ b/drivers/usb/serial/metro-usb.c
@@ -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);
-- 
2.53.0