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

[Johan Hovold <johan@kernel.org>]

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