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

[Johan Hovold <johan@kernel.org>]

On Tue, Dec 18, 2018 at 07:38:07PM +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.

Please mention that this change only affects subsequent connects if the
reconnected device happens to get the same minor number.

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

And you can add a CC stable tag here.

> ---
> Changes for v2:
>  - Move quirks testing of DISABLE_ECHO flag into acm_tty_install().
>  - Change quirks testing into bitwise comparison.
> Changes for v3:
>  - Replace quirks testing from init_termios to tty->termios.
>  - Remove parenthesis for ECHO flag.
> Changes for v4:
>  - Drop quirks varible to simplify the patch.
>  - Move termios operation right after the driver_data has been installed.

No, it should go after the *tty* has been installed (and *before*
setting driver_data)

>  - Write general style comment for suppressing initial echoing. 

>  drivers/usb/class/cdc-acm.c | 12 +++++++++++-
>  drivers/usb/class/cdc-acm.h |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
> index 1b68fed..ab8609e 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -583,6 +583,13 @@ static int acm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>  
>  	tty->driver_data = acm;
>  
> +	/*
> +	 * Suppress initial echoing for some devices which might send data
> +	 * immediately after acm driver has been installed.
> +	 */
> +	if (acm->quirks & DISABLE_ECHO)
> +		tty->termios.c_lflag &= ~ECHO;

You forgot to move this above setting tty->driver data.

> +
>  	return 0;

Thanks,
Johan