[PATCH 00/11] USB: serial: digi_acceleport: registration fix and cleanups

[PATCH 00/11] USB: serial: digi_acceleport: registration fix and cleanups

[Johan Hovold]

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


Johan Hovold (11):
  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 | 299 +++++++++++----------------
 1 file changed, 117 insertions(+), 182 deletions(-)

-- 
2.53.0

[PATCH 01/11] USB: serial: digi_acceleport: fix port registration order

[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: stable@vger.kernel.org	# 3.7
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/digi_acceleport.c | 43 ++++++++++++++--------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index a75813a57fbe..d10d60b5d356 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -1096,6 +1096,13 @@ 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);
 	}
+
+	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;
 }
 
@@ -1107,6 +1114,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)
@@ -1169,15 +1178,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);
@@ -1188,19 +1197,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)
@@ -1252,13 +1255,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);
 }
 
 
-- 
2.53.0

[PATCH 02/11] USB: serial: digi_acceleport: drop unused wait queue

[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 <johan@kernel.org>
---
 drivers/usb/serial/digi_acceleport.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index d10d60b5d356..16ced067c5f9 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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;
 };
@@ -1169,7 +1168,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);
 }
@@ -1218,7 +1216,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;
 
-- 
2.53.0

[PATCH 03/11] USB: serial: digi_acceleport: always stop write urb on close

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

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 16ced067c5f9..7195a63da8a1 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -1116,7 +1116,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;
 
@@ -1162,14 +1161,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);
 }
 
 
-- 
2.53.0

[PATCH 04/11] USB: serial: digi_acceleport: add oob port helper

[Johan Hovold]

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

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

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 7195a63da8a1..80b8863b7942 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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;
-- 
2.53.0

[PATCH 05/11] USB: serial: digi_acceleport: clean up declarations and whitespace

[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 <johan@kernel.org>
---
 drivers/usb/serial/digi_acceleport.c | 88 +++++++++-------------------
 1 file changed, 29 insertions(+), 59 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 80b8863b7942..7a198c4a9e28 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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)
@@ -1112,13 +1093,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);
 
@@ -1175,14 +1155,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);
@@ -1252,7 +1230,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);
@@ -1262,7 +1239,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;
@@ -1294,8 +1270,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)
@@ -1337,7 +1313,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);
 	}
-
 }
 
 /*
@@ -1349,7 +1324,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;
@@ -1427,10 +1401,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
  *
@@ -1439,10 +1411,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;
@@ -1450,8 +1420,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;
@@ -1521,8 +1491,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);
-- 
2.53.0

[PATCH 06/11] USB: serial: digi_acceleport: drop redundant driver data sanity checks

[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 <johan@kernel.org>
---
 drivers/usb/serial/digi_acceleport.c | 40 +++-------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 7a198c4a9e28..e14cf133808f 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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");
@@ -1268,27 +1252,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,
-- 
2.53.0

[PATCH 07/11] USB: serial: digi_acceleport: stop OOB I/O when not in use

[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 <johan@kernel.org>
---
 drivers/usb/serial/digi_acceleport.c | 100 ++++++++++++++-------------
 1 file changed, 51 insertions(+), 49 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index e14cf133808f..efa853a57bb2 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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;
@@ -1071,10 +1104,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)
@@ -1137,36 +1175,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)
@@ -1198,7 +1208,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 = serial->type->num_ports;
 	serial_priv->ds_oob_port = serial->port[serial_priv->ds_oob_port_num];
 
@@ -1214,15 +1225,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;
-- 
2.53.0

[PATCH 08/11] USB: serial: digi_acceleport: drop unused in-buf define

[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 <johan@kernel.org>
---
 drivers/usb/serial/digi_acceleport.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index efa853a57bb2..6db07fa56a17 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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)
 
-- 
2.53.0

[PATCH 09/11] USB: serial: digi_acceleport: clean up xfer buf length expression

[Johan Hovold]

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

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

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 6db07fa56a17..e89e12264f57 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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;
-- 
2.53.0

[PATCH 10/11] USB: serial: digi_acceleport: clean up write completion

[Johan Hovold]

Clean up the write completion callback 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 <johan@kernel.org>
---
 drivers/usb/serial/digi_acceleport.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index e89e12264f57..9fcb53eb82d8 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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;
-- 
2.53.0

[PATCH 11/11] USB: serial: digi_acceleport: clean up inb command submission

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

diff --git a/drivers/usb/serial/digi_acceleport.c b/drivers/usb/serial/digi_acceleport.c
index 9fcb53eb82d8..2b755b328fe6 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -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;
-- 
2.53.0