[PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Andrew Worsley]

ftdi_set_termios() is always setting the baud rate and data bits and
parity on every call. When called while characters are being
transmitted can cause the FTDI chip to corrupt the serial port bit
stream by stalling the output half a bit during the output of a
character.
Simple fix by skipping this setting if the baud rate/data bits/parity
are unchanged.

Signed-off-by: Andrew Worsley <amworsley@gmail.com>
----

This bug was observed on 2.6.32 kernel (this patch is ported to latest
kernel for ease of review).
Using a FTDI USB serial chip at 38400 repeatedly generating output by
running a simple command such as "uname -a" or "echo Linux"  gives
occasional corruption on the output

...
> echo Linux
L�nux

 Andrew:~
> echo Linux
Linux

 Andrew:~
> echo Linux
�inux

 Andrew:~
> echo Linux
Linux

 Andrew:~
>
....

  After tracing the USB URBs sent and looking at the bits coming out
of the serial port it was found that the glitch involves a delay in
the middle of a character of one of the bits by half a bit time
causing incorrect characters to be displayed. i.e. one of the bits is
stretched. I noticed that ftdi_set_termios() was repeatedly called
after line of input and would set the data length (8 bits no parity),
baud rate, and control flow.
It seems this often just hits the chip as it is transmitting the
output and presumably the chip doesn't like having all these
parameters being set while transmitting and causing the glitch. The
following fixed the problem by not doing the FTDI_SIO_SET_DATA	and
FTDI_SIO_SET_BAUD_RATE if they are not required.

I don't know why it repeatedly tries to set this all the time - it
would appear to be quite a lot of work so perhaps there is something
else that could be cleaned up. This was the simplest safe change that
fixed my problem. It appears this code hasn't changed very much since
the first history information in git that I could see - so perhaps
nobody else is really noticing this issue for some reason?

Comments / suggestions appreciated

Thanks

Andrew

To be applied against

commit 396e6e49c58bb23d1814d3c240c736c9f01523c5
Merge: 1897436 6ad390a
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu Oct 27 08:44:20 2011 +0200

    Merge branch 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input

    * 'for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input: (68 commits)
      Input: adp5589-keys - add support for the ADP5585 derivatives
      Input: imx_keypad - add pm suspend and resume support
...

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 8fe034d..bda9e5b 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2104,9 +2104,10 @@ static void ftdi_set_termios(struct tty_struct *tty,

        cflag = termios->c_cflag;

-       /* FIXME -For this cut I don't care if the line is really changing or
-          not  - so just do the change regardless  - should be able to
-          compare old_termios and tty->termios */
+       /* compare old_termios and tty->termios */
+    if (old_termios->c_cflag == termios->c_cflag)
+        goto no_c_cflag_changes;
+
        /* NOTE These routines can get interrupted by
           ftdi_sio_read_bulk_callback  - need to examine what this means -
           don't see any problems yet */
@@ -2176,6 +2177,7 @@ static void ftdi_set_termios(struct tty_struct *tty,
                        set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
        }

+no_c_cflag_changes:
        /* Set flow control */
        /* Note device also supports DTR/CD (ugh) and Xon/Xoff in hardware */
        if (cflag & CRTSCTS) {

Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Uwe Bonnes]

>>>>> "Andrew" == Andrew Worsley <amworsley@gmail.com> writes:

    Andrew> ftdi_set_termios() is always setting the baud rate and data bits
    Andrew> and parity on every call. When called while characters are being
    Andrew> transmitted can cause the FTDI chip to corrupt the serial port
    Andrew> bit stream by stalling the output half a bit during the output
    Andrew> of a character.  Simple fix by skipping this setting if the baud
    Andrew> rate/data bits/parity are unchanged.

    Andrew> Signed-off-by: Andrew Worsley <amworsley@gmail.com> ----

    Andrew> This bug was observed on 2.6.32 kernel (this patch is ported to
    Andrew> latest kernel for ease of review).  Using a FTDI USB serial chip
    Andrew> at 38400 repeatedly generating output by running a simple
    Andrew> command such as "uname -a" or "echo Linux" gives occasional
    Andrew> corruption on the output

    Andrew> ...
    >> echo Linux
    Andrew> L^[$,3u=^[(Bnux

Could you please give a more specific receipe to reproduce the bug. Running
with an adapter with TX/RX shorted at "stty -F /dev/ttyUSB0" 38400", several
> uname -a /dev/ttyUSB0
runs gave no artifact in a "seyon -modem /dev/ttyUSB0" console.

Doing 
> uname -a /dev/ttyUSB0
in one xterm and receiving with
>cat /dev/ttyUSB0
in another xterm gives lot of repeated lines in the receiving terminal. But
I have the same behaviour with /dev/ttyS0 on a real serial port on the
mainboard.

Otherwise, shouldn't all control transfers to the FTDI be only done with the
tx buffer in the FTDI empty? I guess the RX buffer is not affected by some
change. But this is something to be done with great knowledge...

Bye
-- 
Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------

Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Andrew Worsley]

On 28 October 2011 21:50, Uwe Bonnes
<bon@elektron.ikp.physik.tu-darmstadt.de> wrote:
>>>>>> "Andrew" == Andrew Worsley <amworsley@gmail.com> writes:
>
>    Andrew> ftdi_set_termios() is always setting the baud rate and data bits
>    Andrew> and parity on every call. When called while characters are being
>    Andrew> transmitted can cause the FTDI chip to corrupt the serial port
>    Andrew> bit stream by stalling the output half a bit during the output
>    Andrew> of a character.  Simple fix by skipping this setting if the baud
>    Andrew> rate/data bits/parity are unchanged.
>
>    Andrew> Signed-off-by: Andrew Worsley <amworsley@gmail.com> ----
>
>    Andrew> This bug was observed on 2.6.32 kernel (this patch is ported to
>    Andrew> latest kernel for ease of review).  Using a FTDI USB serial chip
>    Andrew> at 38400 repeatedly generating output by running a simple
>    Andrew> command such as "uname -a" or "echo Linux" gives occasional
>    Andrew> corruption on the output
>
>    Andrew> ...
>    >> echo Linux
>    Andrew> L�3u=nux
>
> Could you please give a more specific receipe to reproduce the bug. Running
> with an adapter with TX/RX shorted at "stty -F /dev/ttyUSB0" 38400", several
>> uname -a /dev/ttyUSB0
> runs gave no artifact in a "seyon -modem /dev/ttyUSB0" console.
>
> Doing
>> uname -a /dev/ttyUSB0
> in one xterm and receiving with
>>cat /dev/ttyUSB0
> in another xterm gives lot of repeated lines in the receiving terminal. But
> I have the same behaviour with /dev/ttyS0 on a real serial port on the
> mainboard.
>
> Otherwise, shouldn't all control transfers to the FTDI be only done with the
> tx buffer in the FTDI empty? I guess the RX buffer is not affected by some
> change. But this is something to be done with great knowledge...
>
Thank you for your testing efforts. In a sense I am not surprised that
an obvious attempt at reproduction doesn't show it as I would have
thought this bug would have been found long ago if it
did. At the moment the best I can do is describe my configuration and
invite requests for details on additional fields that you think
important.

We use the ftdi chip to provide a serial console so the program I am
running on it is
/sbin/getty -L ttyUSB0 38400. I can't give you the exact stty settings
the defaults set up as I
am at home but ixon was set but clearing it didn't help.

My key detective was to configure CONFIG_USB_MON and look at the
requests going to the port.
There were always 3 Control requests after each carriage return which
set speed/baud rate/flow control which seemed kinda crazy.

Perhaps it was something to do with the prompt which had an escape
sequence in it as other people noticed that playing with the delay of
the prompt could clear it. Alternatively the more modern kernel may
not illustrate the same issue - due to some fix that avoids or
minimises  the repeated calls of ftdi_set_termios() ?

I suspect the configuration of the tty line is critical and is
something I don't understand as it appears to be constantly adjusting
the settings but only changing the flow control.I don't have the exact
settings in front of me so from memory it appeared to be cycling
through

IXON, IXOFF and may be one other perhaps IXANY or possibly IUCLC?

If people are interested in any particular details I can send you
them, next week. Including
stack traces, CONFIG_USB_MON traces, screen shot of CRO with corrupted
character + other requested
info

But what I don't understand is the logic that drives
ftdi_set_termios() repeated calls with the flow control changes. I
would appreciate an explanation of this and others might be interested
in this if they are trying to optimise the kernel on serial I/O.

Even if such constant calling is justified (some how XON/XOFF is being
implemented?) I think it should be harmless (and more efficient) not
to skip setting the BAUD rate and other data parameters which is a
significant amount of work. And in my case this extract work appears
to be
glitch the FTDI chip.

Andrew

Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Alan Cox]

> I don't know why it repeatedly tries to set this all the time - it
> would appear to be quite a lot of work so perhaps there is something
> else that could be cleaned up. This was the simplest safe change that
> fixed my problem. It appears this code hasn't changed very much since
> the first history information in git that I could see - so perhaps
> nobody else is really noticing this issue for some reason?

It could be a problem specific to some firmware or revision. We've had
a similar quirk with a different USB adapter. The actual calls to keep
changing it are coming from your application however.

>         cflag = termios->c_cflag;
> 
> -       /* FIXME -For this cut I don't care if the line is really
> changing or
> -          not  - so just do the change regardless  - should be able
> to
> -          compare old_termios and tty->termios */
> +       /* compare old_termios and tty->termios */
> +    if (old_termios->c_cflag == termios->c_cflag)
> +        goto no_c_cflag_changes;

You can't do it this way because the speed data is not entirely within
c_cflag. Check c_ispeed and c_ospeed match and for the parity if you want to skip
that check if the parity bits change specifically.

Alan

Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Andrew Worsley]

Avoid unnecessary Control URBs that reset the data/parity or baud rate
to the currently set settings which can cause the FTDI chip to glitch
it's serial output and cause a corruption of a character it is
currently outputting.

Signed-off-by: amworsley@gmail.com

---

On 1 November 2011 22:36, Alan Cox <alan@linux.intel.com> wrote:
>> I don't know why it repeatedly tries to set this all the time - it
>> would appear to be quite a lot of work so perhaps there is something
>> else that could be cleaned up. This was the simplest safe change that
>> fixed my problem. It appears this code hasn't changed very much since
>> the first history information in git that I could see - so perhaps
>> nobody else is really noticing this issue for some reason?
>
> It could be a problem specific to some firmware or revision. We've had
> a similar quirk with a different USB adapter. The actual calls to keep
> changing it are coming from your application however.

"My" application is getty via inittab e.g.
T0:23:respawn:/sbin/getty -L ttyUSB0 115200 vt100

  Something I would expect to be fairly common. Actually after login
it is "bash" which is running presumably the getty set the tty
parameters like speed and flow control.

>
>>         cflag = termios->c_cflag;
>>
>> -       /* FIXME -For this cut I don't care if the line is really
>> changing or
>> -          not  - so just do the change regardless  - should be able
>> to
>> -          compare old_termios and tty->termios */
>> +       /* compare old_termios and tty->termios */
>> +    if (old_termios->c_cflag == termios->c_cflag)
>> +        goto no_c_cflag_changes;
>
> You can't do it this way because the speed data is not entirely within
> c_cflag. Check c_ispeed and c_ospeed match and for the parity if you want to skip
> that check if the parity bits change specifically.
>
This is getting into magic flags I don't understand. There are so few
bits in c_cflag not related to speed and data/parity I am hesitant  to
write a complex check I might well get wrong. But flow control appears
to be switched off / on frequently during data flow,
my guess as part of flow controlling the input and I suggest a speed
change is likely to be a once per login or line usage occurrence.

Here's a more sophisticated version of this which attempts to do this.
It works on my 2.6.32 kernel and compiles the latest kernel.

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 8fe034d..66a2d09 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2104,13 +2104,20 @@ static void ftdi_set_termios(struct tty_struct *tty,

        cflag = termios->c_cflag;

-       /* FIXME -For this cut I don't care if the line is really changing or
-          not  - so just do the change regardless  - should be able to
-          compare old_termios and tty->termios */
+       /* compare old_termios and tty->termios */
+    if (old_termios->c_cflag == termios->c_cflag
+      && old_termios->c_ispeed == termios->c_ispeed
+      && old_termios->c_ospeed == termios->c_ospeed
+      )
+        goto no_c_cflag_changes;
+
        /* NOTE These routines can get interrupted by
           ftdi_sio_read_bulk_callback  - need to examine what this means -
           don't see any problems yet */

+    if ((old_termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)) ==
+        (termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)))
+        goto no_data_parity_stop_changes;
        /* Set number of data bits, parity, stop bits */

        urb_value = 0;
@@ -2150,6 +2157,7 @@ static void ftdi_set_termios(struct tty_struct *tty,
                        "databits/stopbits/parity\n", __func__);
        }

+no_data_parity_stop_changes:
        /* Now do the baudrate */
        if ((cflag & CBAUD) == B0) {
                /* Disable flow control */
@@ -2176,6 +2184,7 @@ static void ftdi_set_termios(struct tty_struct *tty,
                        set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
        }

+no_c_cflag_changes:
        /* Set flow control */
        /* Note device also supports DTR/CD (ugh) and Xon/Xoff in hardware */
        if (cflag & CRTSCTS) {

Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Alan Cox]

> >> +       /* compare old_termios and tty->termios */
> >> +    if (old_termios->c_cflag == termios->c_cflag)
> >> +        goto no_c_cflag_changes;
> >
> > You can't do it this way because the speed data is not entirely
> > within c_cflag. Check c_ispeed and c_ospeed match and for the
> > parity if you want to skip that check if the parity bits change
> > specifically.
> >
> This is getting into magic flags I don't understand. There are so few
> bits in c_cflag not related to speed and data/parity I am hesitant  to
> write a complex check I might well get wrong. But flow control appears
> to be switched off / on frequently during data flow,

It shouldn't be unless the apps you are running are doing odd things.
I'd not expect parity to keep changing certainly. What may be occuring
is that some of these devices only handle RTS/CTS flow and force the
flag on, indicating it back to the app. If the app ignores that then it
may end up trying to clear it several times.

> +    if (old_termios->c_cflag == termios->c_cflag
> +      && old_termios->c_ispeed == termios->c_ispeed
> +      && old_termios->c_ospeed == termios->c_ospeed
> +      )
> +        goto no_c_cflag_changes;

That is safe enough

> +
>         /* NOTE These routines can get interrupted by
>            ftdi_sio_read_bulk_callback  - need to examine what this
> means - don't see any problems yet */
> 
> +    if ((old_termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)) ==
> +        (termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)))

I think you need

CSIZE (for CS7/8 switch)
PARODD (parity odd/even)
CMSPAR (parity mark/space v odd/even)
CSTOPB (stop bits)

while you have PARODD twice.

Otherwise this looks correct.

Alan

Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Andrew Worsley]

Avoid unnecessary Control URBs that reset the data/parity or baud rate
to the currently set settings which can cause the FTDI chip to glitch
it's serial output and cause a corruption of a character it is
currently outputting.

Signed-off-by: amworsley@gmail.com

---

....
> It shouldn't be unless the apps you are running are doing odd things.
> I'd not expect parity to keep changing certainly. What may be occuring
> is that some of these devices only handle RTS/CTS flow and force the
> flag on, indicating it back to the app. If the app ignores that then it
> may end up trying to clear it several times.

Okay - Thanks I will try stracing the process and see if it is issuing
all the requests. I thought
it might be the line discipline trying to flow control the input. I've
lost my box for the moment so that
will be tomorrow perhaps.

....

>> +
>>         /* NOTE These routines can get interrupted by
>>            ftdi_sio_read_bulk_callback  - need to examine what this
>> means - don't see any problems yet */
>>
>> +    if ((old_termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)) ==
>> +        (termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)))
>
> I think you need
>
> CSIZE (for CS7/8 switch)
> PARODD (parity odd/even)
> CMSPAR (parity mark/space v odd/even)
> CSTOPB (stop bits)
>
> while you have PARODD twice.
>
> Otherwise this looks correct.


  Woops - yep that was wrong. I attach a new patch - with CMSPAR and
PARENB which I assume is also parity related.

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index bda9e5b..89dbf8a 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -2105,13 +2105,19 @@ static void ftdi_set_termios(struct tty_struct *tty,
        cflag = termios->c_cflag;

        /* compare old_termios and tty->termios */
-    if (old_termios->c_cflag == termios->c_cflag)
+    if (old_termios->c_cflag == termios->c_cflag
+      && old_termios->c_ispeed == termios->c_ispeed
+      && old_termios->c_ospeed == termios->c_ospeed
+      )
         goto no_c_cflag_changes;

        /* NOTE These routines can get interrupted by
           ftdi_sio_read_bulk_callback  - need to examine what this means -
           don't see any problems yet */

+    if ((old_termios->c_cflag & (CSIZE|PARODD|PARENB|CMSPAR|CSTOPB)) ==
+        (termios->c_cflag & (CSIZE|PARODD|PARENB|CMSPAR|CSTOPB)))
+        goto no_data_parity_stop_changes;
        /* Set number of data bits, parity, stop bits */

        urb_value = 0;
@@ -2151,6 +2157,7 @@ static void ftdi_set_termios(struct tty_struct *tty,
                        "databits/stopbits/parity\n", __func__);
        }

+no_data_parity_stop_changes:
        /* Now do the baudrate */
        if ((cflag & CBAUD) == B0) {
                /* Disable flow control */

Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c

[Greg KH]

On Thu, Nov 03, 2011 at 04:06:31PM +1100, Andrew Worsley wrote:
> Avoid unnecessary Control URBs that reset the data/parity or baud rate
> to the currently set settings which can cause the FTDI chip to glitch
> it's serial output and cause a corruption of a character it is
> currently outputting.
> 
> Signed-off-by: amworsley@gmail.com
> 
> ---
> 
> ....
> > It shouldn't be unless the apps you are running are doing odd things.
> > I'd not expect parity to keep changing certainly. What may be occuring
> > is that some of these devices only handle RTS/CTS flow and force the
> > flag on, indicating it back to the app. If the app ignores that then it
> > may end up trying to clear it several times.
> 
> Okay - Thanks I will try stracing the process and see if it is issuing
> all the requests. I thought
> it might be the line discipline trying to flow control the input. I've
> lost my box for the moment so that
> will be tomorrow perhaps.
> 
> ....
> 
> >> +
> >>         /* NOTE These routines can get interrupted by
> >>            ftdi_sio_read_bulk_callback  - need to examine what this
> >> means - don't see any problems yet */
> >>
> >> +    if ((old_termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)) ==
> >> +        (termios->c_cflag & (CSIZE|PARODD|CSTOPB|PARODD)))
> >
> > I think you need
> >
> > CSIZE (for CS7/8 switch)
> > PARODD (parity odd/even)
> > CMSPAR (parity mark/space v odd/even)
> > CSTOPB (stop bits)
> >
> > while you have PARODD twice.
> >
> > Otherwise this looks correct.
> 
> 
>   Woops - yep that was wrong. I attach a new patch - with CMSPAR and
> PARENB which I assume is also parity related.

Much better, thanks, but the tabs all seem to have been removed from
this patch, making it impossible to apply.

Also, I need a "clean" patch, against the 3.1 tree at the least, in
order to be able to apply this, not against a previous version of your
patch.

Also, run your patch through the scripts/checkpatch.pl script to ensure
it is correct before sending it out.

Care to fix this up and resend so that I can apply this?

thanks,

greg k-h