Re: [PATCH v2] cdc-acm: fix abnormal DATA RX issue for Mediatek Preloader.

[Johan Hovold <johan@kernel.org>]

On Tue, Dec 18, 2018 at 01:00:22PM +0800, Macpaul Lin wrote:
> Mediatek Preloader is a proprietary embedded boot loader for loading
> Little Kernel and Linux into device DRAM.
> 
> This boot loader also handle firmware update. Mediatek Preloader will be
> enumerated as a virtual COM port when the device is connected to Windows
> or Linux OS via CDC-ACM class driver. When the USB enumeration has been
> done, Mediatek Preloader will send out handshake command "READY" to PC
> actively instead of waiting command from the download tool.
> Since Linux 4.12, the commit "tty: reset termios state on device
> registration" (93857edd9829e144acb6c7e72d593f6e01aead66) causes Mediatek
> Preloader receiving some abnoraml command like "READYXX" as it sent.
> This will be recognized as an incorrect response. The behavior change
> also causes the download handshake fail.
> 
> By disabling the ECHO termios flag could avoid this problem. However, it
> cannot be done by user space configuration when download tool open
> /dev/ttyACM0. This is because the device running Mediatek Preloader will
> send handshake command "READY" immediately once the CDC-ACM driver is
> ready.
> 
> This patch wants to fix above problem by introducing "DISABLE_ECHO"
> property in driver_info. When Mediatek Preloader is connected, the
> CDC-ACM driver could disable ECHO flag in termios to avoid the problem.
> 
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> ---
> Change for v2:
>  - Move quirks testing of DISABLE_ECHO flag into acm_tty_install().
>  - Change quirks testing into bitwise comparison.
> 
>  drivers/usb/class/cdc-acm.c | 13 ++++++++++++-
>  drivers/usb/class/cdc-acm.h |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 1b68fed..f1a914d 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -571,12 +571,20 @@ static void acm_softint(struct work_struct *work)
>  static int acm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>  {
>  	struct acm *acm;
> +	unsigned long quirks;
>  	int retval;
>  
>  	acm = acm_get_by_minor(tty->index);
>  	if (!acm)
>  		return -ENODEV;
>  
> +	/* get normal quirks */
> +	quirks = acm->quirks;
> +
> +	/* handle active handshake triggered by device */
> +	if (quirks & DISABLE_ECHO)
> +		driver->init_termios.c_lflag &= ~(ECHO);

You're still changing the shared driver init_termios here, which is no
better than doing so at probe time.

What I meant by moving clearing ECHO to tty install time was that that
will allow you do clear ECHO only for the tty being installed (and
specifically not affecting the termios of other cdc-acm devices).

> +
>  	retval = tty_standard_install(driver, tty);
>  	if (retval)
>  		goto error_init_termios;

So here, after tty_standard_install() returns you can modify
tty->termios after it has been initialised to the default (or saved)
settings.

Also note that you can drop the parenthesis around ECHO above.

Thanks,
Johan