[V2,03/13] USB: serial: keyspan_pda: Consolidate room query

Commit Message

Thomas Gleixner Oct. 19, 2020, 10:06 a.m. UTC

From: Sebastian Andrzej Siewior <[e-mail]>

Having two copies of the same code doesn't make the code more readable and
allocating a buffer of 1 byte for a synchronous operation is a pointless
exercise.

Allocate a byte buffer at init which can be used instead. The buffer is
only used in open() and tty->write(). Console writes are not calling into
the query. open() obviously happens before write() and the writes are
serialized by bit 0 of port->write_urbs_free which protects also the
transaction itself.

Move the actual query into a helper function and cleanup the usage sites in
keyspan_pda_write() and keyspan_pda_open().

Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
Signed-off-by: Thomas Gleixner <[e-mail]>
Cc: Johan Hovold <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: [e-mail]
---
V2; Allocate a real buffer (Alan)
---
 drivers/usb/serial/keyspan_pda.c |  108 +++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 59 deletions(-)

Comments

Johan Hovold Oct. 25, 2020, 5:05 p.m. UTC | #1

On Mon, Oct 19, 2020 at 12:06:32PM +0200, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <[e-mail]>
> 
> Having two copies of the same code doesn't make the code more readable and
> allocating a buffer of 1 byte for a synchronous operation is a pointless
> exercise.

As Alan pointed out, this buffer is in fact required and not pointless
at all even if reallocating it may be suboptimal.

Note however that there are several further allocations done by
usb_control_msg() for each control request.

> Allocate a byte buffer at init which can be used instead. The buffer is
> only used in open() and tty->write(). Console writes are not calling into
> the query. open() obviously happens before write() and the writes are
> serialized by bit 0 of port->write_urbs_free which protects also the
> transaction itself.

As I mentioned in my comments to the previous patch, I've rewritten the
driver so that is no longer does this query in the write path. I kept
the buffer allocation for now though in case you want to rework this one
top.

Johan

Patch

--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -47,6 +47,7 @@  struct keyspan_pda_private {
 	struct work_struct			unthrottle_work;
 	struct usb_serial	*serial;
 	struct usb_serial_port	*port;
+	u8			*query_buf;
 };
 
 
@@ -436,6 +437,31 @@  static int keyspan_pda_tiocmset(struct t
 	return rc;
 }
 
+/*
+ * Using priv->query_buf is safe here because this is only called for TTY
+ * operations open() and write(). write() comes post open() obviously and
+ * write() itself is serialized via bit 0 of port->write_urbs_free. Console
+ * writes are never calling into this.
+ */
+static int keyspan_pda_query_room(struct usb_serial *serial,
+				  struct keyspan_pda_private *priv)
+{
+	int res;
+
+	res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+			      6, /* write_room */
+			      USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN,
+			      0, /* value */
+			      0, /* index */
+			      priv->query_buf,
+			      1,
+			      2000);
+	if (res != 1)
+		return res < 0 ? res : -EIO;
+
+	return (unsigned int)*priv->query_buf;
+}
+
 static int keyspan_pda_write(struct tty_struct *tty,
 	struct usb_serial_port *port, const unsigned char *buf, int count)
 {
@@ -483,39 +509,16 @@  static int keyspan_pda_write(struct tty_
 	 * usage because the console write can't sleep.
 	 */
 	if (count > priv->tx_room && tty) {
-		u8 *room;
-
-		room = kmalloc(1, GFP_KERNEL);
-		if (!room) {
-			rc = -ENOMEM;
-			goto exit;
-		}
-
-		rc = usb_control_msg(serial->dev,
-				     usb_rcvctrlpipe(serial->dev, 0),
-				     6, /* write_room */
-				     USB_TYPE_VENDOR | USB_RECIP_INTERFACE
-				     | USB_DIR_IN,
-				     0, /* value: 0 means "remaining room" */
-				     0, /* index */
-				     room,
-				     1,
-				     2000);
-		if (rc > 0) {
-			dev_dbg(&port->dev, "roomquery says %d\n", *room);
-			priv->tx_room = *room;
-		}
-		kfree(room);
+		rc = keyspan_pda_query_room(serial, priv);
 		if (rc < 0) {
-			dev_dbg(&port->dev, "roomquery failed\n");
-			goto exit;
-		}
-		if (rc == 0) {
-			dev_dbg(&port->dev, "roomquery returned 0 bytes\n");
-			rc = -EIO; /* device didn't return any data */
+			dev_dbg(&port->dev, "roomquery failed %d\n", rc);
 			goto exit;
 		}
+
+		dev_dbg(&port->dev, "roomquery says %d\n", rc);
+		priv->tx_room = rc;
 	}
+
 	if (count > priv->tx_room) {
 		/* we're about to completely fill the Tx buffer, so
 		   we'll be throttled afterwards. */
@@ -615,45 +618,26 @@  static int keyspan_pda_open(struct tty_s
 					struct usb_serial_port *port)
 {
 	struct usb_serial *serial = port->serial;
-	u8 *room;
-	int rc = 0;
 	struct keyspan_pda_private *priv;
+	int rc;
 
-	/* find out how much room is in the Tx ring */
-	room = kmalloc(1, GFP_KERNEL);
-	if (!room)
-		return -ENOMEM;
+	priv = usb_get_serial_port_data(port);
 
-	rc = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-			     6, /* write_room */
-			     USB_TYPE_VENDOR | USB_RECIP_INTERFACE
-			     | USB_DIR_IN,
-			     0, /* value */
-			     0, /* index */
-			     room,
-			     1,
-			     2000);
+	/* find out how much room is in the Tx ring */
+	rc = keyspan_pda_query_room(serial, priv);
 	if (rc < 0) {
-		dev_dbg(&port->dev, "%s - roomquery failed\n", __func__);
-		goto error;
-	}
-	if (rc == 0) {
-		dev_dbg(&port->dev, "%s - roomquery returned 0 bytes\n", __func__);
-		rc = -EIO;
-		goto error;
+		dev_dbg(&port->dev, "roomquery failed %d\n", rc);
+		return rc;
 	}
-	priv = usb_get_serial_port_data(port);
-	priv->tx_room = *room;
-	priv->tx_throttled = *room ? 0 : 1;
+
+	priv->tx_room = rc;
+	priv->tx_throttled = rc ? 0 : 1;
 
 	/*Start reading from the device*/
 	rc = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
-	if (rc) {
+	if (rc)
 		dev_dbg(&port->dev, "%s - usb_submit_urb(read int) failed\n", __func__);
-		goto error;
-	}
-error:
-	kfree(room);
+
 	return rc;
 }
 static void keyspan_pda_close(struct usb_serial_port *port)
@@ -715,6 +699,11 @@  static int keyspan_pda_port_probe(struct
 	priv = kmalloc(sizeof(struct keyspan_pda_private), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
+	priv->query_buf = kmalloc(1, GFP_KERNEL);
+	if (!priv->query_buf) {
+		kfree(priv);
+		return -ENOMEM;
+	}
 
 	INIT_WORK(&priv->wakeup_work, keyspan_pda_wakeup_write);
 	INIT_WORK(&priv->unthrottle_work, keyspan_pda_request_unthrottle);
@@ -731,6 +720,7 @@  static int keyspan_pda_port_remove(struc
 	struct keyspan_pda_private *priv;
 
 	priv = usb_get_serial_port_data(port);
+	kfree(priv->query_buf);
 	kfree(priv);
 
 	return 0;