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

From: Johan Hovold <johan@kernel.org>
To: Macpaul Lin <macpaul.lin@mediatek.com>
Cc: Johan Hovold <johan@kernel.org>, Oliver Neukum <oneukum@suse.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, stable@vger.kernel.org,
	Mediatek WSD Upstream <wsd_upstream@mediatek.com>
Subject: Re: [PATCH v3] cdc-acm: fix abnormal DATA RX issue for Mediatek Preloader.
Date: Tue, 18 Dec 2018 11:54:19 +0100	[thread overview]
Message-ID: <20181218105419.GL20658@localhost> (raw)
In-Reply-To: <1545129680-3224-1-git-send-email-macpaul.lin@mediatek.com>

On Tue, Dec 18, 2018 at 06:41:20PM +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>
> ---
> Changes for v2:
>  - Move quirks testing of DISABLE_ECHO flag into acm_tty_install().
>  - Change quirks testing into bitwise comparison.
> Changes for v3:
>  - Replace clear flag operation from driver->init_termios to tty->termios.
>  - Remove parenthesis of ECHO flag.

Thanks for the update. Looks good, just some minor issues below.

>  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..c1b88c3 100644
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
> @@ -571,6 +571,7 @@ 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);
> @@ -583,6 +584,13 @@ static int acm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>  
>  	tty->driver_data = acm;
>  
> +	/* get normal quirks */
> +	quirks = acm->quirks;

I'd drop the quirks variable as you only need it once and acm->quirks is
pretty short to begin with.

> +
> +	/* handle active handshake triggered by device */

Please make this description more general; other devices may also end up
needing to suppress initial echoing.

> +	if (quirks & DISABLE_ECHO)
> +		tty->termios.c_lflag &= ~ECHO;

And finally, please move this just after installing the tty (i.e. before
setting tty->driver data).

> +
>  	return 0;

Thanks,
Johan