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

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

[Jim Paris]

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 <jim@jtan.com>
---

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(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e934e19f49f5..144bf43c9190 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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,
-- 
2.1.0

Re: [PATCH] cdc-acm: ensure that termios get set when the port is opened

[Peter Hurley]

Hi Jim,

On 10/28/2014 07:26 PM, Jim Paris wrote:
> 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 <jim@jtan.com>
> ---
> 
> 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.

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()).

Note that the ftdi_sio driver is a usb serial port driver; ftdi_open()
is called from serial_port_activate(), which is the activate() method
for all usb serial drivers.

Regards,
Peter Hurley


> ---
>  drivers/usb/class/cdc-acm.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index e934e19f49f5..144bf43c9190 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -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,
> 

[PATCH v2] cdc-acm: ensure that termios get set when the port is activated

[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 <jim@jtan.com>
---

Peter Hurley wrote:
> 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()).

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(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e934e19f49f5..24077deb737a 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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);
@@ -554,6 +605,8 @@ static int acm_port_activate(struct tty_port *port, struct tty_struct *tty)
 		goto error_submit_urb;
 	}
 
+	acm_tty_set_termios(tty, NULL);
+
 	/*
 	 * Unthrottle device in case the TTY was closed while throttled.
 	 */
@@ -949,57 +1002,6 @@ static int acm_tty_ioctl(struct tty_struct *tty,
 	return rv;
 }
 
-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 const struct tty_port_operations acm_port_ops = {
 	.dtr_rts = acm_port_dtr_rts,
 	.shutdown = acm_port_shutdown,
-- 
2.1.0

Re: [PATCH v2] cdc-acm: ensure that termios get set when the port is activated

[Johan Hovold]

On Wed, Oct 29, 2014 at 09:43:41AM -0400, Jim Paris wrote:
> 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 <jim@jtan.com>
> ---
> 
> Peter Hurley wrote:
> > 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()).
> 
> I moved it to acm_port_activate(), which works fine.  Thanks!
> acm_tty_set_termios is just moved in this patch, not changed.

Don't do that. Use a prototype instead of moving.

> Jim
> 
> ---
>  drivers/usb/class/cdc-acm.c | 104 ++++++++++++++++++++++----------------------
>  1 file changed, 53 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index e934e19f49f5..24077deb737a 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -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);
> @@ -554,6 +605,8 @@ static int acm_port_activate(struct tty_port *port, struct tty_struct *tty)
>  		goto error_submit_urb;
>  	}
>  
> +	acm_tty_set_termios(tty, NULL);
> +

Using set_termios this way also has the side-effect of raising DTR (when
baudrate != B0). This is currently not done until after the port has
been fully opened (by .dtr_rts).

This is actually a bug in set_termios which should only raise DTR on
transitions from B0. I'll fix this separately.

>  	/*
>  	 * Unthrottle device in case the TTY was closed while throttled.
>  	 */
> @@ -949,57 +1002,6 @@ static int acm_tty_ioctl(struct tty_struct *tty,
>  	return rv;
>  }

Johan

[PATCH] USB: cdc-acm: only raise DTR on transitions from B0

[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 <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e934e19f49f5..7e58bbfd6319 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -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;
+	}
 
 	if (newctrl != acm->ctrlout)
 		acm_set_control(acm, acm->ctrlout = newctrl);
-- 
2.0.4

Re: [PATCH] USB: cdc-acm: only raise DTR on transitions from B0

[Greg Kroah-Hartman]

On Wed, Oct 29, 2014 at 04:30:40PM +0100, Johan Hovold wrote:
> 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 <johan@kernel.org>
> ---
>  drivers/usb/class/cdc-acm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index e934e19f49f5..7e58bbfd6319 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -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;
> +	}
>  
>  	if (newctrl != acm->ctrlout)
>  		acm_set_control(acm, acm->ctrlout = newctrl);

This should go to older kernels as well, right?

Re: [PATCH] USB: cdc-acm: only raise DTR on transitions from B0

[Johan Hovold]

On Wed, Oct 29, 2014 at 11:56:02PM +0800, Greg Kroah-Hartman wrote:
> On Wed, Oct 29, 2014 at 04:30:40PM +0100, Johan Hovold wrote:
> > 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 <johan@kernel.org>
> > ---
> >  drivers/usb/class/cdc-acm.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> > index e934e19f49f5..7e58bbfd6319 100644
> > --- a/drivers/usb/class/cdc-acm.c
> > +++ b/drivers/usb/class/cdc-acm.c
> > @@ -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;
> > +	}
> >  
> >  	if (newctrl != acm->ctrlout)
> >  		acm_set_control(acm, acm->ctrlout = newctrl);
> 
> 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).

Johan

[PATCH v3] cdc-acm: ensure that termios get set when the port is activated

[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 <jim@jtan.com>
---

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(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e934e19f49f5..d2cd1b6d02a7 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -58,6 +58,9 @@ static struct usb_driver acm_driver;
 static struct tty_driver *acm_tty_driver;
 static struct acm *acm_table[ACM_TTY_MINORS];
 
+static void acm_tty_set_termios(struct tty_struct *tty,
+				struct ktermios *termios_old);
+
 static DEFINE_MUTEX(acm_table_lock);
 
 /*
@@ -554,6 +557,8 @@ static int acm_port_activate(struct tty_port *port, struct tty_struct *tty)
 		goto error_submit_urb;
 	}
 
+	acm_tty_set_termios(tty, NULL);
+
 	/*
 	 * Unthrottle device in case the TTY was closed while throttled.
 	 */
-- 
2.1.0

Re: [PATCH v3] cdc-acm: ensure that termios get set when the port is activated

[Johan Hovold]

On Wed, Oct 29, 2014 at 08:53:14PM -0400, Jim Paris wrote:
> 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 <jim@jtan.com>
> ---
> 
> 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(+)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index e934e19f49f5..d2cd1b6d02a7 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -58,6 +58,9 @@ static struct usb_driver acm_driver;
>  static struct tty_driver *acm_tty_driver;
>  static struct acm *acm_table[ACM_TTY_MINORS];
>  
> +static void acm_tty_set_termios(struct tty_struct *tty,
> +				struct ktermios *termios_old);
> +

Nit: Would you mind placing the prototype after all data declarations
(i.e. below acm_table_lock)?

>  static DEFINE_MUTEX(acm_table_lock);

Thanks,
Johan

[PATCH v4] cdc-acm: ensure that termios get set when the port is activated

[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 <jim@jtan.com>
---

Moved prototype.

Thanks,
Jim

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

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e934e19f49f5..d2cd1b6d02a7 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -58,6 +58,9 @@ static struct usb_driver acm_driver;
 static struct tty_driver *acm_tty_driver;
 static struct acm *acm_table[ACM_TTY_MINORS];
 
+static void acm_tty_set_termios(struct tty_struct *tty,
+				struct ktermios *termios_old);
+
 static DEFINE_MUTEX(acm_table_lock);
 
 /*
@@ -554,6 +557,8 @@ static int acm_port_activate(struct tty_port *port, struct tty_struct *tty)
 		goto error_submit_urb;
 	}
 
+	acm_tty_set_termios(tty, NULL);
+
 	/*
 	 * Unthrottle device in case the TTY was closed while throttled.
 	 */
-- 
2.1.0

Re: [PATCH v4] cdc-acm: ensure that termios get set when the port is activated

[Johan Hovold]

On Thu, Oct 30, 2014 at 10:45:38AM -0400, Jim Paris wrote:
> 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 <jim@jtan.com>
> ---
> 
> Moved prototype.

You seem to have posted the old version again.

> ---
>  drivers/usb/class/cdc-acm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index e934e19f49f5..d2cd1b6d02a7 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -58,6 +58,9 @@ static struct usb_driver acm_driver;
>  static struct tty_driver *acm_tty_driver;
>  static struct acm *acm_table[ACM_TTY_MINORS];
>  
> +static void acm_tty_set_termios(struct tty_struct *tty,
> +				struct ktermios *termios_old);
> +
>  static DEFINE_MUTEX(acm_table_lock);

Johan

[PATCH v4-real] cdc-acm: ensure that termios get set when the port is activated

[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 <jim@jtan.com>
---

Johan Hovold wrote:
> On Thu, Oct 30, 2014 at 10:45:38AM -0400, Jim Paris wrote:
> > Moved prototype.
> 
> You seem to have posted the old version again.

Doh.  Fixed.

Jim

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

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e934e19f49f5..6c358c5e05ab 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -60,6 +60,9 @@ static struct acm *acm_table[ACM_TTY_MINORS];
 
 static DEFINE_MUTEX(acm_table_lock);
 
+static void acm_tty_set_termios(struct tty_struct *tty,
+				struct ktermios *termios_old);
+
 /*
  * acm_table accessors
  */
@@ -554,6 +557,8 @@ static int acm_port_activate(struct tty_port *port, struct tty_struct *tty)
 		goto error_submit_urb;
 	}
 
+	acm_tty_set_termios(tty, NULL);
+
 	/*
 	 * Unthrottle device in case the TTY was closed while throttled.
 	 */
-- 
2.1.0

Re: [PATCH v4-real] cdc-acm: ensure that termios get set when the port is activated

[Johan Hovold]

On Thu, Oct 30, 2014 at 11:04:47AM -0400, Jim Paris wrote:
> 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 <jim@jtan.com>

Reviewed-by: Johan Hovold <johan@kernel.org>

> ---
> 
> Johan Hovold wrote:
> > On Thu, Oct 30, 2014 at 10:45:38AM -0400, Jim Paris wrote:
> > > Moved prototype.
> > 
> > You seem to have posted the old version again.
> 
> Doh.  Fixed.

Thanks,
Johan

Re: [PATCH v4-real] cdc-acm: ensure that termios get set when the port is activated

[Oliver Neukum]

On Fri, 2014-10-31 at 12:45 +0100, Johan Hovold wrote:
> On Thu, Oct 30, 2014 at 11:04:47AM -0400, Jim Paris wrote:
> > 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 <jim@jtan.com>
> 
> Reviewed-by: Johan Hovold <johan@kernel.org>
Acked-by: Oliver Neukum <oneukum@suse.de>

Re: [PATCH] USB: cdc-acm: only raise DTR on transitions from B0

[Oliver Neukum]

On Wed, 2014-10-29 at 16:58 +0100, Johan Hovold wrote:
> On Wed, Oct 29, 2014 at 11:56:02PM +0800, Greg Kroah-Hartman wrote:

> > 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).

It may not hit many people, but whom it hits, it hits hard. It
should go into stable.

	Regards
		Oliver

[PATCH Resend] USB: cdc-acm: only raise DTR on transitions from B0

[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.

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---

Greg,

I just noticed that this one did not make into usb-linus yet, although
24cb4502c97b ("cdc-acm: ensure that termios get set when the port is
activated"), which depend on this patch did.

Johan


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

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 6771f884cb82..9d6495424b06 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -985,11 +985,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;
+	}
 
 	if (newctrl != acm->ctrlout)
 		acm_set_control(acm, acm->ctrlout = newctrl);
-- 
2.0.4