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

From: Johan Hovold
Date: Wed Oct 29 2014 - 11:10:29 EST


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@xxxxxxxx>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/