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 <[email protected]>
> ---
> 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
