cdc-acm: ensure that termios get set when the port is opened

Do what other drivers like ftdi_sio do, and ensure that termios are written to the device when the port is first opened.

Signed-off-by Jim Paris <[email protected]>

---

Tested on v3.16.5.

I've seen a problem on two CDC-ACM systems based on a Segger J-Link where the port does not get initialized at the correct baudrate when opening (using e.g. python-serial). I think this occurs when the tty device was previously opened at the same baudrate, then the device was unplugged and replugged. While the port is open, manually switching to a different baudrate and back fixes it.

Debug output in the failing case is e.g.:

  Oct 28 18:37:45 pilot kernel: [1214446.586460] tty ttyACM0: acm_tty_install
  Oct 28 18:37:45 pilot kernel: [1214446.586474] tty ttyACM0: acm_tty_open
  Oct 28 18:37:45 pilot kernel: [1214446.586477] cdc_acm 1-2.7:1.0: acm_port_activate
  Oct 28 18:37:45 pilot kernel: [1214446.586670] cdc_acm 1-2.7:1.0: acm_ctrl_msg - rq 0x22, val 0x3, len 0x0, result 0

which is missing the important:

  Oct 28 19:03:18 pilot kernel: [1215981.178020] cdc_acm 1-2.7:1.0: acm_tty_set_termios - set line: 38400 0 0 8
  Oct 28 19:03:18 pilot kernel: [1215981.178135] cdc_acm 1-2.7:1.0: acm_ctrl_msg - rq 0x20, val 0x0, len 0x7, result 7

that I get when changing settings to something different than they previously were.

I don't really follow all of the termios and tty stuff, so I don't know if this is the right fix or the real cause. I suspect it have to do with cached values associated with the particular TTY ("lazy saved data" in tty-io.c); this patch just does what I see in ftdi_sio and ensures that the termios settings are written to the device when the port is opened.

---
 drivers/usb/class/cdc-acm.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

drivers/usb/class/cdc-acm.c +12 -9

index e934e19f49f5..144bf43c9190 100644
@@ -495,15 +495,6 @@ error_init_termios:
 	return retval;
 }
 
-static int acm_tty_open(struct tty_struct *tty, struct file *filp)
-{
-	struct acm *acm = tty->driver_data;
-
-	dev_dbg(tty->dev, "%s\n", __func__);
-
-	return tty_port_open(&acm->port, tty, filp);
-}
-
 static void acm_port_dtr_rts(struct tty_port *port, int raise)
 {
 	struct acm *acm = container_of(port, struct acm, port);

@@ -1000,6 +991,18 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 	}
 }
 
+static int acm_tty_open(struct tty_struct *tty, struct file *filp)
+{
+	struct acm *acm = tty->driver_data;
+
+	dev_dbg(tty->dev, "%s\n", __func__);
+
+	if (tty)
+		acm_tty_set_termios(tty, NULL);
+
+	return tty_port_open(&acm->port, tty, filp);
+}
+
 static const struct tty_port_operations acm_port_ops = {
 	.dtr_rts = acm_port_dtr_rts,
 	.shutdown = acm_port_shutdown,

Yeah, you're right that the cdc-acm driver isn't properly configuring the hardware for the current termios settings under all conditions.

But you don't want to do it for every tty open, only for opens requiring port initialization, which is what the tty_port->activate() method is for (ie., acm_port_activate()).

The driver wasn't properly configuring the hardware for the current termios settings under all conditions. Ensure that termios are written to the device when the port is activated.

Signed-off-by Jim Paris <[email protected]>

---

Quoted 3 lines · Peter Hurley

Yeah, you're right that the cdc-acm driver isn't properly configuring the hardware for the current termios settings under all conditions.

Quoted 3 lines

But you don't want to do it for every tty open, only for opens requiring port initialization, which is what the tty_port->activate() method is for (ie., acm_port_activate()).

I moved it to acm_port_activate(), which works fine. Thanks! acm_tty_set_termios is just moved in this patch, not changed.

Jim

---
 drivers/usb/class/cdc-acm.c | 104 ++++++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 51 deletions(-)

drivers/usb/class/cdc-acm.c +51 -0

index e934e19f49f5..24077deb737a 100644
@@ -504,6 +504,57 @@ static int acm_tty_open(struct tty_struct *tty, struct file *filp)
 	return tty_port_open(&acm->port, tty, filp);
 }
 
+static void acm_tty_set_termios(struct tty_struct *tty,
+						struct ktermios *termios_old)
+{
+	struct acm *acm = tty->driver_data;
+	struct ktermios *termios = &tty->termios;
+	struct usb_cdc_line_coding newline;
+	int newctrl = acm->ctrlout;
+
+	newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty));
+	newline.bCharFormat = termios->c_cflag & CSTOPB ? 2 : 0;
+	newline.bParityType = termios->c_cflag & PARENB ?
+				(termios->c_cflag & PARODD ? 1 : 2) +
+				(termios->c_cflag & CMSPAR ? 2 : 0) : 0;
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		newline.bDataBits = 5;
+		break;
+	case CS6:
+		newline.bDataBits = 6;
+		break;
+	case CS7:
+		newline.bDataBits = 7;
+		break;
+	case CS8:
+	default:
+		newline.bDataBits = 8;
+		break;
+	}
+	/* FIXME: Needs to clear unsupported bits in the termios */
+	acm->clocal = ((termios->c_cflag & CLOCAL) != 0);
+
+	if (!newline.dwDTERate) {
+		newline.dwDTERate = acm->line.dwDTERate;
+		newctrl &= ~ACM_CTRL_DTR;
+	} else
+		newctrl |=  ACM_CTRL_DTR;
+
+	if (newctrl != acm->ctrlout)
+		acm_set_control(acm, acm->ctrlout = newctrl);
+
+	if (memcmp(&acm->line, &newline, sizeof newline)) {
+		memcpy(&acm->line, &newline, sizeof newline);
+		dev_dbg(&acm->control->dev, "%s - set line: %d %d %d %d\n",
+			__func__,
+			le32_to_cpu(newline.dwDTERate),
+			newline.bCharFormat, newline.bParityType,
+			newline.bDataBits);
+		acm_set_line(acm, &acm->line);
+	}
+}
+
 static void acm_port_dtr_rts(struct tty_port *port, int raise)
 {
 	struct acm *acm = container_of(port, struct acm, port);

Make sure to only raise DTR on transitions from B0 in set_termios.

Also allow set_termios to be called from open with a termios_old of NULL. Note that DTR will not be raised prematurely in this case.

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

---
 drivers/usb/class/cdc-acm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/usb/class/cdc-acm.c +0 -1

index e934e19f49f5..7e58bbfd6319 100644
@@ -980,11 +980,12 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 	/* FIXME: Needs to clear unsupported bits in the termios */
 	acm->clocal = ((termios->c_cflag & CLOCAL) != 0);
 
-	if (!newline.dwDTERate) {

Quoted 27 lines · Johan Hovold

Make sure to only raise DTR on transitions from B0 in set_termios.

Also allow set_termios to be called from open with a termios_old of NULL. Note that DTR will not be raised prematurely in this case.

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

---
 drivers/usb/class/cdc-acm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/usb/class/cdc-acm.c +3 -2

index e934e19f49f5..7e58bbfd6319 100644
@@ -980,11 +980,12 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 	/* FIXME: Needs to clear unsupported bits in the termios */
 	acm->clocal = ((termios->c_cflag & CLOCAL) != 0);
 
-	if (!newline.dwDTERate) {
+	if (C_BAUD(tty) == B0) {
 		newline.dwDTERate = acm->line.dwDTERate;
 		newctrl &= ~ACM_CTRL_DTR;
-	} else
+	} else if (termios_old && (termios_old->c_cflag & CBAUD) == B0) {
 		newctrl |=  ACM_CTRL_DTR;
+	}
 

drivers/usb/class/cdc-acm.c static void acm_tty_set_termios(struct tty_struct *tty, · line 991

Quoted 2 lines

drivers/usb/class/cdc-acm.c +0 -0

 	if (newctrl != acm->ctrlout)
 		acm_set_control(acm, acm->ctrlout = newctrl);

This should go to older kernels as well, right?

The driver wasn't properly configuring the hardware for the current termios settings under all conditions. Ensure that termios are written to the device when the port is activated.

Signed-off-by Jim Paris <[email protected]>

---

Switched to Johan's suggestion of using a prototype rather than moving acm_tty_set_termios. This depends on his patch in order to get proper DTR handling.

Thanks,
Jim

---
 drivers/usb/class/cdc-acm.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/usb/class/cdc-acm.c +0 -0

index e934e19f49f5..d2cd1b6d02a7 100644

The driver wasn't properly configuring the hardware for the current termios settings under all conditions. Ensure that termios are written to the device when the port is activated.

Signed-off-by Jim Paris <[email protected]>

---

Moved prototype.

---
 drivers/usb/class/cdc-acm.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/usb/class/cdc-acm.c +0 -0

index e934e19f49f5..d2cd1b6d02a7 100644

Quoted 1 line · Jim Paris

Moved prototype.

You seem to have posted the old version again.

The driver wasn't properly configuring the hardware for the current termios settings under all conditions. Ensure that termios are written to the device when the port is activated.

Signed-off-by Jim Paris <[email protected]>

---

Quoted 4 lines · Johan Hovold

Quoted 1 line · Jim Paris

Moved prototype.

You seem to have posted the old version again.

Doh. Fixed.

Quoted 5 lines · Jim Paris

The driver wasn't properly configuring the hardware for the current termios settings under all conditions. Ensure that termios are written to the device when the port is activated.

Signed-off-by Jim Paris <[email protected]>

Reviewed-by Johan Hovold <[email protected]>

Quoted 1 line · Greg Kroah-Hartman

This should go to older kernels as well, right?

Yes, if you want.

It's fixing handling of B0, but I doubt many people care (hence the missing stable tag). Note that set_termios is currently not called during open() (but Jim's patch will be relying on this one).