LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Newall <davidn@davidnewall.com>
To: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Greg KH <greg@kroah.com>,
	linux-usb@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: Handshaking on USB serial devices
Date: Fri, 22 Feb 2008 01:52:09 +1030	[thread overview]
Message-ID: <47BD9721.70907@davidnewall.com> (raw)
In-Reply-To: <20080214193637.5df1c205@core>

Alan,

Alan Cox wrote:
>> That's a very good point.  Even so: on the 2.4 driver, write_room isn't
>> implemented (refer to a previous message by Alan); and in 2.6, a 1k
>> buffer is built into the driver, with nothing to prevent it being sent
>> when the hardware buffer fills.
>>     
[...]

> Careful - a lot of hardware handles this properly itself, you simply
> don't get the URB completing until its all done.
>   

I am incredibly grateful to you for telling me this.  I didn't know it
and hadn't noticed that that was occurring; it was.  I thought data was
being lost by the converter -- I could see where it was missing on the
screen! -- but it's actually a bug in the tty layer.

Here's what really was happening: The PL2303 contains a 256 byte buffer,
and write URBs don't complete when that's full, as you said.   At such
times, that is when a bulk write was pending, the tty layer could and
would still call pl2303_write, which would return 0.  The tty layer
would loop, trying one byte followed by the remaining count, repeating
until no data remained.  The following clause, from write_chan in
n_tty.c, appears responsible; it is within a while(1) loop:

                if (O_OPOST(tty) && !(test_bit(TTY_HW_COOK_OUT, &tty->flags))) {
                        while (nr > 0) {
                                ssize_t num = opost_block(tty, b, nr);
                                if (num < 0) {
                                        if (num == -EAGAIN)
                                                break;
                                        retval = num;
                                        goto break_out;
                                }
                                b += num;
                                nr -= num;
                                if (nr == 0)
                                        break;
                                get_user(c, b);
                                if (opost(c, tty) < 0)
                                        break;
                                b++; nr--;
                        }
                        if (tty->driver.flush_chars)
                                tty->driver.flush_chars(tty);
                } ...


Note that opost() assumes that driver.putchar (which translates to a
one-byte write) always succeeds.  This behaviour is documented.

By the way, the true cause of the problem was somewhat disguised by the
fact that space in the PL2303's buffer could become available part-way
through the cycle, causing non-deterministic (i.e. not exactly
reproducible) loss of part of a write; it looked exactly like a flow
control problem.

The (2.4, remember) PL2303 driver has no write_room function, which I
gather is the appropriate place to signal that a write cannot be
performed.  Providing one that returns 0 when the (singleton) write urb
is busy, resolves the issue.  The patch that I had previously been
developing is entirely wrong.  Oh well.  Mind you, providing a
write_room function is NOT a real solution; it merely reduces the
condition to a usually-winnable race.  (Ironically, when I back-ported
the 2.6 driver, I excluded its new write_room function.  Mistake.)

Thanks for the nugget.

  reply	other threads:[~2008-02-21 15:21 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-13 14:45 David Newall
2008-02-14  5:02 ` Greg KH
2008-02-14  9:25   ` David Newall
2008-02-14 12:10     ` Alan Cox
2008-02-14 16:16       ` Gene Heskett
2008-02-14 16:31         ` Alan Cox
2008-02-14 17:55           ` Gene Heskett
2008-02-14 19:37             ` Alan Cox
2008-02-14 20:04               ` Gene Heskett
2008-02-14 20:52                 ` Greg KH
2008-02-14 21:32                   ` Gene Heskett
2008-02-15  5:08               ` David Newall
2008-02-14 22:39         ` Krzysztof Halasa
2008-02-14 23:09           ` Gene Heskett
2008-02-14 18:04       ` David Newall
2008-02-14 18:53         ` David Brownell
2008-02-14 19:36         ` Alan Cox
2008-02-21 15:22           ` David Newall [this message]
2008-02-21 15:15             ` Alan Cox
2008-02-21 19:35               ` David Newall
2008-02-21 20:58                 ` Greg KH
2008-02-14 20:47     ` Greg KH
2008-02-15  5:19       ` David Newall
2008-02-14 11:55   ` Alan Cox
     [not found] <9Wr5Z-7cw-1@gated-at.bofh.it>
     [not found] ` <9WSJ4-222-21@gated-at.bofh.it>
     [not found]   ` <9WTlN-2T0-7@gated-at.bofh.it>
     [not found]     ` <9WTYn-3Zb-29@gated-at.bofh.it>
2008-02-15 12:00       ` Bodo Eggert

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=47BD9721.70907@davidnewall.com \
    --to=davidn@davidnewall.com \
    --cc=alan@lxorguk.ukuu.org.uk \
    --cc=greg@kroah.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --subject='Re: Handshaking on USB serial devices' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).