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

[Johan Hovold <johan@kernel.org>]

On Tue, Dec 18, 2018 at 07:59:46PM +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. 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>
> Cc: stable@vger.kernel.org
> ---
> 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.
>  - Write general style comment for suppressing initial echoing. 
> Changes for v5:
>  - Fix: termios operation right abover the driver_data has been installed.
>  - Update commit comment about this patch affects the reconnected device
>    which get the same minor numbers.

Thanks for sticking with it.

After addressing a comment below you have my:

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

> 
>  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..336cf13 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -581,6 +581,13 @@ static int acm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>  	if (retval)
>  		goto error_init_termios;
>  
> +	/*
> +	 * 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;
> +
>  	tty->driver_data = acm;
>  
>  	return 0;
> @@ -1655,7 +1662,10 @@ static int acm_pre_reset(struct usb_interface *intf)
>  	.driver_info = NO_UNION_NORMAL, /* has no union descriptor */
>  	},
>  	{ USB_DEVICE(0x0e8d, 0x0003), /* FIREFLY, MediaTek Inc; andrey.arapov@gmail.com */
> -	.driver_info = NO_UNION_NORMAL, /* has no union descriptor */
> +	.driver_info = DISABLE_ECHO, /* DISABLE ECHO in termios flag */

I just noticed that you remove the NO_UNION_NORMAL here, which looks
wrong and definitely requires a motivation.

Thanks,
Johan