public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Alan Cox <alan@linux.intel.com>
To: Andrew Worsley <amworsley@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>,
	Uwe Bonnes <bon@elektron.ikp.physik.tu-darmstadt.de>,
	Johan Hovold <jhovold@gmail.com>,
	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c
Date: Wed, 2 Nov 2011 11:31:04 +0000	[thread overview]
Message-ID: <20111102113104.3a641f7a@bob.linux.org.uk> (raw)
In-Reply-To: <CA+Y=x3k+jxN7LEk64mHkFQkkQhWa_KX7Uh+Uwuk0u87c+FiG_Q@mail.gmail.com>

> >> +       /* 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


  reply	other threads:[~2011-11-02 11:20 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-10-28  6:57 [PATCH] Fix Corruption issue in USB ftdi driver drivers/usb/serial/ftdi_sio.c Andrew Worsley
2011-10-28 10:50 ` Uwe Bonnes
2011-10-28 20:25   ` Andrew Worsley
2011-11-01 11:36 ` Alan Cox
2011-11-02  5:21   ` Andrew Worsley
2011-11-02 11:31     ` Alan Cox [this message]
2011-11-03  5:06       ` Andrew Worsley
2011-11-14 21:54         ` Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20111102113104.3a641f7a@bob.linux.org.uk \
    --to=alan@linux.intel.com \
    --cc=amworsley@gmail.com \
    --cc=bon@elektron.ikp.physik.tu-darmstadt.de \
    --cc=gregkh@suse.de \
    --cc=jhovold@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=plagnioj@jcrosoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox