On Fri, 2018-12-14 at 12:07 +0100, Johan Hovold wrote:
> On Fri, Dec 14, 2018 at 10:00:16AM +0800, Macpaul Lin wrote:
> > On Thu, 2018-12-13 at 11:18 +0100, Johan Hovold wrote:
> > > On Thu, Dec 13, 2018 at 11:13:54AM +0100, Oliver Neukum wrote:
> > > > On Do, 2018-12-13 at 10:43 +0100, Johan Hovold wrote:
> > > > > On Thu, Dec 13, 2018 at 11:27:56AM +0800, macpaul.lin@xxxxxxxxxxxx wrote:
> > > > > > From: Macpaul Lin <macpaul.lin@xxxxxxxxxxxx>
> > > > > >
> > > > > > + /* handle active handshake triggered by device */
> > > > > > + if (quirks == DISABLE_ECHO)
> > > > > > + acm_tty_driver->init_termios.c_lflag &= ~(ECHO);
> > > > >
> > > > > You cannot change the driver init_termios like this as that will affect
> > > > > all cdc-acm devices that are probed later. If this is at all needed,
> > > > > this will have to be done at tty install time.
> > > >
> > > > Right. How do with decide on a sensible default anyway?
> > >
> > > I think the current defaults are sensible. They are based on
> > > tty_std_termios, which has ECHO set, as for most (all?) tty drivers.
>
> > Well, the problem is that the phone device (preloader) will get
> > "READYXX" even "no any download tool has ever been launched on PC".
>
> Something much hold the port open on the host (PC) side for it to be
> echoed back, but perhaps the outgoing data is buffered in the device
> until it is eventually opened.
>
> > After the reset termios state change simply set EHCO enable, each time
> > the phone device simple send out "READY" to PC after USB has been
> > connected. The "READY" indication has no any "\0" character at the end
> > of the command string, hence it will receive some dirty data (ex.
> > "READYXX") back on RX path.
>
> So you have a firmware bug which sends out some garbage characters
> ("XX")?
No, "XX" just meant any random data of any possible length, not the real
characters "XX". From the bottom of hardware FIIO, usb driver buffers,
to the higher level CDC driver and download handshake flow, the firmware
keeps separated RX and TX data buffers. The initial command "READY" is
static declared and only been used once when the handshake flow starts
in stead of dynamic allocated. We've also tested memory set at the time
when RX/TX buffer is allocated when system boot, but the problem did not
recovered.
> Either way, you should have hit this also before the commit which
> started resetting the terminal settings whenever a device was plugged
> in instead of reusing a potentially random other device's settings.
We've also reviewed the firmware code, too. This mechanism of download
flow (firmware code) has been used more than 10 years and has never been
changed. Our phone product ships 100 million per year. What I mean here
is this download flow has been verified in the factory producing line
both on Windows and Linux PC so many time. Hence the possibility of bug
in the data buffer management is very rare.
This problem has been reported by one of our customer about 1 month ago
because they upgraded their factory PC from Ubuntu 16.04 (Linux 4.4) to
Ubuntu 18.04. (Linux 4.15). Hence Mediatek has done lots of bisect test
from 4.4 release to 4.15 on both tty and cdc-acm changes to find out
which commit caused behavior change on PC side. Because the new reset
termios state method comes with another 2 patches. We also tested these
patch separately to confirm the bisect result.
tty: reset termios state on device registration
tty: drop obsolete termios_locked comments
tty: close race between device register and open
We've also remove reset termios state commit on latest 4.19 kernel to
confirm the behavior of PC is back to work for the download, too.
> However, after clearing echo and replugging the device, nothing would
> have been echoed back on next open, but only *if* the device happened to
> be assigned the same minor number.
>
> But if this confuses your firmware and there's no way to restart the
> handshake in your host tool, perhaps clearing ECHO at tty install time
> (i.e. in tty_acm_install()) is the only way to deal with this.
Thanks for you comment!
I'll make a change to move the clearing ECHO into tty_acm_install() and
to check if it also work for the download process. Thanks a lot!
> Johan
Regards,
Macpaul Lin
