Skip to contentSkip to conversation

USB: serial: digi_acceleport: registration fix and cleanups

PATCH New Johan Hovold 1 participant 13 messages Jun 23

Subsystems (3)
Stable branch USB USB serial
lore
Share & tools
lore

Johan Hovold

author maintainer
#0 lore raw

v2 cover USB: serial: digi_acceleport: registration fix and cleanups

This series fixes a port registration ordering issue and makes the driver stop all I/O when the device is not in use, while cleaning up the driver somewhat.

Johan

Changes in v2:
- fix broken rx after throttle (new)
- rebase on fix now in mainline

Johan Hovold (12):
USB: serial: digi_acceleport: fix broken rx after throttle
USB: serial: digi_acceleport: fix port registration order
USB: serial: digi_acceleport: drop unused wait queue
USB: serial: digi_acceleport: always stop write urb on close
USB: serial: digi_acceleport: add oob port helper
USB: serial: digi_acceleport: clean up declarations and whitespace
USB: serial: digi_acceleport: drop redundant driver data sanity checks
USB: serial: digi_acceleport: stop OOB I/O when not in use
USB: serial: digi_acceleport: drop unused in-buf define
USB: serial: digi_acceleport: clean up xfer buf length expression
USB: serial: digi_acceleport: clean up write completion
USB: serial: digi_acceleport: clean up inb command submission

 drivers/usb/serial/digi_acceleport.c | 304 +++++++++++----------------
 1 file changed, 122 insertions(+), 182 deletions(-)
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

PATCH v2 1/12 USB: serial: digi_acceleport: fix broken rx after throttle

If the port is closed while throttled, the read urb is never resubmitted and the port will not receive any further data until the device is reconnected (or the driver is rebound).

Clear the throttle flags and submit the urb if needed when opening the port.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/usb/serial/digi_acceleport.c +16 -0
index 6899aebfd6ae..097769525ca4 100644
@@ -1069,6 +1069,7 @@ static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	unsigned char buf[32];
 	struct digi_port *priv = usb_get_serial_port_data(port);
 	struct ktermios not_termios;
+	int throttled;
 
 	/* be sure the device is started up */
 	if (digi_startup_device(port->serial) != 0)

@@ -1096,6 +1097,21 @@ static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 		not_termios.c_iflag = ~tty->termios.c_iflag;
 		digi_set_termios(tty, port, &not_termios);
 	}
+
+	spin_lock_irq(&priv->dp_port_lock);
+	throttled = priv->dp_throttle_restart;
+	priv->dp_throttled = 0;
+	priv->dp_throttle_restart = 0;
+	spin_unlock_irq(&priv->dp_port_lock);
+
+	if (throttled) {
+		ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
+		if (ret) {
+			dev_err(&port->dev, "failed to submit read urb: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return 0;
 }
Signature · 1 line
2.53.0
author maintainer
#2 lore raw

PATCH v2 2/12 USB: serial: digi_acceleport: fix port registration order

In reply to Johan Hovold

The driver submits the read urbs for all ports when the first port is opened, which could happen before the other ports have been probed and their private data set up.

If such an urb completes before the port has been probed, the completion handler will not resubmit it, thus preventing any further reads.

Fix the ordering issue by not submitting the port read urbs until the port is opened.

This also avoids wasting resources (e.g. power) when ports are not in use.

Note that the port write urbs are already stopped on close (unless unbinding, but they are also stopped by core on disconnect).

Fixes fb44ff854e14 ("USB: digi_acceleport: fix port-data memory leak")
Cc [email protected] # 3.7
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 48 +++++++++++-----------------
 1 file changed, 19 insertions(+), 29 deletions(-)
drivers/usb/serial/digi_acceleport.c +19 -29
index 097769525ca4..046cd9d57600 100644
@@ -1069,7 +1069,6 @@ static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	unsigned char buf[32];
 	struct digi_port *priv = usb_get_serial_port_data(port);
 	struct ktermios not_termios;
-	int throttled;
 
 	/* be sure the device is started up */
 	if (digi_startup_device(port->serial) != 0)

@@ -1099,17 +1098,14 @@ static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	}
 
 	spin_lock_irq(&priv->dp_port_lock);
-	throttled = priv->dp_throttle_restart;
 	priv->dp_throttled = 0;
 	priv->dp_throttle_restart = 0;
 	spin_unlock_irq(&priv->dp_port_lock);
 
-	if (throttled) {
-		ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
-		if (ret) {
-			dev_err(&port->dev, "failed to submit read urb: %d\n", ret);
-			return ret;
-		}
+	ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
+	if (ret) {
+		dev_err(&port->dev, "failed to submit read urb: %d\n", ret);
+		return ret;
 	}
 
 	return 0;

@@ -1123,6 +1119,8 @@ static void digi_close(struct usb_serial_port *port)
 	unsigned char buf[32];
 	struct digi_port *priv = usb_get_serial_port_data(port);
 
+	usb_kill_urb(port->read_urb);
+
 	mutex_lock(&port->serial->disc_mutex);
 	/* if disconnected, just clear flags */
 	if (port->serial->disconnected)

@@ -1185,15 +1183,15 @@ static void digi_close(struct usb_serial_port *port)
 /*
  *  Digi Startup Device
  *
- *  Starts reads on all ports.  Must be called AFTER startup, with
+ *  Starts read on the OOB port.  Must be called AFTER startup, with
  *  urbs initialized.  Returns 0 if successful, non-zero error otherwise.
  */
 
 static int digi_startup_device(struct usb_serial *serial)
 {
-	int i, ret = 0;
 	struct digi_serial *serial_priv = usb_get_serial_data(serial);
-	struct usb_serial_port *port;
+	struct usb_serial_port *oob_port = serial_priv->ds_oob_port;
+	int ret;
 
 	/* be sure this happens exactly once */
 	spin_lock(&serial_priv->ds_serial_lock);

@@ -1204,19 +1202,13 @@ static int digi_startup_device(struct usb_serial *serial)
 	serial_priv->ds_device_started = 1;
 	spin_unlock(&serial_priv->ds_serial_lock);
 
-	/* start reading from each bulk in endpoint for the device */
-	/* set USB_DISABLE_SPD flag for write bulk urbs */
-	for (i = 0; i < serial->type->num_ports + 1; i++) {
-		port = serial->port[i];
-		ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
-		if (ret != 0) {
-			dev_err(&port->dev,
-				"%s: usb_submit_urb failed, ret=%d, port=%d\n",
-				__func__, ret, i);
-			break;
-		}
+	ret = usb_submit_urb(oob_port->read_urb, GFP_KERNEL);
+	if (ret) {
+		dev_err(&serial->interface->dev, "failed to submit OOB read urb: %d\n", ret);
+		return ret;
 	}
-	return ret;
+
+	return 0;
 }
 
 static int digi_port_init(struct usb_serial_port *port, unsigned port_num)

@@ -1287,13 +1279,11 @@ static int digi_startup(struct usb_serial *serial)
 
 static void digi_disconnect(struct usb_serial *serial)
 {
-	int i;
+	struct digi_serial *serial_priv = usb_get_serial_data(serial);
+	struct usb_serial_port *oob_port = serial_priv->ds_oob_port;
 
-	/* stop reads and writes on all ports */
-	for (i = 0; i < serial->type->num_ports + 1; i++) {
-		usb_kill_urb(serial->port[i]->read_urb);
-		usb_kill_urb(serial->port[i]->write_urb);
-	}
+	usb_kill_urb(oob_port->read_urb);
+	usb_kill_urb(oob_port->write_urb);
 }
Signature · 1 line
2.53.0
author maintainer
#3 lore raw

PATCH v2 3/12 USB: serial: digi_acceleport: drop unused wait queue

In reply to Johan Hovold

Drop the close wait queue which has not been used since commit 335f8514f200 ("tty: Bring the usb tty port structure into more use").

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/usb/serial/digi_acceleport.c +0 -3
index 046cd9d57600..83c4bacc2df8 100644
@@ -194,7 +194,6 @@ struct digi_port {
 	int dp_throttled;
 	int dp_throttle_restart;
 	wait_queue_head_t dp_flush_wait;
-	wait_queue_head_t dp_close_wait;	/* wait queue for close */
 	wait_queue_head_t write_wait;
 	struct usb_serial_port *dp_port;
 };

@@ -1174,7 +1173,6 @@ static void digi_close(struct usb_serial_port *port)
 exit:
 	spin_lock_irq(&priv->dp_port_lock);
 	priv->dp_write_urb_in_use = 0;
-	wake_up_interruptible(&priv->dp_close_wait);
 	spin_unlock_irq(&priv->dp_port_lock);
 	mutex_unlock(&port->serial->disc_mutex);
 }

@@ -1223,7 +1221,6 @@ static int digi_port_init(struct usb_serial_port *port, unsigned port_num)
 	priv->dp_port_num = port_num;
 	init_waitqueue_head(&priv->dp_transmit_idle_wait);
 	init_waitqueue_head(&priv->dp_flush_wait);
-	init_waitqueue_head(&priv->dp_close_wait);
 	init_waitqueue_head(&priv->write_wait);
 	priv->dp_port = port;
Signature · 1 line
2.53.0
author maintainer
#4 lore raw

PATCH v2 4/12 USB: serial: digi_acceleport: always stop write urb on close

In reply to Johan Hovold

Explicitly stop the write urb on close() also if the device is being unbound instead of relying on core to do it after returning.

Note that the dp_write_urb_in_use flag is cleared by the completion handler.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/usb/serial/digi_acceleport.c +2 -6
index 83c4bacc2df8..7a52600c09b1 100644
@@ -1121,7 +1121,6 @@ static void digi_close(struct usb_serial_port *port)
 	usb_kill_urb(port->read_urb);
 
 	mutex_lock(&port->serial->disc_mutex);
-	/* if disconnected, just clear flags */
 	if (port->serial->disconnected)
 		goto exit;
 

@@ -1167,14 +1166,11 @@ static void digi_close(struct usb_serial_port *port)
 			TASK_INTERRUPTIBLE);
 	schedule_timeout(DIGI_CLOSE_TIMEOUT);
 	finish_wait(&priv->dp_flush_wait, &wait);
+exit:
+	mutex_unlock(&port->serial->disc_mutex);
 
 	/* shutdown any outstanding bulk writes */
 	usb_kill_urb(port->write_urb);
-exit:
-	spin_lock_irq(&priv->dp_port_lock);
-	priv->dp_write_urb_in_use = 0;
-	spin_unlock_irq(&priv->dp_port_lock);
-	mutex_unlock(&port->serial->disc_mutex);
 }
Signature · 1 line
2.53.0
author maintainer
#5 lore raw

PATCH v2 5/12 USB: serial: digi_acceleport: add oob port helper

In reply to Johan Hovold

Add a helper function for retrieving the OOB port to replace two convoluted expressions.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/usb/serial/digi_acceleport.c +9 -2
index 7a52600c09b1..ae4807455dc0 100644
@@ -350,6 +350,13 @@ __releases(lock)
 	return timeout;
 }
 
+static struct usb_serial_port *digi_get_oob_port(struct usb_serial *serial)
+{
+	struct digi_serial *serial_priv = usb_get_serial_data(serial);
+
+	return serial_priv->ds_oob_port;
+}
+
 /*
  *  Digi Write OOB Command
  *

@@ -366,7 +373,7 @@ static int digi_write_oob_command(struct usb_serial_port *port,
 {
 	int ret = 0;
 	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 usb_serial_port *oob_port = digi_get_oob_port(port->serial);
 	struct digi_port *oob_priv = usb_get_serial_port_data(oob_port);
 	unsigned long flags;
 

@@ -504,7 +511,7 @@ static int digi_set_modem_signals(struct usb_serial_port *port,
 
 	int ret;
 	struct digi_port *port_priv = usb_get_serial_port_data(port);
-	struct usb_serial_port *oob_port = (struct usb_serial_port *) ((struct digi_serial *)(usb_get_serial_data(port->serial)))->ds_oob_port;
+	struct usb_serial_port *oob_port = digi_get_oob_port(port->serial);
 	struct digi_port *oob_priv = usb_get_serial_port_data(oob_port);
 	unsigned char *data = oob_port->write_urb->transfer_buffer;
 	unsigned long flags;
Signature · 1 line
2.53.0
author maintainer
#6 lore raw

PATCH v2 6/12 USB: serial: digi_acceleport: clean up declarations and whitespace

In reply to Johan Hovold

Clean up the driver by moving some declarations to approximate reverse xmas style and removing some stray newlines (and adding a few for readability).

While at it, also replace two spaces before tabs in the driver structs.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 88 +++++++++-------------------
 1 file changed, 29 insertions(+), 59 deletions(-)
drivers/usb/serial/digi_acceleport.c +29 -59
index ae4807455dc0..0dd898a96c33 100644
@@ -271,7 +271,7 @@ static struct usb_serial_driver digi_acceleport_2_device = {
 	.dtr_rts =			digi_dtr_rts,
 	.write =			digi_write,
 	.write_room =			digi_write_room,
-	.write_bulk_callback = 		digi_write_bulk_callback,
+	.write_bulk_callback =		digi_write_bulk_callback,
 	.read_bulk_callback =		digi_read_bulk_callback,
 	.chars_in_buffer =		digi_chars_in_buffer,
 	.throttle =			digi_rx_throttle,

@@ -300,7 +300,7 @@ static struct usb_serial_driver digi_acceleport_4_device = {
 	.close =			digi_close,
 	.write =			digi_write,
 	.write_room =			digi_write_room,
-	.write_bulk_callback = 		digi_write_bulk_callback,
+	.write_bulk_callback =		digi_write_bulk_callback,
 	.read_bulk_callback =		digi_read_bulk_callback,
 	.chars_in_buffer =		digi_chars_in_buffer,
 	.throttle =			digi_rx_throttle,

@@ -334,7 +334,6 @@ static struct usb_serial_driver * const serial_drivers[] = {
  *  interruptible_sleep_on_timeout is deprecated and has been replaced
  *  with the equivalent code.
  */
-
 static long cond_wait_interruptible_timeout_irqrestore(
 	wait_queue_head_t *q, long timeout,
 	spinlock_t *lock, unsigned long flags)

@@ -367,15 +366,14 @@ static struct usb_serial_port *digi_get_oob_port(struct usb_serial *serial)
  *  the interruptible flag is true, or a negative error
  *  returned by usb_submit_urb.
  */
-
 static int digi_write_oob_command(struct usb_serial_port *port,
 	unsigned char *buf, int count, int interruptible)
 {
-	int ret = 0;
-	int len;
 	struct usb_serial_port *oob_port = digi_get_oob_port(port->serial);
 	struct digi_port *oob_priv = usb_get_serial_port_data(oob_port);
 	unsigned long flags;
+	int ret = 0;
+	int len;
 
 	dev_dbg(&port->dev,
 		"digi_write_oob_command: TOP: port=%d, count=%d\n",

@@ -410,10 +408,8 @@ static int digi_write_oob_command(struct usb_serial_port *port,
 		dev_err(&port->dev, "%s: usb_submit_urb failed, ret=%d\n",
 			__func__, ret);
 	return ret;
-
 }
 
-
 /*
  *  Digi Write In Band Command
  *

@@ -425,15 +421,14 @@ static int digi_write_oob_command(struct usb_serial_port *port,
  *  timeout ticks.  Returns 0 if successful, or a negative
  *  error returned by digi_write.
  */
-
 static int digi_write_inb_command(struct usb_serial_port *port,
 	unsigned char *buf, int count, unsigned long timeout)
 {
-	int ret = 0;
-	int len;
 	struct digi_port *priv = usb_get_serial_port_data(port);
 	unsigned char *data = port->write_urb->transfer_buffer;
 	unsigned long flags;
+	int ret = 0;
+	int len;
 
 	dev_dbg(&port->dev, "digi_write_inb_command: TOP: port=%d, count=%d\n",
 		priv->dp_port_num, count);

@@ -483,7 +478,6 @@ static int digi_write_inb_command(struct usb_serial_port *port,
 			count -= len;
 			buf += len;
 		}
-
 	}
 	spin_unlock_irqrestore(&priv->dp_port_lock, flags);
 

@@ -494,7 +488,6 @@ static int digi_write_inb_command(struct usb_serial_port *port,
 	return ret;
 }
 
-
 /*
  *  Digi Set Modem Signals
  *

@@ -504,17 +497,15 @@ static int digi_write_inb_command(struct usb_serial_port *port,
  *  -EINTR if interrupted while sleeping, or a non-zero error
  *  returned by usb_submit_urb.
  */
-
 static int digi_set_modem_signals(struct usb_serial_port *port,
 	unsigned int modem_signals, int interruptible)
 {
-
-	int ret;
 	struct digi_port *port_priv = usb_get_serial_port_data(port);
 	struct usb_serial_port *oob_port = digi_get_oob_port(port->serial);
 	struct digi_port *oob_priv = usb_get_serial_port_data(oob_port);
 	unsigned char *data = oob_port->write_urb->transfer_buffer;
 	unsigned long flags;
+	int ret;
 
 	dev_dbg(&port->dev,
 		"digi_set_modem_signals: TOP: port=%d, modem_signals=0x%x\n",

@@ -572,14 +563,13 @@ static int digi_set_modem_signals(struct usb_serial_port *port,
  *  is only called from close, and only one process can be in close on a
  *  port at a time, so its ok.
  */
-
 static int digi_transmit_idle(struct usb_serial_port *port,
 	unsigned long timeout)
 {
-	int ret;
-	unsigned char buf[2];
 	struct digi_port *priv = usb_get_serial_port_data(port);
+	unsigned char buf[2];
 	unsigned long flags;
+	int ret;
 
 	spin_lock_irqsave(&priv->dp_port_lock, flags);
 	priv->dp_transmit_idle = 0;

@@ -606,16 +596,15 @@ static int digi_transmit_idle(struct usb_serial_port *port,
 	}
 	priv->dp_transmit_idle = 0;
 	spin_unlock_irqrestore(&priv->dp_port_lock, flags);
-	return 0;
 
+	return 0;
 }
 
-
 static void digi_rx_throttle(struct tty_struct *tty)
 {
-	unsigned long flags;
 	struct usb_serial_port *port = tty->driver_data;
 	struct digi_port *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
 
 	/* stop receiving characters by not resubmitting the read urb */
 	spin_lock_irqsave(&priv->dp_port_lock, flags);

@@ -624,13 +613,12 @@ static void digi_rx_throttle(struct tty_struct *tty)
 	spin_unlock_irqrestore(&priv->dp_port_lock, flags);
 }
 
-
 static void digi_rx_unthrottle(struct tty_struct *tty)
 {
-	int ret = 0;
-	unsigned long flags;
 	struct usb_serial_port *port = tty->driver_data;
 	struct digi_port *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+	int ret = 0;
 
 	spin_lock_irqsave(&priv->dp_port_lock, flags);
 

@@ -650,7 +638,6 @@ static void digi_rx_unthrottle(struct tty_struct *tty)
 			__func__, ret, priv->dp_port_num);
 }
 
-
 static void digi_set_termios(struct tty_struct *tty,
 			     struct usb_serial_port *port,
 			     const struct ktermios *old_termios)

@@ -761,7 +748,6 @@ static void digi_set_termios(struct tty_struct *tty,
 
 	/* set stop bits */
 	if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
-
 		if ((cflag & CSTOPB))
 			arg = DIGI_STOP_BITS_2;
 		else

@@ -771,7 +757,6 @@ static void digi_set_termios(struct tty_struct *tty,
 		buf[i++] = priv->dp_port_num;
 		buf[i++] = arg;
 		buf[i++] = 0;
-
 	}
 
 	/* set input flow control */

@@ -840,7 +825,6 @@ static void digi_set_termios(struct tty_struct *tty,
 	tty_encode_baud_rate(tty, baud, baud);
 }
 
-
 static int digi_break_ctl(struct tty_struct *tty, int break_state)
 {
 	struct usb_serial_port *port = tty->driver_data;

@@ -854,43 +838,41 @@ static int digi_break_ctl(struct tty_struct *tty, int break_state)
 	return digi_write_inb_command(port, buf, 4, 0);
 }
 
-
 static int digi_tiocmget(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct digi_port *priv = usb_get_serial_port_data(port);
-	unsigned int val;
 	unsigned long flags;
+	unsigned int val;
 
 	spin_lock_irqsave(&priv->dp_port_lock, flags);
 	val = priv->dp_modem_signals;
 	spin_unlock_irqrestore(&priv->dp_port_lock, flags);
+
 	return val;
 }
 
-
 static int digi_tiocmset(struct tty_struct *tty,
 					unsigned int set, unsigned int clear)
 {
 	struct usb_serial_port *port = tty->driver_data;
 	struct digi_port *priv = usb_get_serial_port_data(port);
-	unsigned int val;
 	unsigned long flags;
+	unsigned int val;
 
 	spin_lock_irqsave(&priv->dp_port_lock, flags);
 	val = (priv->dp_modem_signals & ~clear) | set;
 	spin_unlock_irqrestore(&priv->dp_port_lock, flags);
+
 	return digi_set_modem_signals(port, val, 1);
 }
 
-
 static int digi_write(struct tty_struct *tty, struct usb_serial_port *port,
 					const unsigned char *buf, int count)
 {
-
-	int ret, data_len, new_len;
 	struct digi_port *priv = usb_get_serial_port_data(port);
 	unsigned char *data = port->write_urb->transfer_buffer;
+	int ret, data_len, new_len;
 	unsigned long flags;
 
 	dev_dbg(&port->dev, "digi_write: TOP: port=%d, count=%d\n",

@@ -953,21 +935,20 @@ static int digi_write(struct tty_struct *tty, struct usb_serial_port *port,
 			"%s: usb_submit_urb failed, ret=%d, port=%d\n",
 			__func__, ret, priv->dp_port_num);
 	dev_dbg(&port->dev, "digi_write: returning %d\n", ret);
-	return ret;
 
+	return ret;
 }
 
 static void digi_write_bulk_callback(struct urb *urb)
 {
-
 	struct usb_serial_port *port = urb->context;
 	struct usb_serial *serial;
 	struct digi_port *priv;
 	struct digi_serial *serial_priv;
-	unsigned long flags;
-	int ret = 0;
 	int status = urb->status;
+	unsigned long flags;
 	bool wakeup;
+	int ret = 0;
 
 	/* port and serial sanity check */
 	if (port == NULL || (priv = usb_get_serial_port_data(port)) == NULL) {

@@ -1040,8 +1021,8 @@ static unsigned int digi_write_room(struct tty_struct *tty)
 
 	spin_unlock_irqrestore(&priv->dp_port_lock, flags);
 	dev_dbg(&port->dev, "digi_write_room: port=%d, room=%u\n", priv->dp_port_num, room);
-	return room;
 
+	return room;
 }
 
 static unsigned int digi_chars_in_buffer(struct tty_struct *tty)

@@ -1071,10 +1052,10 @@ static void digi_dtr_rts(struct usb_serial_port *port, int on)
 
 static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
-	int ret;
-	unsigned char buf[32];
 	struct digi_port *priv = usb_get_serial_port_data(port);
 	struct ktermios not_termios;
+	unsigned char buf[32];
+	int ret;
 
 	/* be sure the device is started up */
 	if (digi_startup_device(port->serial) != 0)

@@ -1117,13 +1098,12 @@ static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	return 0;
 }
 
-
 static void digi_close(struct usb_serial_port *port)
 {
+	struct digi_port *priv = usb_get_serial_port_data(port);
+	unsigned char buf[32];
 	DEFINE_WAIT(wait);
 	int ret;
-	unsigned char buf[32];
-	struct digi_port *priv = usb_get_serial_port_data(port);
 
 	usb_kill_urb(port->read_urb);
 

@@ -1180,14 +1160,12 @@ static void digi_close(struct usb_serial_port *port)
 	usb_kill_urb(port->write_urb);
 }
 
-
 /*
  *  Digi Startup Device
  *
  *  Starts read on the OOB port.  Must be called AFTER startup, with
  *  urbs initialized.  Returns 0 if successful, non-zero error otherwise.
  */
-
 static int digi_startup_device(struct usb_serial *serial)
 {
 	struct digi_serial *serial_priv = usb_get_serial_data(serial);

@@ -1276,7 +1254,6 @@ static int digi_startup(struct usb_serial *serial)
 	return 0;
 }
 
-
 static void digi_disconnect(struct usb_serial *serial)
 {
 	struct digi_serial *serial_priv = usb_get_serial_data(serial);

@@ -1286,7 +1263,6 @@ static void digi_disconnect(struct usb_serial *serial)
 	usb_kill_urb(oob_port->write_urb);
 }
 
-
 static void digi_release(struct usb_serial *serial)
 {
 	struct digi_serial *serial_priv;

@@ -1318,8 +1294,8 @@ static void digi_read_bulk_callback(struct urb *urb)
 	struct usb_serial_port *port = urb->context;
 	struct digi_port *priv;
 	struct digi_serial *serial_priv;
-	int ret;
 	int status = urb->status;
+	int ret;
 
 	/* port sanity check, do not resubmit if port is not valid */
 	if (port == NULL)

@@ -1361,7 +1337,6 @@ static void digi_read_bulk_callback(struct urb *urb)
 			"%s: failed resubmitting urb, ret=%d, port=%d\n",
 			__func__, ret, priv->dp_port_num);
 	}
-
 }
 
 /*

@@ -1373,7 +1348,6 @@ static void digi_read_bulk_callback(struct urb *urb)
  *  It returns 0 if successful, 1 if successful but the port is
  *  throttled, and -1 if the sanity checks failed.
  */
-
 static int digi_read_inb_callback(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;

@@ -1451,10 +1425,8 @@ static int digi_read_inb_callback(struct urb *urb)
 		dev_dbg(&port->dev, "%s: unknown opcode: %d\n", __func__, opcode);
 
 	return throttled ? 1 : 0;
-
 }
 
-
 /*
  *  Digi Read OOB Callback
  *

@@ -1463,10 +1435,8 @@ static int digi_read_inb_callback(struct urb *urb)
  *  the port->serial is valid.  It returns 0 if successful, and
  *  -1 if the sanity checks failed.
  */
-
 static int digi_read_oob_callback(struct urb *urb)
 {
-
 	struct usb_serial_port *port = urb->context;
 	struct usb_serial *serial = port->serial;
 	struct tty_struct *tty;

@@ -1474,8 +1444,8 @@ static int digi_read_oob_callback(struct urb *urb)
 	unsigned char *buf = urb->transfer_buffer;
 	int opcode, line, status, val;
 	unsigned long flags;
-	int i;
 	unsigned int rts;
+	int i;
 
 	if (urb->actual_length < 4)
 		return -1;

@@ -1545,8 +1515,8 @@ static int digi_read_oob_callback(struct urb *urb)
 		}
 		tty_kref_put(tty);
 	}
-	return 0;
 
+	return 0;
 }
 
 module_usb_serial_driver(serial_drivers, id_table_combined);
Signature · 1 line
2.53.0
author maintainer
#7 lore raw

PATCH v2 7/12 USB: serial: digi_acceleport: drop redundant driver data sanity checks

In reply to Johan Hovold

The urb context pointer does not change while an urb is in flight so there is never a need to check for NULL on completion.

The port driver data is not freed until the port is unbound at which point all I/O for that port has been stopped (and I/O is no longer started for a port that has not yet been probed).

The device driver data is not freed until after the driver has been unbound and at which point all I/O has also ceased.

Drop the redundant, overly defensive (and still incomplete) sanity checks from the completion callbacks.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 40 +++-------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)
drivers/usb/serial/digi_acceleport.c +4 -36
index 0dd898a96c33..64b3f42af981 100644
@@ -942,28 +942,12 @@ static int digi_write(struct tty_struct *tty, struct usb_serial_port *port,
 static void digi_write_bulk_callback(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;
-	struct usb_serial *serial;
-	struct digi_port *priv;
-	struct digi_serial *serial_priv;
-	int status = urb->status;
+	struct digi_serial *serial_priv = usb_get_serial_data(port->serial);
+	struct digi_port *priv = usb_get_serial_port_data(port);
 	unsigned long flags;
 	bool wakeup;
 	int ret = 0;
 
-	/* port and serial sanity check */
-	if (port == NULL || (priv = usb_get_serial_port_data(port)) == NULL) {
-		pr_err("%s: port or port->private is NULL, status=%d\n",
-			__func__, status);
-		return;
-	}
-	serial = port->serial;
-	if (serial == NULL || (serial_priv = usb_get_serial_data(serial)) == NULL) {
-		dev_err(&port->dev,
-			"%s: serial or serial->private is NULL, status=%d\n",
-			__func__, status);
-		return;
-	}
-
 	/* handle oob callback */
 	if (priv->dp_port_num == serial_priv->ds_oob_port_num) {
 		dev_dbg(&port->dev, "digi_write_bulk_callback: oob callback\n");

@@ -1292,27 +1276,11 @@ static void digi_port_remove(struct usb_serial_port *port)
 static void digi_read_bulk_callback(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;
-	struct digi_port *priv;
-	struct digi_serial *serial_priv;
+	struct digi_serial *serial_priv = usb_get_serial_data(port->serial);
+	struct digi_port *priv = usb_get_serial_port_data(port);
 	int status = urb->status;
 	int ret;
 
-	/* port sanity check, do not resubmit if port is not valid */
-	if (port == NULL)
-		return;
-	priv = usb_get_serial_port_data(port);
-	if (priv == NULL) {
-		dev_err(&port->dev, "%s: port->private is NULL, status=%d\n",
-			__func__, status);
-		return;
-	}
-	if (port->serial == NULL ||
-		(serial_priv = usb_get_serial_data(port->serial)) == NULL) {
-		dev_err(&port->dev, "%s: serial is bad or serial->private "
-			"is NULL, status=%d\n", __func__, status);
-		return;
-	}
-
 	/* do not resubmit urb if it has any status error */
 	if (status) {
 		dev_err(&port->dev,
Signature · 1 line
2.53.0
author maintainer
#8 lore raw

PATCH v2 8/12 USB: serial: digi_acceleport: stop OOB I/O when not in use

In reply to Johan Hovold

The driver submits the OOB read urb on first open of a port and does not stop it until the device is disconnected.

Add an open counter and submit the urb on first open and stop it on last close to avoid wasting resources (e.g. power) when the device is not in use.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 100 ++++++++++++++-------------
 1 file changed, 51 insertions(+), 49 deletions(-)
drivers/usb/serial/digi_acceleport.c +51 -49
index 64b3f42af981..e4ab41d9f3a5 100644
@@ -176,10 +176,10 @@
 /* Structures */
 
 struct digi_serial {
-	spinlock_t ds_serial_lock;
+	struct mutex open_mutex;
 	struct usb_serial_port *ds_oob_port;	/* out-of-band port */
 	int ds_oob_port_num;			/* index of out-of-band port */
-	int ds_device_started;
+	int open_count;
 };
 
 struct digi_port {

@@ -226,9 +226,7 @@ static unsigned int digi_chars_in_buffer(struct tty_struct *tty);
 static int digi_open(struct tty_struct *tty, struct usb_serial_port *port);
 static void digi_close(struct usb_serial_port *port);
 static void digi_dtr_rts(struct usb_serial_port *port, int on);
-static int digi_startup_device(struct usb_serial *serial);
 static int digi_startup(struct usb_serial *serial);
-static void digi_disconnect(struct usb_serial *serial);
 static void digi_release(struct usb_serial *serial);
 static int digi_port_probe(struct usb_serial_port *port);
 static void digi_port_remove(struct usb_serial_port *port);

@@ -281,7 +279,6 @@ static struct usb_serial_driver digi_acceleport_2_device = {
 	.tiocmget =			digi_tiocmget,
 	.tiocmset =			digi_tiocmset,
 	.attach =			digi_startup,
-	.disconnect =			digi_disconnect,
 	.release =			digi_release,
 	.port_probe =			digi_port_probe,
 	.port_remove =			digi_port_remove,

@@ -310,7 +307,6 @@ static struct usb_serial_driver digi_acceleport_4_device = {
 	.tiocmget =			digi_tiocmget,
 	.tiocmset =			digi_tiocmset,
 	.attach =			digi_startup,
-	.disconnect =			digi_disconnect,
 	.release =			digi_release,
 	.port_probe =			digi_port_probe,
 	.port_remove =			digi_port_remove,

@@ -1034,6 +1030,43 @@ static void digi_dtr_rts(struct usb_serial_port *port, int on)
 	digi_set_modem_signals(port, on * (TIOCM_DTR | TIOCM_RTS), 1);
 }
 
+static int digi_open_oob_port(struct usb_serial *serial)
+{
+	struct digi_serial *serial_priv = usb_get_serial_data(serial);
+	struct usb_serial_port *oob_port = serial_priv->ds_oob_port;
+	int ret = 0;
+
+	mutex_lock(&serial_priv->open_mutex);
+
+	if (serial_priv->open_count++ == 0) {
+		ret = usb_submit_urb(oob_port->read_urb, GFP_KERNEL);
+		if (ret) {
+			dev_err(&serial->interface->dev, "failed to submit OOB read urb: %d\n",
+					ret);
+			serial_priv->open_count--;
+		}
+	}
+
+	mutex_unlock(&serial_priv->open_mutex);
+
+	return ret;
+}
+
+static void digi_close_oob_port(struct usb_serial *serial)
+{
+	struct digi_serial *serial_priv = usb_get_serial_data(serial);
+	struct usb_serial_port *oob_port = serial_priv->ds_oob_port;
+
+	mutex_lock(&serial_priv->open_mutex);
+
+	if (serial_priv->open_count-- == 1) {
+		usb_kill_urb(oob_port->read_urb);
+		usb_kill_urb(oob_port->write_urb);
+	}
+
+	mutex_unlock(&serial_priv->open_mutex);
+}
+
 static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	struct digi_port *priv = usb_get_serial_port_data(port);

@@ -1041,9 +1074,9 @@ static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	unsigned char buf[32];
 	int ret;
 
-	/* be sure the device is started up */
-	if (digi_startup_device(port->serial) != 0)
-		return -ENXIO;
+	ret = digi_open_oob_port(port->serial);
+	if (ret)
+		return ret;
 
 	/* read modem signals automatically whenever they change */
 	buf[0] = DIGI_CMD_READ_INPUT_SIGNALS;

@@ -1076,10 +1109,15 @@ static int digi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
 	if (ret) {
 		dev_err(&port->dev, "failed to submit read urb: %d\n", ret);
-		return ret;
+		goto err_close_oob;
 	}
 
 	return 0;
+
+err_close_oob:
+	digi_close_oob_port(port->serial);
+
+	return ret;
 }
 
 static void digi_close(struct usb_serial_port *port)

@@ -1142,36 +1180,8 @@ static void digi_close(struct usb_serial_port *port)
 
 	/* shutdown any outstanding bulk writes */
 	usb_kill_urb(port->write_urb);
-}
-
-/*
- *  Digi Startup Device
- *
- *  Starts read on the OOB port.  Must be called AFTER startup, with
- *  urbs initialized.  Returns 0 if successful, non-zero error otherwise.
- */
-static int digi_startup_device(struct usb_serial *serial)
-{
-	struct digi_serial *serial_priv = usb_get_serial_data(serial);
-	struct usb_serial_port *oob_port = serial_priv->ds_oob_port;
-	int ret;
-
-	/* be sure this happens exactly once */
-	spin_lock(&serial_priv->ds_serial_lock);
-	if (serial_priv->ds_device_started) {
-		spin_unlock(&serial_priv->ds_serial_lock);
-		return 0;
-	}
-	serial_priv->ds_device_started = 1;
-	spin_unlock(&serial_priv->ds_serial_lock);
 
-	ret = usb_submit_urb(oob_port->read_urb, GFP_KERNEL);
-	if (ret) {
-		dev_err(&serial->interface->dev, "failed to submit OOB read urb: %d\n", ret);
-		return ret;
-	}
-
-	return 0;
+	digi_close_oob_port(port->serial);
 }
 
 static int digi_port_init(struct usb_serial_port *port, unsigned port_num)

@@ -1222,7 +1232,8 @@ static int digi_startup(struct usb_serial *serial)
 	if (!serial_priv)
 		return -ENOMEM;
 
-	spin_lock_init(&serial_priv->ds_serial_lock);
+	mutex_init(&serial_priv->open_mutex);
+
 	serial_priv->ds_oob_port_num = oob_port_num;
 	serial_priv->ds_oob_port = serial->port[oob_port_num];
 

@@ -1238,15 +1249,6 @@ static int digi_startup(struct usb_serial *serial)
 	return 0;
 }
 
-static void digi_disconnect(struct usb_serial *serial)
-{
-	struct digi_serial *serial_priv = usb_get_serial_data(serial);
-	struct usb_serial_port *oob_port = serial_priv->ds_oob_port;
-
-	usb_kill_urb(oob_port->read_urb);
-	usb_kill_urb(oob_port->write_urb);
-}
-
 static void digi_release(struct usb_serial *serial)
 {
 	struct digi_serial *serial_priv;
Signature · 1 line
2.53.0
author maintainer
#9 lore raw

PATCH v2 9/12 USB: serial: digi_acceleport: drop unused in-buf define

In reply to Johan Hovold

Drop the in-buf size define which has not been used since the port buffers were removed by commit 5fea2a4dabdf ("USB: digi_acceleport further buffer clean up").

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/usb/serial/digi_acceleport.c +0 -4
index e4ab41d9f3a5..8c21c669c1a2 100644
@@ -32,10 +32,6 @@
 /* so we can be sure to send the full buffer in one urb */
 #define DIGI_OUT_BUF_SIZE		8
 
-/* port input buffer length -- must be >= transfer buffer length - 3 */
-/* so we can be sure to hold at least one full buffer from one urb */
-#define DIGI_IN_BUF_SIZE		64
-
 /* retry timeout while sleeping */
 #define DIGI_RETRY_TIMEOUT		(HZ/10)
Signature · 1 line
2.53.0
author maintainer
#10 lore raw

PATCH v2 10/12 USB: serial: digi_acceleport: clean up xfer buf length expression

In reply to Johan Hovold

Add the missing space around operators in transfer-buffer length expressions to make the code more readable.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/serial/digi_acceleport.c +4 -4
index 8c21c669c1a2..f7630fdcdceb 100644
@@ -445,7 +445,7 @@ static int digi_write_inb_command(struct usb_serial_port *port,
 		/* len must be a multiple of 4 and small enough to */
 		/* guarantee the write will send buffered data first, */
 		/* so commands are in order with data and not split */
-		len = min(count, port->bulk_out_size-2-priv->dp_out_buf_len);
+		len = min(count, port->bulk_out_size - 2 - priv->dp_out_buf_len);
 		if (len > 4)
 			len &= ~3;
 

@@ -871,7 +871,7 @@ static int digi_write(struct tty_struct *tty, struct usb_serial_port *port,
 		priv->dp_port_num, count);
 
 	/* copy user data (which can sleep) before getting spin lock */
-	count = min(count, port->bulk_out_size-2);
+	count = min(count, port->bulk_out_size - 2);
 	count = min(64, count);
 
 	/* be sure only one write proceeds at a time */

@@ -893,7 +893,7 @@ static int digi_write(struct tty_struct *tty, struct usb_serial_port *port,
 
 	/* allow space for any buffered data and for new data, up to */
 	/* transfer buffer size - 2 (for command and length bytes) */
-	new_len = min(count, port->bulk_out_size-2-priv->dp_out_buf_len);
+	new_len = min(count, port->bulk_out_size - 2 - priv->dp_out_buf_len);
 	data_len = new_len + priv->dp_out_buf_len;
 
 	if (data_len == 0) {

@@ -901,7 +901,7 @@ static int digi_write(struct tty_struct *tty, struct usb_serial_port *port,
 		return 0;
 	}
 
-	port->write_urb->transfer_buffer_length = data_len+2;
+	port->write_urb->transfer_buffer_length = data_len + 2;
 
 	*data++ = DIGI_CMD_SEND_DATA;
 	*data++ = data_len;
Signature · 1 line
2.53.0
author maintainer
#11 lore raw

PATCH v2 11/12 USB: serial: digi_acceleport: clean up write completion

In reply to Johan Hovold

Clean up the write completion handler by adding a temporary variable for the transfer buffer and using the pre-existing urb pointer while dropping some redundant casts.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
drivers/usb/serial/digi_acceleport.c +8 -9
index f7630fdcdceb..6a557fc1ad85 100644
@@ -936,6 +936,7 @@ static void digi_write_bulk_callback(struct urb *urb)
 	struct usb_serial_port *port = urb->context;
 	struct digi_serial *serial_priv = usb_get_serial_data(port->serial);
 	struct digi_port *priv = usb_get_serial_port_data(port);
+	unsigned char *data = urb->transfer_buffer;
 	unsigned long flags;
 	bool wakeup;
 	int ret = 0;

@@ -955,15 +956,13 @@ static void digi_write_bulk_callback(struct urb *urb)
 	spin_lock_irqsave(&priv->dp_port_lock, flags);
 	priv->dp_write_urb_in_use = 0;
 	if (priv->dp_out_buf_len > 0) {
-		*((unsigned char *)(port->write_urb->transfer_buffer))
-			= (unsigned char)DIGI_CMD_SEND_DATA;
-		*((unsigned char *)(port->write_urb->transfer_buffer) + 1)
-			= (unsigned char)priv->dp_out_buf_len;
-		port->write_urb->transfer_buffer_length =
-						priv->dp_out_buf_len + 2;
-		memcpy(port->write_urb->transfer_buffer + 2, priv->dp_out_buf,
-			priv->dp_out_buf_len);
-		ret = usb_submit_urb(port->write_urb, GFP_ATOMIC);
+		data[0] = DIGI_CMD_SEND_DATA;
+		data[1] = priv->dp_out_buf_len;
+		memcpy(data + 2, priv->dp_out_buf, priv->dp_out_buf_len);
+
+		urb->transfer_buffer_length = priv->dp_out_buf_len + 2;
+
+		ret = usb_submit_urb(urb, GFP_ATOMIC);
 		if (ret == 0) {
 			priv->dp_write_urb_in_use = 1;
 			priv->dp_out_buf_len = 0;
Signature · 1 line
2.53.0
author maintainer
#12 lore raw

PATCH v2 12/12 USB: serial: digi_acceleport: clean up inb command submission

In reply to Johan Hovold

Clean up the inb command handling a bit by removing an unnecessary line break and moving the assignment operator before breaking another long expression.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/digi_acceleport.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/usb/serial/digi_acceleport.c +3 -4
index 6a557fc1ad85..6790892ca2a4 100644
@@ -453,11 +453,10 @@ static int digi_write_inb_command(struct usb_serial_port *port,
 		if (priv->dp_out_buf_len > 0) {
 			data[0] = DIGI_CMD_SEND_DATA;
 			data[1] = priv->dp_out_buf_len;
-			memcpy(data + 2, priv->dp_out_buf,
-				priv->dp_out_buf_len);
+			memcpy(data + 2, priv->dp_out_buf, priv->dp_out_buf_len);
 			memcpy(data + 2 + priv->dp_out_buf_len, buf, len);
-			port->write_urb->transfer_buffer_length
-				= priv->dp_out_buf_len + 2 + len;
+			port->write_urb->transfer_buffer_length =
+					priv->dp_out_buf_len + 2 + len;
 		} else {
 			memcpy(data, buf, len);
 			port->write_urb->transfer_buffer_length = len;
Signature · 1 line
2.53.0