USB: serial: buffer-callback clean ups

PATCH In review Johan Hovold 2 participants 9 messages May 19, 2021 – May 21, 2021

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Johan Hovold

author maintainer

2021-05-19 09:20 UTC #0 lore raw

cover USB: serial: buffer-callback clean ups

This series clean up a few things related to the chars_in_buffer and write_room callbacks that were found during review of the recent conversion to have these callbacks return an unsigned int.

Johan

Johan Hovold (6):
USB: serial: digi_acceleport: reduce chars_in_buffer over-reporting
USB: serial: digi_acceleport: add chars_in_buffer locking
USB: serial: io_edgeport: drop buffer-callback sanity checks
USB: serial: mos7720: drop buffer-callback sanity checks
USB: serial: mos7840: drop buffer-callback return-value comments
USB: serial: drop irq-flags initialisations

 drivers/usb/serial/digi_acceleport.c | 34 ++++++++++++++--------------
 drivers/usb/serial/io_edgeport.c     | 27 +---------------------
 drivers/usb/serial/metro-usb.c       | 12 +++++-----
 drivers/usb/serial/mos7720.c         | 17 ++------------
 drivers/usb/serial/mos7840.c         |  5 ----
 drivers/usb/serial/quatech2.c        |  2 +-
 6 files changed, 27 insertions(+), 70 deletions(-)

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-05-19 09:20 UTC #1 lore raw

PATCH 1/6 USB: serial: digi_acceleport: reduce chars_in_buffer over-reporting

Due to an ancient quirk in n_tty poll implementation, the digi_acceleport driver has been reporting that its queue contains 256 (WAKEUP_CHARS) characters whenever its write URB is in use.

This has not been necessary since 2003 when the line-discipline started taking the write room into account so let's return the maximum transfer size again in order to over-report a little less and incidentally fix the related debug statement.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/digi_acceleport.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/usb/serial/digi_acceleport.c +1 -2

index 19ee8191647c..a4194b70a6fe 100644
@@ -1048,8 +1048,7 @@ static unsigned int digi_chars_in_buffer(struct tty_struct *tty)
 	if (priv->dp_write_urb_in_use) {
 		dev_dbg(&port->dev, "digi_chars_in_buffer: port=%d, chars=%d\n",
 			priv->dp_port_num, port->bulk_out_size - 2);
-		/* return(port->bulk_out_size - 2); */
-		return 256;
+		return port->bulk_out_size - 2;
 	} else {
 		dev_dbg(&port->dev, "digi_chars_in_buffer: port=%d, chars=%d\n",
 			priv->dp_port_num, priv->dp_out_buf_len);

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-05-19 09:20 UTC #2 lore raw

PATCH 2/6 USB: serial: digi_acceleport: add chars_in_buffer locking

In reply to Johan Hovold

Both the dp_write_urb_in_use flag and dp_out_buf_len counter should be accessed while holding the driver port lock. Add the missing locking to chars_in_buffer and clean up the implementation somewhat by using a common exit path.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/digi_acceleport.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

drivers/usb/serial/digi_acceleport.c +11 -9

index a4194b70a6fe..754c66ff0fc1 100644
@@ -1044,17 +1044,19 @@ static unsigned int digi_chars_in_buffer(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct digi_port *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+	unsigned int chars;
 
-	if (priv->dp_write_urb_in_use) {
-		dev_dbg(&port->dev, "digi_chars_in_buffer: port=%d, chars=%d\n",
-			priv->dp_port_num, port->bulk_out_size - 2);
-		return port->bulk_out_size - 2;
-	} else {
-		dev_dbg(&port->dev, "digi_chars_in_buffer: port=%d, chars=%d\n",
-			priv->dp_port_num, priv->dp_out_buf_len);
-		return priv->dp_out_buf_len;
-	}
+	spin_lock_irqsave(&priv->dp_port_lock, flags);
+	if (priv->dp_write_urb_in_use)
+		chars = port->bulk_out_size - 2;
+	else
+		chars = priv->dp_out_buf_len;
+	spin_unlock_irqrestore(&priv->dp_port_lock, flags);
 
+	dev_dbg(&port->dev, "%s: port=%d, chars=%d\n", __func__,
+			priv->dp_port_num, chars);
+	return chars;
 }
 
 static void digi_dtr_rts(struct usb_serial_port *port, int on)

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-05-19 09:20 UTC #3 lore raw

PATCH 3/6 USB: serial: io_edgeport: drop buffer-callback sanity checks

In reply to Johan Hovold

The driver write_room and chars_in_buffer callbacks used to incorrectly return a negative errno in case they were called while or after the port had been closed. The return value was later changed to zero by commit d76f2f4462bb ("io_edgeport: Fix various bogus returns to the tty layer") but the bogus sanity checks were left in place as were the outdated function-header comments.

These callbacks will never be called for an uninitialised port so drop the unnecessary sanity checks and the outdated comments.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/io_edgeport.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

drivers/usb/serial/io_edgeport.c +1 ~~-26~~

index 1f8dff4390c7..ea4edf5eed27 100644
@@ -1351,9 +1351,7 @@ static void send_more_port_data(struct edgeport_serial *edge_serial,
 /*****************************************************************************
  * edge_write_room
  *	this function is called by the tty driver when it wants to know how
- *	many bytes of data we can accept for a specific port. If successful,
- *	we return the amount of room that we have for this port	(the txCredits)
- *	otherwise we return a negative error number.
+ *	many bytes of data we can accept for a specific port.
  *****************************************************************************/
 static unsigned int edge_write_room(struct tty_struct *tty)
 {

@@ -1362,16 +1360,6 @@ static unsigned int edge_write_room(struct tty_struct *tty)
 	unsigned int room;
 	unsigned long flags;
 
-	if (edge_port == NULL)
-		return 0;
-	if (edge_port->closePending)
-		return 0;
-
-	if (!edge_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return 0;
-	}
-
 	/* total of both buffers is still txCredit */
 	spin_lock_irqsave(&edge_port->ep_lock, flags);
 	room = edge_port->txCredits - edge_port->txfifo.count;

@@ -1387,9 +1375,6 @@ static unsigned int edge_write_room(struct tty_struct *tty)
  *	this function is called by the tty driver when it wants to know how
  *	many bytes of data we currently have outstanding in the port (data that
  *	has been written, but hasn't made it out the port yet)
- *	If successful, we return the number of bytes left to be written in the
- *	system,
- *	Otherwise we return a negative error number.
  *****************************************************************************/
 static unsigned int edge_chars_in_buffer(struct tty_struct *tty)
 {

@@ -1398,16 +1383,6 @@ static unsigned int edge_chars_in_buffer(struct tty_struct *tty)
 	unsigned int num_chars;
 	unsigned long flags;
 
-	if (edge_port == NULL)
-		return 0;
-	if (edge_port->closePending)
-		return 0;
-
-	if (!edge_port->open) {
-		dev_dbg(&port->dev, "%s - port not opened\n", __func__);
-		return 0;
-	}
-
 	spin_lock_irqsave(&edge_port->ep_lock, flags);
 	num_chars = edge_port->maxTxCredits - edge_port->txCredits +
 						edge_port->txfifo.count;

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-05-19 09:20 UTC #4 lore raw

PATCH 4/6 USB: serial: mos7720: drop buffer-callback sanity checks

In reply to Johan Hovold

The driver write_room and chars_in_buffer callbacks used to incorrectly return a negative errno in case they were ever called with a NULL port driver-data pointer. The return value was later changed to zero by commit 23198fda7182 ("tty: fix chars_in_buffers") but the bogus sanity checks were left in place as were the outdated function-header comments.

The port driver data isn't cleared until after the port has been deregistered and all open ttys have been hung up so drop the unnecessary sanity checks and the outdated comments.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/mos7720.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

drivers/usb/serial/mos7720.c +2 ~~-15~~

index ce41009756f3..227f43d2bd56 100644
@@ -945,20 +945,13 @@ static int mos7720_open(struct tty_struct *tty, struct usb_serial_port *port)
  *	this function is called by the tty driver when it wants to know how many
  *	bytes of data we currently have outstanding in the port (data that has
  *	been written, but hasn't made it out the port yet)
- *	If successful, we return the number of bytes left to be written in the
- *	system,
- *	Otherwise we return a negative error number.
  */
 static unsigned int mos7720_chars_in_buffer(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
+	struct moschip_port *mos7720_port = usb_get_serial_port_data(port);
 	int i;
 	unsigned int chars = 0;
-	struct moschip_port *mos7720_port;
-
-	mos7720_port = usb_get_serial_port_data(port);
-	if (mos7720_port == NULL)
-		return 0;
 
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (mos7720_port->write_urb_pool[i] &&

@@ -1030,20 +1023,14 @@ static void mos7720_break(struct tty_struct *tty, int break_state)
  * mos7720_write_room
  *	this function is called by the tty driver when it wants to know how many
  *	bytes of data we can accept for a specific port.
- *	If successful, we return the amount of room that we have for this port
- *	Otherwise we return a negative error number.
  */
 static unsigned int mos7720_write_room(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct moschip_port *mos7720_port;
+	struct moschip_port *mos7720_port = usb_get_serial_port_data(port);
 	unsigned int room = 0;
 	int i;
 
-	mos7720_port = usb_get_serial_port_data(port);
-	if (mos7720_port == NULL)
-		return 0;
-
 	/* FIXME: Locking */
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (mos7720_port->write_urb_pool[i] &&

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-05-19 09:20 UTC #5 lore raw

PATCH 5/6 USB: serial: mos7840: drop buffer-callback return-value comments

In reply to Johan Hovold

The driver write_room and chars_in_buffer callbacks used to incorrectly return a negative errno in case they were called with a NULL port driver-data pointer or if some other always-true sanity checks failed.

The bogus sanity checks were later removed by commit ce039bd4b21f ("USB:
serial: mos7840: drop paranoid port checks") and 7b2faede671a ("USB:
serial: mos7840: drop port driver data accessors") but the function-header comments were never updated to match.

Drop the outdated return-value comments.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/mos7840.c | 5 -----
 1 file changed, 5 deletions(-)

drivers/usb/serial/mos7840.c +0 -5

index b22ccbd98998..d7fe33ca73e4 100644
@@ -730,9 +730,6 @@ static int mos7840_open(struct tty_struct *tty, struct usb_serial_port *port)
  *	this function is called by the tty driver when it wants to know how many
  *	bytes of data we currently have outstanding in the port (data that has
  *	been written, but hasn't made it out the port yet)
- *	If successful, we return the number of bytes left to be written in the
- *	system,
- *	Otherwise we return zero.
  *****************************************************************************/
 
 static unsigned int mos7840_chars_in_buffer(struct tty_struct *tty)

@@ -814,8 +811,6 @@ static void mos7840_break(struct tty_struct *tty, int break_state)
  * mos7840_write_room
  *	this function is called by the tty driver when it wants to know how many
  *	bytes of data we can accept for a specific port.
- *	If successful, we return the amount of room that we have for this port
- *	Otherwise we return a negative error number.
  *****************************************************************************/
 
 static unsigned int mos7840_write_room(struct tty_struct *tty)

Signature · 1 line

2.26.3

Johan Hovold

author maintainer

2021-05-19 09:20 UTC #6 lore raw

PATCH 6/6 USB: serial: drop irq-flags initialisations

In reply to Johan Hovold

There's no need to initialise irq-flags variables before saving the interrupt state.

Drop the redundant initialisations from the three drivers that got this wrong.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/digi_acceleport.c | 13 ++++++-------
 drivers/usb/serial/metro-usb.c       | 12 ++++++------
 drivers/usb/serial/quatech2.c        |  2 +-
 3 files changed, 13 insertions(+), 14 deletions(-)

drivers/usb/serial/digi_acceleport.c +6 -7

index 754c66ff0fc1..af65eb863d70 100644
@@ -372,7 +372,7 @@ static int digi_write_oob_command(struct usb_serial_port *port,
 	int len;
 	struct usb_serial_port *oob_port = (struct usb_serial_port *)((struct digi_serial *)(usb_get_serial_data(port->serial)))->ds_oob_port;
 	struct digi_port *oob_priv = usb_get_serial_port_data(oob_port);
-	unsigned long flags = 0;
+	unsigned long flags;
 
 	dev_dbg(&port->dev,
 		"digi_write_oob_command: TOP: port=%d, count=%d\n",

@@ -430,7 +430,7 @@ static int digi_write_inb_command(struct usb_serial_port *port,
 	int len;
 	struct digi_port *priv = usb_get_serial_port_data(port);
 	unsigned char *data = port->write_urb->transfer_buffer;
-	unsigned long flags = 0;
+	unsigned long flags;
 
 	dev_dbg(&port->dev, "digi_write_inb_command: TOP: port=%d, count=%d\n",
 		priv->dp_port_num, count);

@@ -511,8 +511,7 @@ static int digi_set_modem_signals(struct usb_serial_port *port,
 	struct usb_serial_port *oob_port = (struct usb_serial_port *) ((struct digi_serial *)(usb_get_serial_data(port->serial)))->ds_oob_port;
 	struct digi_port *oob_priv = usb_get_serial_port_data(oob_port);
 	unsigned char *data = oob_port->write_urb->transfer_buffer;
-	unsigned long flags = 0;
-
+	unsigned long flags;
 
 	dev_dbg(&port->dev,
 		"digi_set_modem_signals: TOP: port=%d, modem_signals=0x%x\n",

@@ -577,7 +576,7 @@ static int digi_transmit_idle(struct usb_serial_port *port,
 	int ret;
 	unsigned char buf[2];
 	struct digi_port *priv = usb_get_serial_port_data(port);
-	unsigned long flags = 0;
+	unsigned long flags;
 
 	spin_lock_irqsave(&priv->dp_port_lock, flags);
 	priv->dp_transmit_idle = 0;

@@ -887,7 +886,7 @@ static int digi_write(struct tty_struct *tty, struct usb_serial_port *port,
 	int ret, data_len, new_len;
 	struct digi_port *priv = usb_get_serial_port_data(port);
 	unsigned char *data = port->write_urb->transfer_buffer;
-	unsigned long flags = 0;
+	unsigned long flags;
 
 	dev_dbg(&port->dev, "digi_write: TOP: port=%d, count=%d\n",
 		priv->dp_port_num, count);

@@ -1024,8 +1023,8 @@ static unsigned int digi_write_room(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct digi_port *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
 	unsigned int room;
-	unsigned long flags = 0;
 
 	spin_lock_irqsave(&priv->dp_port_lock, flags);

drivers/usb/serial/metro-usb.c +6 -6

index f9ce9e7b9b80..30ab565e0738 100644
@@ -109,9 +109,9 @@ static void metrousb_read_int_callback(struct urb *urb)
 	struct usb_serial_port *port = urb->context;
 	struct metrousb_private *metro_priv = usb_get_serial_port_data(port);
 	unsigned char *data = urb->transfer_buffer;
+	unsigned long flags;
 	int throttled = 0;
 	int result = 0;
-	unsigned long flags = 0;
 
 	dev_dbg(&port->dev, "%s\n", __func__);

@@ -171,7 +171,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 = 0;
+	unsigned long flags;
 	int result = 0;
 
 	/* Set the private data information for the port. */

@@ -268,7 +268,7 @@ static void metrousb_throttle(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 = 0;
+	unsigned long flags;
 
 	/* Set the private information for the port to stop reading data. */
 	spin_lock_irqsave(&metro_priv->lock, flags);

@@ -281,7 +281,7 @@ static int metrousb_tiocmget(struct tty_struct *tty)
 	unsigned long control_state = 0;
 	struct usb_serial_port *port = tty->driver_data;
 	struct metrousb_private *metro_priv = usb_get_serial_port_data(port);
-	unsigned long flags = 0;
+	unsigned long flags;
 
 	spin_lock_irqsave(&metro_priv->lock, flags);
 	control_state = metro_priv->control_state;

@@ -296,7 +296,7 @@ static int metrousb_tiocmset(struct tty_struct *tty,
 	struct usb_serial_port *port = tty->driver_data;
 	struct usb_serial *serial = port->serial;
 	struct metrousb_private *metro_priv = usb_get_serial_port_data(port);
-	unsigned long flags = 0;
+	unsigned long flags;
 	unsigned long control_state = 0;
 
 	dev_dbg(&port->dev, "%s - set=%d, clear=%d\n", __func__, set, clear);

@@ -323,7 +323,7 @@ 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 = 0;
+	unsigned long flags;
 	int result = 0;
 
 	/* Set the private information for the port to resume reading data. */

drivers/usb/serial/quatech2.c +1 -1

index 3b5f2032ecdb..d3377f3b5a23 100644
@@ -874,7 +874,7 @@ static unsigned int qt2_write_room(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct qt2_port_private *port_priv;
-	unsigned long flags = 0;
+	unsigned long flags;
 	unsigned int r;
 
 	port_priv = usb_get_serial_port_data(port);

Signature · 1 line

2.26.3

Greg Kroah-Hartman

maintainer Reviewed-by on the v1 series

2021-05-21 12:22 UTC #7 lore raw

In reply to Johan Hovold

Quoted 15 lines · Johan Hovold

This series clean up a few things related to the chars_in_buffer and write_room callbacks that were found during review of the recent conversion to have these callbacks return an unsigned int.
Johan
Johan Hovold (6):
USB: serial: digi_acceleport: reduce chars_in_buffer over-reporting
USB: serial: digi_acceleport: add chars_in_buffer locking
USB: serial: io_edgeport: drop buffer-callback sanity checks
USB: serial: mos7720: drop buffer-callback sanity checks
USB: serial: mos7840: drop buffer-callback return-value comments
USB: serial: drop irq-flags initialisations

Quoted 7 lines

 drivers/usb/serial/digi_acceleport.c | 34 ++++++++++++++--------------
 drivers/usb/serial/io_edgeport.c     | 27 +---------------------
 drivers/usb/serial/metro-usb.c       | 12 +++++-----
 drivers/usb/serial/mos7720.c         | 17 ++------------
 drivers/usb/serial/mos7840.c         |  5 ----
 drivers/usb/serial/quatech2.c        |  2 +-
 6 files changed, 27 insertions(+), 70 deletions(-)

Reviewed-by Greg Kroah-Hartman <[email protected]>

Johan Hovold

author maintainer

2021-05-21 13:48 UTC #8 lore raw

Quoted 24 lines · Greg Kroah-Hartman

Quoted 15 lines · Johan Hovold
This series clean up a few things related to the chars_in_buffer and write_room callbacks that were found during review of the recent conversion to have these callbacks return an unsigned int.
Johan
Johan Hovold (6):
USB: serial: digi_acceleport: reduce chars_in_buffer over-reporting
USB: serial: digi_acceleport: add chars_in_buffer locking
USB: serial: io_edgeport: drop buffer-callback sanity checks
USB: serial: mos7720: drop buffer-callback sanity checks
USB: serial: mos7840: drop buffer-callback return-value comments
USB: serial: drop irq-flags initialisations
Quoted 7 lines
 drivers/usb/serial/digi_acceleport.c | 34 ++++++++++++++--------------
 drivers/usb/serial/io_edgeport.c     | 27 +---------------------
 drivers/usb/serial/metro-usb.c       | 12 +++++-----
 drivers/usb/serial/mos7720.c         | 17 ++------------
 drivers/usb/serial/mos7840.c         |  5 ----
 drivers/usb/serial/quatech2.c        |  2 +-
 6 files changed, 27 insertions(+), 70 deletions(-)

Quoted 1 line

Reviewed-by Greg Kroah-Hartman <[email protected]>

Thanks for reviewing. Now applied.

Johan