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

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

[Johan Hovold]

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

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

[macpaul.lin]

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

 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;
+
 	return 0;
 
 error_init_termios:
@@ -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 */
+	},
+	{ USB_DEVICE(0x0e8d, 0x2000), /* FIREFLY, MediaTek Inc; Preloader */
+	.driver_info = DISABLE_ECHO, /* DISABLE ECHO in termios flag */
 	},
 	{ USB_DEVICE(0x0e8d, 0x3329), /* MediaTek Inc GPS */
 	.driver_info = NO_UNION_NORMAL, /* has no union descriptor */
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index ca06b20..515aad0 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -140,3 +140,4 @@ struct acm {
 #define QUIRK_CONTROL_LINE_STATE	BIT(6)
 #define CLEAR_HALT_CONDITIONS		BIT(7)
 #define SEND_ZERO_PACKET		BIT(8)
+#define DISABLE_ECHO			BIT(9)