LinuxLists.cc - [PATCH] USB: ftdi_sio: Remove tty->low

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Am Donnerstag, 24. September 2009 17:40:23 schrieb Johan Hovold:
> Is there a reason why this was not fixed in ftdi_sio (and whiteheat?)
> along with the other drivers?

No good reason. They escaped my grep pattern. Mea culpa.

Regards
Oliver

2009-09-24 19:20:29

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Thu, 24 Sep 2009 21:03:47 +0200
Oliver Neukum <[email protected]> wrote:

> Am Donnerstag, 24. September 2009 17:40:23 schrieb Johan Hovold:
> > Is there a reason why this was not fixed in ftdi_sio (and whiteheat?)
> > along with the other drivers?
>
> No good reason. They escaped my grep pattern. Mea culpa.

ftdi_sio is correct with low_latency set as it uses a work queue to
process the packets received.

Alan

2009-09-24 21:15:00

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Thu, Sep 24, 2009 at 08:21:07PM +0100, Alan Cox wrote:
> On Thu, 24 Sep 2009 21:03:47 +0200
> Oliver Neukum <[email protected]> wrote:

> > Am Donnerstag, 24. September 2009 17:40:23 schrieb Johan Hovold:
> > > Is there a reason why this was not fixed in ftdi_sio (and whiteheat?)
> > > along with the other drivers?

> > No good reason. They escaped my grep pattern. Mea culpa.

> ftdi_sio is correct with low_latency set as it uses a work queue to
> process the packets received.

AFAICT it only uses the work queue if tty_buffer_request_room fails to
allocate enough space. This being the exception, the completion
handler normally processes the packets in interrupt context and this is
where I get my lockdep traces (and it happens every time I hit the echo
or throttle paths).

/Johan

2009-09-25 17:48:00

by Michael Trimarchi

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Johan Hovold wrote:
> On Thu, Sep 24, 2009 at 08:21:07PM +0100, Alan Cox wrote:
>
>> On Thu, 24 Sep 2009 21:03:47 +0200
>> Oliver Neukum <[email protected]> wrote:
>>
>
>
>>> Am Donnerstag, 24. September 2009 17:40:23 schrieb Johan Hovold:
>>>
>>>> Is there a reason why this was not fixed in ftdi_sio (and whiteheat?)
>>>> along with the other drivers?
>>>>
>
>
>>> No good reason. They escaped my grep pattern. Mea culpa.
>>>
>
>
>> ftdi_sio is correct with low_latency set as it uses a work queue to
>> process the packets received.
>>
>
> AFAICT it only uses the work queue if tty_buffer_request_room fails to
> allocate enough space. This being the exception, the completion
> handler normally processes the packets in interrupt context and this is
> where I get my lockdep traces (and it happens every time I hit the echo
> or throttle paths).
>
Hi,

Using two urb, double buffering and schedule a tasklet to complete the
the reading phase. The usb will use the other free urb during receiving
process.
I think remove tty_latency is not a good fix.

Michael
> /Johan
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>

2009-09-29 14:55:17

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Fri, Sep 25, 2009 at 07:46:51PM +0200, Michael Trimarchi wrote:
> Johan Hovold wrote:
> > On Thu, Sep 24, 2009 at 08:21:07PM +0100, Alan Cox wrote:
> >> ftdi_sio is correct with low_latency set as it uses a work queue to
> >> process the packets received.
> >>
> > AFAICT it only uses the work queue if tty_buffer_request_room fails to
> > allocate enough space. This being the exception, the completion
> > handler normally processes the packets in interrupt context and this is
> > where I get my lockdep traces (and it happens every time I hit the echo
> > or throttle paths).
> >
> Using two urb, double buffering and schedule a tasklet to complete the
> the reading phase. The usb will use the other free urb during receiving
> process.
> I think remove tty_latency is not a good fix.

What do you say, Alan? Should ftdi_sio be rewritten so that it actually
defers all processing, or should low_latency go?

As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
interrupt context with low_latency set and that is obviously incorrect,
right?

Regards,
Johan

2009-09-29 22:55:19

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

> As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
> interrupt context with low_latency set and that is obviously incorrect,
> right?

It seems to do it from a work queue - or did I miss a case ?

2009-09-30 06:34:54

by Michael Trimarchi

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Alan Cox wrote:
>> As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
>> interrupt context with low_latency set and that is obviously incorrect,
>> right?
>>
>
> It seems to do it from a work queue - or did I miss a case ?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
usb_fill_bulk_urb(port->read_urb, dev,
usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
port->read_urb->transfer_buffer,
port->read_urb->transfer_buffer_length,
ftdi_read_bulk_callback, port);

(can be call in the interrupt context)
ftdi_read_bulk_callback--->
ftdi_process_read-->
tty_flip_buffer_push(tty);

Michael

2009-09-30 09:05:43

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Tue, Sep 29, 2009 at 11:52:32PM +0100, Alan Cox wrote:
> > As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
> > interrupt context with low_latency set and that is obviously incorrect,
> > right?
>
> It seems to do it from a work queue - or did I miss a case ?

The function used for deferred work is actually called directly from
ftdi_read_bulk_callback:

ftdi_process_read(&priv->rx_work.work);

It only gets scheduled on the work queue when unthrottled (or if
tty_buffer_request_room(tty, length) < length before serial_throttle is
called).

So basically, unless throttled, it is always called from interrupt
context.

/Johan

2009-10-02 02:52:32

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Alan Cox <[email protected]> writes:

>> As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
>> interrupt context with low_latency set and that is obviously incorrect,
>> right?
>
> It seems to do it from a work queue - or did I miss a case ?

ftdi_sio crash quite regularly for me with 2.6.31.

With a bunch of nasties like:
BUG: scheduling while atomic: swapper/0/0x00010000
bad: scheduling from the idle thread!

I don't know if I have a good backtrace as things
scrolled away faster than they were captured
but the code below looks like it may be.

Eric

BUG: scheduling while atomic: swapper/0/0x00010000
Modules linked in: nfsd lockd nfs_acl auth_rpcgss exportfs sco bridge stp bnep l2cap bluetooth sunrpc ipv6 cpufreq_ondemand powernow_k8 freq_table dm_mirror dm_region_hash dm_log dm_multipath dm_mod uinput kvm_amd kvm fuse xt_multiport iptable_nat ip_tables nf_nat x_tables nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 tun 8021q snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_timer snd amd64_edac_mod firewire_ohci firewire_core soundcore i2c_nforce2 k8temp sg edac_core hwmon pcspkr sata_sil24 pata_amd snd_page_alloc e1000e forcedeth crc_itu_t i2c_core ftdi_sio usbserial ata_generic pata_acpi sata_nv libata sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd [last unloaded: scsi_wait_scan]
CPU 0:
Modules linked in: nfsd lockd nfs_acl auth_rpcgss exportfs sco bridge stp bnep l2cap bluetooth sunrpc ipv6 cpufreq_ondemand powernow_k8 freq_table dm_mirror dm_region_hash dm_log dm_multipath dm_mod uinput kvm_amd kvm fuse xt_multiport iptable_nat ip_tables nf_nat x_tables nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 tun 8021q snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_timer snd amd64_edac_mod firewire_ohci firewire_core soundcore i2c_nforce2 k8temp sg edac_core hwmon pcspkr sata_sil24 pata_amd snd_page_alloc e1000e forcedeth crc_itu_t i2c_core ftdi_sio usbserial ata_generic pata_acpi sata_nv libata sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd [last unloaded: scsi_wait_scan]
Pid: 0, comm: swapper Not tainted 2.6.31-185494.2008.AroraEbiederm.fc11.x86_64 #1
RIP: 0010:[<ffffffff8102c86c>] [<ffffffff8102c86c>] native_safe_halt+0x6/0x8
RSP: 0018:ffffffff81541e48 EFLAGS: 00000246
RAX: 0000000000000000 RBX: ffffffff81541e48 RCX: 0000000003000000
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffff81541e58
RBP: ffffffff8100c7ce R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: ffff880038d0fef8 R12: ffffffff81073a1f
R13: ffffffff81541dd8 R14: ffffffff8105c8d7 R15: ffffffff81541e38
FS: 00007fe289d71910(0000) GS:ffff8800017ba000(0000) knlGS:0000000000000000
CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000404070 CR3: 000000003b87d000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Call Trace:
[<ffffffff81014126>] ? default_idle+0x51/0x8b
[<ffffffff81014265>] ? c1e_idle+0x105/0x120
[<ffffffff8100ae15>] ? cpu_idle+0xb0/0xf3
[<ffffffff81392135>] ? rest_init+0x79/0x8f
[<ffffffff815c8fae>] ? start_kernel+0x3dc/0x3fd
[<ffffffff815c82d4>] ? x86_64_start_reservations+0xbb/0xd6
[<ffffffff815c83f4>] ? x86_64_start_kernel+0x105/0x128
bad: scheduling from the idle thread!
Pid: 0, comm: swapper Not tainted 2.6.31-185494.2008.AroraEbiederm.fc11.x86_64 #1
Call Trace:
<IRQ> [<ffffffff81041261>] dequeue_task_idle+0x37/0x5a
[<ffffffff81040a23>] dequeue_task+0xce/0xf0
[<ffffffff81040a7c>] deactivate_task+0x37/0x56
[<ffffffff813a4fb7>] schedule+0x13d/0x6f3
[<ffffffff81042b82>] ? enqueue_task_fair+0xdf/0x13c
[<ffffffff810404df>] ? enqueue_task+0x6f/0x91
[<ffffffff813a5e87>] __mutex_lock_common+0x12f/0x1aa
[<ffffffff813a5f29>] __mutex_lock_slowpath+0x27/0x3d
[<ffffffff813a5c3d>] mutex_lock+0x25/0x53
[<ffffffff81253ff9>] tty_unthrottle+0x29/0x6d
[<ffffffff8125298a>] reset_buffer_flags+0xe8/0x105
[<ffffffff812529cb>] n_tty_flush_buffer+0x24/0x97
[<ffffffff8125367b>] n_tty_receive_buf+0xc3d/0xe72
[<ffffffff8129b0fd>] ? usb_hcd_submit_urb+0x888/0x943
[<ffffffff81254e83>] ? tty_ldisc_try+0x53/0x71
[<ffffffff81255f85>] flush_to_ldisc+0x116/0x1bd
[<ffffffff8125608a>] tty_flip_buffer_push+0x5e/0x85
[<ffffffffa010a0ab>] ftdi_process_read+0x481/0x627 [ftdi_sio]
[<ffffffffa0001aea>] ? timer_action+0x63/0x79 [ehci_hcd]
[<ffffffffa010a480>] ftdi_read_bulk_callback+0x22f/0x25a [ftdi_sio]
[<ffffffff81040e89>] ? complete+0x54/0x73
[<ffffffffa000597f>] ? ehci_irq+0x351/0x391 [ehci_hcd]
[<ffffffff81299a32>] usb_hcd_giveback_urb+0x9b/0xe5
[<ffffffffa00010f9>] ehci_urb_done+0x91/0xbc [ehci_hcd]
[<ffffffffa00027f3>] qh_completions+0x42a/0x4ca [ehci_hcd]
[<ffffffffa0002938>] ehci_work+0xa5/0x7ab [ehci_hcd]
[<ffffffffa00db90f>] ? nv_swncq_interrupt+0x6a3/0x6d1 [sata_nv]
[<ffffffffa000597f>] ehci_irq+0x351/0x391 [ehci_hcd]
[<ffffffff81073641>] ? clocksource_read+0x1d/0x33
[<ffffffff81073a1f>] ? getnstimeofday+0x69/0xd3
[<ffffffff8129933c>] usb_hcd_irq+0x4d/0xa1
[<ffffffff810a2553>] handle_IRQ_event+0x6a/0x13f
[<ffffffff810a4613>] handle_fasteoi_irq+0x90/0xe1
[<ffffffff8100eb5a>] handle_irq+0x95/0xb7
[<ffffffff8100df49>] do_IRQ+0x6a/0xe0
[<ffffffff8100c7d3>] ret_from_intr+0x0/0x11
<EOI> [<ffffffff8102c86c>] ? native_safe_halt+0x6/0x8
[<ffffffff81014126>] ? default_idle+0x51/0x8b
[<ffffffff81014265>] ? c1e_idle+0x105/0x120
[<ffffffff8100ae15>] ? cpu_idle+0xb0/0xf3
[<ffffffff81392135>] ? rest_init+0x79/0x8f
[<ffffffff815c8fae>] ? start_kernel+0x3dc/0x3fd
[<ffffffff815c82d4>] ? x86_64_start_reservations+0xbb/0xd6
[<ffffffff815c83f4>] ? x86_64_start_kernel+0x105/0x128
BUG: scheduling while atomic: swapper/0/0x00010000

2009-10-02 08:47:59

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Thu, Oct 01, 2009 at 07:52:21PM -0700, Eric W. Biederman wrote:
> Alan Cox <[email protected]> writes:
>
> >> As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
> >> interrupt context with low_latency set and that is obviously incorrect,
> >> right?
> >
> > It seems to do it from a work queue - or did I miss a case ?
>
> ftdi_sio crash quite regularly for me with 2.6.31.
>
> With a bunch of nasties like:
> BUG: scheduling while atomic: swapper/0/0x00010000
> bad: scheduling from the idle thread!

It's the same problem.

Greg, can't we apply the patch for stable at least? Then we can massage
ftdi_sio into actually using the work queue for doing _all_ processing
in the meantime if deemed necessary.

Alan, did you have time to look at it? Are there any reasons for wanting
to keep low_latency in ftdi_sio when it was removed from all other
drivers processing in interrupt context (without doing work queue
re-implementations)?

Thanks,
Johan

2009-10-02 09:03:50

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Thu, 01 Oct 2009 19:52:21 -0700
[email protected] (Eric W. Biederman) wrote:

> Alan Cox <[email protected]> writes:
>
> >> As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
> >> interrupt context with low_latency set and that is obviously incorrect,
> >> right?
> >
> > It seems to do it from a work queue - or did I miss a case ?
>
> ftdi_sio crash quite regularly for me with 2.6.31.
>
> With a bunch of nasties like:
> BUG: scheduling while atomic: swapper/0/0x00010000
> bad: scheduling from the idle thread!
>
> I don't know if I have a good backtrace as things
> scrolled away faster than they were captured
> but the code below looks like it may be.

So it is indeed wrong. Removing the tty->low_latency = 1 will fix the
crash and probably should go for stable. I'll have a deeper look at what
is doing with the rest of the stuff as the driver may just be trying to
be far cleverer than it needs with the newer buffer code.

Alan

2009-10-02 09:51:47

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Having had a look through the driver someone with docs or similar needs
to explain how the ftdi_sio does hardware and software flow control. It
has throttle/unthrottle methods but these just stall the URB queuing and
don't do any flow processing of their own at all. Does the ftdi_sio do
this in hardware when the urbs run low ?

Otherwise it can certainly be simplified a lot. In particular at the
point you get throttle event you've got about 64K of cushion to react
(flow control being async anyway) and you actually *want* to pull data
after you whack the modem lines because the other end also has a latency
to respond.

So all the clever partial processing of urb stuff is overkill. Whether it
needs not to repost urbs to the device or to implement flow control in
software I don't know without docs.

With that done the driver ought to be a good deal easier to debug.

Alan

2009-10-02 16:32:30

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

> Alan, did you have time to look at it? Are there any reasons for wanting
> to keep low_latency in ftdi_sio when it was removed from all other
> drivers processing in interrupt context (without doing work queue
> re-implementations)?

Yes for latency handling (two layers of work queue is bad) but its the
right fix for stable.

For upstream how does this look as a tidy up
ftdi_sio: simplify driver

From: Alan Cox <[email protected]>

This does a lot of stuff that the modern buffering logic will cover itself
so remove the cruft.

- Remove the code handling throttle half way through a packet. We have 64K
of slack and flow control is async anyway so stopping is the wrong thing
to do
- Remove various commented out bits
- Without the partial packet stuff we can remove the async queue stuff and
split it into sensible functions for URB processing and for queueing urbs
for receipt
- Remove the unused rx_bytes count. We take locks for it, we jump through
hoops for it and we never expose it.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/usb/serial/ftdi_sio.c | 199 +++++++++++------------------------------
1 files changed, 51 insertions(+), 148 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 4f883b1..f796b07 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -78,10 +78,7 @@ struct ftdi_private {
char prev_status, diff_status; /* Used for TIOCMIWAIT */
__u8 rx_flags; /* receive state flags (throttling) */
spinlock_t rx_lock; /* spinlock for receive state */
- struct delayed_work rx_work;
struct usb_serial_port *port;
- int rx_processed;
- unsigned long rx_bytes;

__u16 interface; /* FT2232C, FT2232H or FT4232H port interface
(0 for FT232/245) */
@@ -763,7 +760,8 @@ static int ftdi_write_room(struct tty_struct *tty);
static int ftdi_chars_in_buffer(struct tty_struct *tty);
static void ftdi_write_bulk_callback(struct urb *urb);
static void ftdi_read_bulk_callback(struct urb *urb);
-static void ftdi_process_read(struct work_struct *work);
+static void ftdi_process_read(struct ftdi_private *priv,
+ struct tty_struct *tty);
static void ftdi_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old);
static int ftdi_tiocmget(struct tty_struct *tty, struct file *file);
@@ -1549,7 +1547,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
port->read_urb->transfer_buffer_length = BUFSZ;
}

- INIT_DELAYED_WORK(&priv->rx_work, ftdi_process_read);
priv->port = port;

/* Free port's existing write urb and transfer buffer. */
@@ -1700,9 +1697,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_lock_irqsave(&priv->tx_lock, flags);
priv->tx_bytes = 0;
spin_unlock_irqrestore(&priv->tx_lock, flags);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes = 0;
- spin_unlock_irqrestore(&priv->rx_lock, flags);

if (tty)
tty->low_latency = (priv->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
@@ -1730,7 +1724,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_unlock_irqrestore(&priv->rx_lock, flags);

/* Start reading from the device */
- priv->rx_processed = 0;
usb_fill_bulk_urb(port->read_urb, dev,
usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
port->read_urb->transfer_buffer,
@@ -1787,10 +1780,6 @@ static void ftdi_close(struct usb_serial_port *port)

dbg("%s", __func__);

-
- /* cancel any scheduled reading */
- cancel_delayed_work_sync(&priv->rx_work);
-
/* shutdown our bulk read */
usb_kill_urb(port->read_urb);
kref_put(&priv->kref, ftdi_sio_priv_release);
@@ -2019,7 +2008,6 @@ static void ftdi_read_bulk_callback(struct urb *urb)
struct tty_struct *tty;
struct ftdi_private *priv;
unsigned long countread;
- unsigned long flags;
int status = urb->status;

if (urb->number_of_packets > 0) {
@@ -2036,94 +2024,88 @@ static void ftdi_read_bulk_callback(struct urb *urb)
if (port->port.count <= 0)
return;

- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
+ if (status) {
+ /* This will happen at close every time so it is a dbg not an
+ err */
+ dbg("(this is ok on close) nonzero read bulk status received: %d", status);
return;
}

priv = usb_get_serial_port_data(port);
if (!priv) {
dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
+ return;
}

- if (urb != port->read_urb)
- dev_err(&port->dev, "%s - Not my urb!\n", __func__);
+ tty = tty_port_tty_get(&port->port);
+ if (!tty) {
+ dbg("%s - bad tty pointer - exiting", __func__);
+ return;
+ }

- if (status) {
- /* This will happen at close every time so it is a dbg not an
- err */
- dbg("(this is ok on close) nonzero read bulk status received: %d", status);
- goto out;
+
+ if (urb != port->read_urb) {
+ dev_err(&port->dev, "%s - Not my urb!\n", __func__);
+ return;
}

/* count data bytes, but not status bytes */
countread = urb->actual_length;
countread -= 2 * DIV_ROUND_UP(countread, priv->max_packet_size);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes += countread;
- spin_unlock_irqrestore(&priv->rx_lock, flags);

- ftdi_process_read(&priv->rx_work.work);
-out:
+ ftdi_process_read(priv, tty);
tty_kref_put(tty);
} /* ftdi_read_bulk_callback */

-static void ftdi_process_read(struct work_struct *work)
-{ /* ftdi_process_read */
- struct ftdi_private *priv =
- container_of(work, struct ftdi_private, rx_work.work);
+static void ftdi_repost_urb(struct ftdi_private *priv)
+{
+ struct usb_serial_port *port = priv->port;
+ int result;
+
+ /* if the port is closed or throttled stop trying to read */
+ if (port->port.count <= 0 || (priv->rx_flags & THROTTLED))
+ return;
+ /* Continue trying to always read */
+ usb_fill_bulk_urb(port->read_urb, port->serial->dev,
+ usb_rcvbulkpipe(port->serial->dev,
+ port->bulk_in_endpointAddress),
+ port->read_urb->transfer_buffer,
+ port->read_urb->transfer_buffer_length,
+ ftdi_read_bulk_callback, port);
+
+ result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
+ if (result)
+ dev_err(&port->dev,
+ "%s - failed resubmitting read urb, error %d\n",
+ __func__, result);
+}
+
+static void ftdi_process_read(struct ftdi_private *priv, struct tty_struct *tty)
+{
struct usb_serial_port *port = priv->port;
struct urb *urb;
- struct tty_struct *tty;
char error_flag;
unsigned char *data;

int i;
- int result;
int need_flip;
int packet_offset;
- unsigned long flags;

dbg("%s - port %d", __func__, port->number);

if (port->port.count <= 0)
return;

- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
- return;
- }
-
- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
- }
-
urb = port->read_urb;
- if (!urb) {
- dbg("%s - bad read_urb pointer - exiting", __func__);
- goto out;
- }
-
data = urb->transfer_buffer;

- if (priv->rx_processed) {
- dbg("%s - already processed: %d bytes, %d remain", __func__,
- priv->rx_processed,
- urb->actual_length - priv->rx_processed);
- } else {
- /* The first two bytes of every read packet are status */
- if (urb->actual_length > 2)
- usb_serial_debug_data(debug, &port->dev, __func__,
- urb->actual_length, data);
- else
- dbg("Status only: %03oo %03oo", data[0], data[1]);
- }
+ /* The first two bytes of every read packet are status */
+ if (urb->actual_length > 2)
+ usb_serial_debug_data(debug, &port->dev, __func__,
+ urb->actual_length, data);
+ else
+ dbg("Status only: %03oo %03oo", data[0], data[1]);

/* TO DO -- check for hung up line and handle appropriately: */
@@ -2132,7 +2114,7 @@ static void ftdi_process_read(struct work_struct *work)
/* if CD is dropped and the line is not CLOCAL then we should hangup */

need_flip = 0;
- for (packet_offset = priv->rx_processed;
+ for (packet_offset = 0;
packet_offset < urb->actual_length; packet_offset += priv->max_packet_size) {
int length;

@@ -2155,17 +2137,6 @@ static void ftdi_process_read(struct work_struct *work)
length = 0;
}

- if (priv->rx_flags & THROTTLED) {
- dbg("%s - throttled", __func__);
- break;
- }
- if (tty_buffer_request_room(tty, length) < length) {
- /* break out & wait for throttling/unthrottling to
- happen */
- dbg("%s - receive room low", __func__);
- break;
- }
-
/* Handle errors and break */
error_flag = TTY_NORMAL;
/* Although the device uses a bitmask and hence can have
@@ -2203,79 +2174,11 @@ static void ftdi_process_read(struct work_struct *work)
need_flip = 1;
}

-#ifdef NOT_CORRECT_BUT_KEEPING_IT_FOR_NOW
- /* if a parity error is detected you get status packets forever
- until a character is sent without a parity error.
- This doesn't work well since the application receives a
- never ending stream of bad data - even though new data
- hasn't been sent. Therefore I (bill) have taken this out.
- However - this might make sense for framing errors and so on
- so I am leaving the code in for now.
- */
- else {
- if (error_flag != TTY_NORMAL) {
- dbg("error_flag is not normal");
- /* In this case it is just status - if that is
- an error send a bad character */
- if (tty->flip.count >= TTY_FLIPBUF_SIZE)
- tty_flip_buffer_push(tty);
- tty_insert_flip_char(tty, 0xff, error_flag);
- need_flip = 1;
- }
- }
-#endif
} /* "for(packet_offset=0..." */

- /* Low latency */
if (need_flip)
tty_flip_buffer_push(tty);
-
- if (packet_offset < urb->actual_length) {
- /* not completely processed - record progress */
- priv->rx_processed = packet_offset;
- dbg("%s - incomplete, %d bytes processed, %d remain",
- __func__, packet_offset,
- urb->actual_length - packet_offset);
- /* check if we were throttled while processing */
- spin_lock_irqsave(&priv->rx_lock, flags);
- if (priv->rx_flags & THROTTLED) {
- priv->rx_flags |= ACTUALLY_THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- dbg("%s - deferring remainder until unthrottled",
- __func__);
- goto out;
- }
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- /* if the port is closed stop trying to read */
- if (port->port.count > 0)
- /* delay processing of remainder */
- schedule_delayed_work(&priv->rx_work, 1);
- else
- dbg("%s - port is closed", __func__);
- goto out;
- }
-
- /* urb is completely processed */
- priv->rx_processed = 0;
-
- /* if the port is closed stop trying to read */
- if (port->port.count > 0) {
- /* Continue trying to always read */
- usb_fill_bulk_urb(port->read_urb, port->serial->dev,
- usb_rcvbulkpipe(port->serial->dev,
- port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
-
- result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
- if (result)
- dev_err(&port->dev,
- "%s - failed resubmitting read urb, error %d\n",
- __func__, result);
- }
-out:
- tty_kref_put(tty);
+ ftdi_repost_urb(priv);
} /* ftdi_process_read */

@@ -2635,7 +2538,7 @@ static void ftdi_unthrottle(struct tty_struct *tty)
spin_unlock_irqrestore(&priv->rx_lock, flags);

if (actually_throttled)
- schedule_delayed_work(&priv->rx_work, 0);
+ ftdi_repost_urb(priv);
}

static int __init ftdi_init(void)

2009-10-02 17:02:04

by Greg KH

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Fri, Oct 02, 2009 at 10:47:55AM +0200, Johan Hovold wrote:
> On Thu, Oct 01, 2009 at 07:52:21PM -0700, Eric W. Biederman wrote:
> > Alan Cox <[email protected]> writes:
> >
> > >> As it stands today ftdi_sio does indeed call tty_flip_buffer_push from
> > >> interrupt context with low_latency set and that is obviously incorrect,
> > >> right?
> > >
> > > It seems to do it from a work queue - or did I miss a case ?
> >
> > ftdi_sio crash quite regularly for me with 2.6.31.
> >
> > With a bunch of nasties like:
> > BUG: scheduling while atomic: swapper/0/0x00010000
> > bad: scheduling from the idle thread!
>
> It's the same problem.
>
> Greg, can't we apply the patch for stable at least? Then we can massage
> ftdi_sio into actually using the work queue for doing _all_ processing
> in the meantime if deemed necessary.

Patches need to be in Linus's tree first, before they can get into the
-stable releases.

I'm still digging through my patch queue, sorry, been swamped with the
-stable stuff, due to the merge window and 4 conferences over the past 2
weeks...

thanks,

greg k-h

2009-10-02 22:30:10

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Alan Cox <[email protected]> writes:

>> Alan, did you have time to look at it? Are there any reasons for wanting
>> to keep low_latency in ftdi_sio when it was removed from all other
>> drivers processing in interrupt context (without doing work queue
>> re-implementations)?
>
> Yes for latency handling (two layers of work queue is bad) but its the
> right fix for stable.
>
> For upstream how does this look as a tidy up
> ftdi_sio: simplify driver
>
> From: Alan Cox <[email protected]>
>
> This does a lot of stuff that the modern buffering logic will cover itself
> so remove the cruft.
>
> - Remove the code handling throttle half way through a packet. We have 64K
> of slack and flow control is async anyway so stopping is the wrong thing
> to do
> - Remove various commented out bits
> - Without the partial packet stuff we can remove the async queue stuff and
> split it into sensible functions for URB processing and for queueing urbs
> for receipt
> - Remove the unused rx_bytes count. We take locks for it, we jump through
> hoops for it and we never expose it.
>
> Signed-off-by: Alan Cox <[email protected]>

The code doesn't fall over immediately in my testing. So at first glance
this appears to be as good as removing low_latency.

Eric

2009-10-02 23:00:46

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

I have seen a hang in:
/bin/stty (changing the baud rate)
set_termios
tty_wait_until_sent
tty_chars_in_buffer
ftdi_chars_in_buffer

Where the driver wedged for a serial port and no progress
was made.

This happened to me several times with 2.6.31. My initial
hypothesis was this was a hardware error (as it only happened
on single piece of hardware). With all of the driver problems
I suspect it could be a driver bug.

Any ideas from the people who understand this code?

Eric

2009-10-03 10:21:28

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Fri, Oct 02, 2009 at 03:29:56PM -0700, Eric W. Biederman wrote:
> The code doesn't fall over immediately in my testing. So at first glance
> this appears to be as good as removing low_latency.

The patch wasn't intended as a replacement for removing low_latency. It
still calls tty_flip_buffer_push from interrupt context.

/Johan

2009-10-03 11:42:33

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Fri, Oct 02, 2009 at 05:33:08PM +0100, Alan Cox wrote:
> For upstream how does this look as a tidy up

I found a couple of issues:

- The patch breaks unthrottling, as it checks THROTTLE before resubmitting
urb whereas unthrottle checks ACTUALLY_UNTROTTLED which is never set.

- The countread stuff should also be removed from
ftdi_read_bulk_callback as they where only used for updating
rx_bytes,

And, obviously, this doesn't solve the problem of tty_flip_buffer_push
being called from interrupt context (but I assume that was never the
intention).

I've actually been working on cleaning up ftdi_sio inspired by the
generic driver. I also threw out the work queue, but had not noticed the
unused rx_byte yet. I also used the generic drivers scheme of not
pushing to tty until unthrottled, but as you point out this is not really
needed anymore.

My patch is quite large, and could be split up once finished.

With the removal of low_latency patch it solves the remaining issues at
hand with ftdi_sio:

- It does not stall when a read callback is made during serial_open.
- It handles the late call to unthrottle during serial_open.

What do you think?

Thanks,
Johan

From: Johan Hovold <[email protected]>
Date: Sat, 3 Oct 2009 12:30:13 +0200
Subject: [PATCH] USB: ftdi_sio: Rewrite.

- Remove work queue.
- Use urb status to determine when port is closed rather than port
count. (Fixes stalled reads due to open race on 2.6.31).
- Re-structure read processing.
- Use tty_insert_flip_string instead of per character push unless
console.
- Always process before throttling.
- Handle late call to unthrottle during serial_close by checking
ASYNCB_CLOSING.
- Remove rx_flags and lock and use flags in usb_serial_port instead.
- Remove unused rx_bytes counter.
---
drivers/usb/serial/ftdi_sio.c | 425 ++++++++++++++---------------------------
1 files changed, 142 insertions(+), 283 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 0ac2c2f..a7c76df 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -76,12 +76,7 @@ struct ftdi_private {
unsigned long last_dtr_rts; /* saved modem control outputs */
wait_queue_head_t delta_msr_wait; /* Used for TIOCMIWAIT */
char prev_status, diff_status; /* Used for TIOCMIWAIT */
- __u8 rx_flags; /* receive state flags (throttling) */
- spinlock_t rx_lock; /* spinlock for receive state */
- struct delayed_work rx_work;
struct usb_serial_port *port;
- int rx_processed;
- unsigned long rx_bytes;

__u16 interface; /* FT2232C, FT2232H or FT4232H port interface
(0 for FT232/245) */
@@ -737,10 +732,6 @@ static const char *ftdi_chip_name[] = {
/* Constants for read urb and write urb */
#define BUFSZ 512

-/* rx_flags */
-#define THROTTLED 0x01
-#define ACTUALLY_THROTTLED 0x02
-
/* Used for TIOCMIWAIT */
#define FTDI_STATUS_B0_MASK (FTDI_RS0_CTS | FTDI_RS0_DSR | FTDI_RS0_RI | FTDI_RS0_RLSD)
#define FTDI_STATUS_B1_MASK (FTDI_RS_BI)
@@ -763,7 +754,7 @@ static int ftdi_write_room(struct tty_struct *tty);
static int ftdi_chars_in_buffer(struct tty_struct *tty);
static void ftdi_write_bulk_callback(struct urb *urb);
static void ftdi_read_bulk_callback(struct urb *urb);
-static void ftdi_process_read(struct work_struct *work);
+static void ftdi_process_read(struct usb_serial_port *port);
static void ftdi_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old);
static int ftdi_tiocmget(struct tty_struct *tty, struct file *file);
@@ -1526,7 +1517,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
}

kref_init(&priv->kref);
- spin_lock_init(&priv->rx_lock);
spin_lock_init(&priv->tx_lock);
init_waitqueue_head(&priv->delta_msr_wait);
/* This will push the characters through immediately rather
@@ -1548,7 +1538,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
port->read_urb->transfer_buffer_length = BUFSZ;
}

- INIT_DELAYED_WORK(&priv->rx_work, ftdi_process_read);
priv->port = port;

/* Free port's existing write urb and transfer buffer. */
@@ -1685,6 +1674,26 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port)
return 0;
}

+int ftdi_submit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
+{
+ struct urb *urb = port->read_urb;
+ struct usb_serial *serial = port->serial;
+ int result;
+
+ usb_fill_bulk_urb(urb, serial->dev,
+ usb_rcvbulkpipe(serial->dev,
+ port->bulk_in_endpointAddress),
+ urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ ftdi_read_bulk_callback, port);
+ result = usb_submit_urb(urb, mem_flags);
+ if (result)
+ dev_err(&port->dev,
+ "%s - failed submitting read urb, error %d\n",
+ __func__, result);
+ return result;
+}
+
static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
{ /* ftdi_open */
struct usb_device *dev = port->serial->dev;
@@ -1699,9 +1708,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_lock_irqsave(&priv->tx_lock, flags);
priv->tx_bytes = 0;
spin_unlock_irqrestore(&priv->tx_lock, flags);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes = 0;
- spin_unlock_irqrestore(&priv->rx_lock, flags);

write_latency_timer(port);

@@ -1721,23 +1727,13 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
ftdi_set_termios(tty, port, tty->termios);

/* Not throttled */
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

/* Start reading from the device */
- priv->rx_processed = 0;
- usb_fill_bulk_urb(port->read_urb, dev,
- usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
- result = usb_submit_urb(port->read_urb, GFP_KERNEL);
- if (result)
- dev_err(&port->dev,
- "%s - failed submitting read urb, error %d\n",
- __func__, result);
- else
+ result = ftdi_submit_read_urb(port, GFP_KERNEL);
+ if (!result)
kref_get(&priv->kref);

return result;
@@ -1783,10 +1779,6 @@ static void ftdi_close(struct usb_serial_port *port)

dbg("%s", __func__);

-
- /* cancel any scheduled reading */
- cancel_delayed_work_sync(&priv->rx_work);
-
/* shutdown our bulk read */
usb_kill_urb(port->read_urb);
kref_put(&priv->kref, ftdi_sio_priv_release);
@@ -2009,271 +2001,141 @@ static int ftdi_chars_in_buffer(struct tty_struct *tty)
return buffered;
}

-static void ftdi_read_bulk_callback(struct urb *urb)
+static void ftdi_process_packet(struct tty_struct *tty,
+ struct usb_serial_port *port, struct ftdi_private *priv,
+ char *packet, int len)
{
- struct usb_serial_port *port = urb->context;
- struct tty_struct *tty;
- struct ftdi_private *priv;
- unsigned long countread;
- unsigned long flags;
- int status = urb->status;
-
- if (urb->number_of_packets > 0) {
- dev_err(&port->dev, "%s transfer_buffer_length %d "
- "actual_length %d number of packets %d\n", __func__,
- urb->transfer_buffer_length,
- urb->actual_length, urb->number_of_packets);
- dev_err(&port->dev, "%s transfer_flags %x\n", __func__,
- urb->transfer_flags);
- }
+ int i;
+ char status;
+ char flag;
+ char *ch;

dbg("%s - port %d", __func__, port->number);

- if (port->port.count <= 0)
- return;
-
- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
- return;
+ /* Compare new line status to the old one, signal if different/
+ N.B. packet may be processed more than once, but differences
+ are only processed once. */
+ status = packet[0] & FTDI_STATUS_B0_MASK;
+ if (status != priv->prev_status) {
+ priv->diff_status |= status ^ priv->prev_status;
+ wake_up_interruptible(&priv->delta_msr_wait);
+ priv->prev_status = status;
}

- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
+ /*
+ * Although the device uses a bitmask and hence can have multiple
+ * errors on a packet - the order here sets the priority the error is
+ * returned to the tty layer.
+ */
+ flag = TTY_NORMAL;
+ if (packet[1] & FTDI_RS_OE) {
+ flag = TTY_OVERRUN;
+ dbg("OVERRRUN error");
}
-
- if (urb != port->read_urb)
- dev_err(&port->dev, "%s - Not my urb!\n", __func__);
-
- if (status) {
- /* This will happen at close every time so it is a dbg not an
- err */
- dbg("(this is ok on close) nonzero read bulk status received: %d", status);
- goto out;
+ if (packet[1] & FTDI_RS_BI) {
+ flag = TTY_BREAK;
+ dbg("BREAK received");
+ usb_serial_handle_break(port);
+ }
+ if (packet[1] & FTDI_RS_PE) {
+ flag = TTY_PARITY;
+ dbg("PARITY error");
+ }
+ if (packet[1] & FTDI_RS_FE) {
+ flag = TTY_FRAME;
+ dbg("FRAMING error");
}

- /* count data bytes, but not status bytes */
- countread = urb->actual_length;
- countread -= 2 * DIV_ROUND_UP(countread, priv->max_packet_size);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes += countread;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
-
- ftdi_process_read(&priv->rx_work.work);
-out:
- tty_kref_put(tty);
-} /* ftdi_read_bulk_callback */
+ /*
+ * The per character mucking around with sysrq path it too slow, so
+ * shortcircuit it in the 99.9999999% of cases where the USB serial is
+ * not a console anyway.
+ */
+ ch = packet + 2;
+ len -= 2;
+ if (!port->console || !port->sysrq)
+ tty_insert_flip_string(tty, ch, len);
+ else {
+ /* Push data to tty */
+ for (i = 0; i < len; i++, ch++) {
+ if (!usb_serial_handle_sysrq_char(tty, port, *ch))
+ tty_insert_flip_char(tty, *ch, flag);
+ }
+ }
+}

-static void ftdi_process_read(struct work_struct *work)
-{ /* ftdi_process_read */
- struct ftdi_private *priv =
- container_of(work, struct ftdi_private, rx_work.work);
- struct usb_serial_port *port = priv->port;
- struct urb *urb;
+static void ftdi_process_read(struct usb_serial_port *port)
+{
+ struct urb *urb = port->read_urb;
struct tty_struct *tty;
- char error_flag;
- unsigned char *data;
-
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ char *data = (char *)urb->transfer_buffer;
int i;
- int result;
+ int len;
int need_flip;
- int packet_offset;
- unsigned long flags;

- dbg("%s - port %d", __func__, port->number);
-
- if (port->port.count <= 0)
- return;
+ if (urb->actual_length <= 2)
+ return; /* status only */

- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
+ tty = tty_port_tty_get(&port->port);
+ if (!tty)
return;
- }

- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
- }
-
- urb = port->read_urb;
- if (!urb) {
- dbg("%s - bad read_urb pointer - exiting", __func__);
- goto out;
- }
-
- data = urb->transfer_buffer;
-
- if (priv->rx_processed) {
- dbg("%s - already processed: %d bytes, %d remain", __func__,
- priv->rx_processed,
- urb->actual_length - priv->rx_processed);
- } else {
- /* The first two bytes of every read packet are status */
- if (urb->actual_length > 2)
- usb_serial_debug_data(debug, &port->dev, __func__,
- urb->actual_length, data);
- else
- dbg("Status only: %03oo %03oo", data[0], data[1]);
- }
-
-
- /* TO DO -- check for hung up line and handle appropriately: */
- /* send hangup */
- /* See acm.c - you do a tty_hangup - eg tty_hangup(tty) */
- /* if CD is dropped and the line is not CLOCAL then we should hangup */
-
- need_flip = 0;
- for (packet_offset = priv->rx_processed;
- packet_offset < urb->actual_length; packet_offset += priv->max_packet_size) {
- int length;
-
- /* Compare new line status to the old one, signal if different/
- N.B. packet may be processed more than once, but differences
- are only processed once. */
- char new_status = data[packet_offset + 0] &
- FTDI_STATUS_B0_MASK;
- if (new_status != priv->prev_status) {
- priv->diff_status |=
- new_status ^ priv->prev_status;
- wake_up_interruptible(&priv->delta_msr_wait);
- priv->prev_status = new_status;
- }
+ /* FIXME: check priv? */

- length = min_t(u32, priv->max_packet_size, urb->actual_length-packet_offset)-2;
- if (length < 0) {
- dev_err(&port->dev, "%s - bad packet length: %d\n",
- __func__, length+2);
- length = 0;
- }
-
- if (priv->rx_flags & THROTTLED) {
- dbg("%s - throttled", __func__);
- break;
- }
- if (tty_buffer_request_room(tty, length) < length) {
- /* break out & wait for throttling/unthrottling to
- happen */
- dbg("%s - receive room low", __func__);
- break;
- }
-
- /* Handle errors and break */
- error_flag = TTY_NORMAL;
- /* Although the device uses a bitmask and hence can have
- multiple errors on a packet - the order here sets the
- priority the error is returned to the tty layer */
-
- if (data[packet_offset+1] & FTDI_RS_OE) {
- error_flag = TTY_OVERRUN;
- dbg("OVERRRUN error");
- }
- if (data[packet_offset+1] & FTDI_RS_BI) {
- error_flag = TTY_BREAK;
- dbg("BREAK received");
- usb_serial_handle_break(port);
- }
- if (data[packet_offset+1] & FTDI_RS_PE) {
- error_flag = TTY_PARITY;
- dbg("PARITY error");
- }
- if (data[packet_offset+1] & FTDI_RS_FE) {
- error_flag = TTY_FRAME;
- dbg("FRAMING error");
- }
- if (length > 0) {
- for (i = 2; i < length+2; i++) {
- /* Note that the error flag is duplicated for
- every character received since we don't know
- which character it applied to */
- if (!usb_serial_handle_sysrq_char(tty, port,
- data[packet_offset + i]))
- tty_insert_flip_char(tty,
- data[packet_offset + i],
- error_flag);
- }
+ for (i = 0; i < urb->actual_length; i += priv->max_packet_size) {
+ len = min_t(int, urb->actual_length - i, priv->max_packet_size);
+ if (len > 2) {
+ ftdi_process_packet(tty, port, priv, &data[i], len);
need_flip = 1;
}
+ }

-#ifdef NOT_CORRECT_BUT_KEEPING_IT_FOR_NOW
- /* if a parity error is detected you get status packets forever
- until a character is sent without a parity error.
- This doesn't work well since the application receives a
- never ending stream of bad data - even though new data
- hasn't been sent. Therefore I (bill) have taken this out.
- However - this might make sense for framing errors and so on
- so I am leaving the code in for now.
- */
- else {
- if (error_flag != TTY_NORMAL) {
- dbg("error_flag is not normal");
- /* In this case it is just status - if that is
- an error send a bad character */
- if (tty->flip.count >= TTY_FLIPBUF_SIZE)
- tty_flip_buffer_push(tty);
- tty_insert_flip_char(tty, 0xff, error_flag);
- need_flip = 1;
- }
- }
-#endif
- } /* "for(packet_offset=0..." */
-
- /* Low latency */
if (need_flip)
tty_flip_buffer_push(tty);
+ tty_kref_put(tty);
+}

- if (packet_offset < urb->actual_length) {
- /* not completely processed - record progress */
- priv->rx_processed = packet_offset;
- dbg("%s - incomplete, %d bytes processed, %d remain",
- __func__, packet_offset,
- urb->actual_length - packet_offset);
- /* check if we were throttled while processing */
- spin_lock_irqsave(&priv->rx_lock, flags);
- if (priv->rx_flags & THROTTLED) {
- priv->rx_flags |= ACTUALLY_THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- dbg("%s - deferring remainder until unthrottled",
- __func__);
- goto out;
- }
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- /* if the port is closed stop trying to read */
- if (port->port.count > 0)
- /* delay processing of remainder */
- schedule_delayed_work(&priv->rx_work, 1);
- else
- dbg("%s - port is closed", __func__);
- goto out;
- }
+static void ftdi_read_bulk_callback(struct urb *urb)
+{
+ struct usb_serial_port *port = urb->context;
+ unsigned char *data = urb->transfer_buffer;
+ int status = urb->status;
+ unsigned long flags;

- /* urb is completely processed */
- priv->rx_processed = 0;
+ dbg("%s - port %d", __func__, port->number);

- /* if the port is closed stop trying to read */
- if (port->port.count > 0) {
- /* Continue trying to always read */
- usb_fill_bulk_urb(port->read_urb, port->serial->dev,
- usb_rcvbulkpipe(port->serial->dev,
- port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
+ /* FIXME: remove? */
+ if (urb->number_of_packets > 0) {
+ dev_err(&port->dev, "%s transfer_buffer_length %d "
+ "actual_length %d number of packets %d\n", __func__,
+ urb->transfer_buffer_length,
+ urb->actual_length, urb->number_of_packets);
+ dev_err(&port->dev, "%s transfer_flags %x\n", __func__,
+ urb->transfer_flags);
+ }

- result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
- if (result)
- dev_err(&port->dev,
- "%s - failed resubmitting read urb, error %d\n",
- __func__, result);
+ if (unlikely(status != 0)) {
+ dbg("%s - nonzero read bulk status received: %d",
+ __func__, status);
+ return;
}
-out:
- tty_kref_put(tty);
-} /* ftdi_process_read */

+ /* FIXME: check urb != port->read_urb? */
+
+ usb_serial_debug_data(debug, &port->dev, __func__,
+ urb->actual_length, data);
+
+ ftdi_process_read(port);
+
+ spin_lock_irqsave(&port->lock, flags);
+ if (!port->throttled) {
+ spin_unlock_irqrestore(&port->lock, flags);
+ ftdi_submit_read_urb(port, GFP_ATOMIC);
+ } else
+ spin_unlock_irqrestore(&port->lock, flags);
+}

static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
{
@@ -2605,33 +2467,30 @@ static int ftdi_ioctl(struct tty_struct *tty, struct file *file,
static void ftdi_throttle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags |= THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = 1;
+ spin_unlock_irqrestore(&port->lock, flags);
}

-
-static void ftdi_unthrottle(struct tty_struct *tty)
+void ftdi_unthrottle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
- int actually_throttled;
+ int was_throttled;
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- actually_throttled = priv->rx_flags & ACTUALLY_THROTTLED;
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ was_throttled = port->throttled;
+ port->throttled = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

- if (actually_throttled)
- schedule_delayed_work(&priv->rx_work, 0);
+ if (was_throttled && !test_bit(ASYNCB_CLOSING, &port->port.flags))
+ ftdi_submit_read_urb(port, GFP_KERNEL);
}

static int __init ftdi_init(void)
--
1.6.4.2

2009-10-03 12:09:58

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Am Samstag, 3. Oktober 2009 13:42:29 schrieb Johan Hovold:
> +???????spin_lock_irqsave(&port->lock, flags);
> +???????if (!port->throttled) {
> +???????????????spin_unlock_irqrestore(&port->lock, flags);
> +???????????????ftdi_submit_read_urb(port, GFP_ATOMIC);
> +???????} else
> +???????????????spin_unlock_irqrestore(&port->lock, flags);
> +}
> ?
> ?static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
> ?{
> @@ -2605,33 +2467,30 @@ static int ftdi_ioctl(struct tty_struct *tty,
> struct file *file, static void ftdi_throttle(struct tty_struct *tty)
[..]
> +???????spin_lock_irqsave(&port->lock, flags);
> +???????port->throttled = 1;
> +???????spin_unlock_irqrestore(&port->lock, flags);
> ?}
> ?
> -
> -static void ftdi_unthrottle(struct tty_struct *tty)
> +void ftdi_unthrottle(struct tty_struct *tty)
[..]
> +???????spin_lock_irqsave(&port->lock, flags);
> +???????was_throttled = port->throttled;
> +???????port->throttled = 0;
> +???????spin_unlock_irqrestore(&port->lock, flags);
> ?
> -???????if (actually_throttled)
> -???????????????schedule_delayed_work(&priv->rx_work, 0);
> +???????if (was_throttled && !test_bit(ASYNCB_CLOSING, &port->port.flags))
> +???????????????ftdi_submit_read_urb(port, GFP_KERNEL);
> ?}

This unfortunately is incorrect. If an unthrottling happens before the read
callback runs you submit a running URB.

Regards
Oliver

2009-10-03 12:28:50

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Sat, Oct 03, 2009 at 02:11:40PM +0200, Oliver Neukum wrote:
> This unfortunately is incorrect. If an unthrottling happens before the read
> callback runs you submit a running URB.

Duh. :) Here's an update that uses both throttle flags which is of
course required.

Thanks,
Johan

>From 41c00680e69b72bd8bd3650609b29ed5ec3f1dde Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Sat, 3 Oct 2009 12:30:13 +0200
Subject: [PATCH] USB: ftdi_sio: Rewrite.

- Remove work queue.
- Use urb status to determine when port is closed rather than port
count. (Fixes stalled reads due to open race on 2.6.31).
- Re-structure read processing.
- Use tty_insert_flip_string instead of per character push unless
console.
- Always process before throttling.
- Handle late call to unthrottle during serial_close by checking
ASYNCB_CLOSING.
- Remove rx_flags and lock and use flags in usb_serial_port instead.
- Remove unused rx_bytes counter.
---
drivers/usb/serial/ftdi_sio.c | 427 ++++++++++++++---------------------------
1 files changed, 144 insertions(+), 283 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 0ac2c2f..84c2b83 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -76,12 +76,7 @@ struct ftdi_private {
unsigned long last_dtr_rts; /* saved modem control outputs */
wait_queue_head_t delta_msr_wait; /* Used for TIOCMIWAIT */
char prev_status, diff_status; /* Used for TIOCMIWAIT */
- __u8 rx_flags; /* receive state flags (throttling) */
- spinlock_t rx_lock; /* spinlock for receive state */
- struct delayed_work rx_work;
struct usb_serial_port *port;
- int rx_processed;
- unsigned long rx_bytes;

__u16 interface; /* FT2232C, FT2232H or FT4232H port interface
(0 for FT232/245) */
@@ -737,10 +732,6 @@ static const char *ftdi_chip_name[] = {
/* Constants for read urb and write urb */
#define BUFSZ 512

-/* rx_flags */
-#define THROTTLED 0x01
-#define ACTUALLY_THROTTLED 0x02
-
/* Used for TIOCMIWAIT */
#define FTDI_STATUS_B0_MASK (FTDI_RS0_CTS | FTDI_RS0_DSR | FTDI_RS0_RI | FTDI_RS0_RLSD)
#define FTDI_STATUS_B1_MASK (FTDI_RS_BI)
@@ -763,7 +754,7 @@ static int ftdi_write_room(struct tty_struct *tty);
static int ftdi_chars_in_buffer(struct tty_struct *tty);
static void ftdi_write_bulk_callback(struct urb *urb);
static void ftdi_read_bulk_callback(struct urb *urb);
-static void ftdi_process_read(struct work_struct *work);
+static void ftdi_process_read(struct usb_serial_port *port);
static void ftdi_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old);
static int ftdi_tiocmget(struct tty_struct *tty, struct file *file);
@@ -1526,7 +1517,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
}

kref_init(&priv->kref);
- spin_lock_init(&priv->rx_lock);
spin_lock_init(&priv->tx_lock);
init_waitqueue_head(&priv->delta_msr_wait);
/* This will push the characters through immediately rather
@@ -1548,7 +1538,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
port->read_urb->transfer_buffer_length = BUFSZ;
}

- INIT_DELAYED_WORK(&priv->rx_work, ftdi_process_read);
priv->port = port;

/* Free port's existing write urb and transfer buffer. */
@@ -1685,6 +1674,26 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port)
return 0;
}

+int ftdi_submit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
+{
+ struct urb *urb = port->read_urb;
+ struct usb_serial *serial = port->serial;
+ int result;
+
+ usb_fill_bulk_urb(urb, serial->dev,
+ usb_rcvbulkpipe(serial->dev,
+ port->bulk_in_endpointAddress),
+ urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ ftdi_read_bulk_callback, port);
+ result = usb_submit_urb(urb, mem_flags);
+ if (result)
+ dev_err(&port->dev,
+ "%s - failed submitting read urb, error %d\n",
+ __func__, result);
+ return result;
+}
+
static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
{ /* ftdi_open */
struct usb_device *dev = port->serial->dev;
@@ -1699,9 +1708,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_lock_irqsave(&priv->tx_lock, flags);
priv->tx_bytes = 0;
spin_unlock_irqrestore(&priv->tx_lock, flags);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes = 0;
- spin_unlock_irqrestore(&priv->rx_lock, flags);

write_latency_timer(port);

@@ -1721,23 +1727,14 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
ftdi_set_termios(tty, port, tty->termios);

/* Not throttled */
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = 0;
+ port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

/* Start reading from the device */
- priv->rx_processed = 0;
- usb_fill_bulk_urb(port->read_urb, dev,
- usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
- result = usb_submit_urb(port->read_urb, GFP_KERNEL);
- if (result)
- dev_err(&port->dev,
- "%s - failed submitting read urb, error %d\n",
- __func__, result);
- else
+ result = ftdi_submit_read_urb(port, GFP_KERNEL);
+ if (!result)
kref_get(&priv->kref);

return result;
@@ -1783,10 +1780,6 @@ static void ftdi_close(struct usb_serial_port *port)

dbg("%s", __func__);

-
- /* cancel any scheduled reading */
- cancel_delayed_work_sync(&priv->rx_work);
-
/* shutdown our bulk read */
usb_kill_urb(port->read_urb);
kref_put(&priv->kref, ftdi_sio_priv_release);
@@ -2009,271 +2002,142 @@ static int ftdi_chars_in_buffer(struct tty_struct *tty)
return buffered;
}

-static void ftdi_read_bulk_callback(struct urb *urb)
+static void ftdi_process_packet(struct tty_struct *tty,
+ struct usb_serial_port *port, struct ftdi_private *priv,
+ char *packet, int len)
{
- struct usb_serial_port *port = urb->context;
- struct tty_struct *tty;
- struct ftdi_private *priv;
- unsigned long countread;
- unsigned long flags;
- int status = urb->status;
-
- if (urb->number_of_packets > 0) {
- dev_err(&port->dev, "%s transfer_buffer_length %d "
- "actual_length %d number of packets %d\n", __func__,
- urb->transfer_buffer_length,
- urb->actual_length, urb->number_of_packets);
- dev_err(&port->dev, "%s transfer_flags %x\n", __func__,
- urb->transfer_flags);
- }
+ int i;
+ char status;
+ char flag;
+ char *ch;

dbg("%s - port %d", __func__, port->number);

- if (port->port.count <= 0)
- return;
-
- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
- return;
+ /* Compare new line status to the old one, signal if different/
+ N.B. packet may be processed more than once, but differences
+ are only processed once. */
+ status = packet[0] & FTDI_STATUS_B0_MASK;
+ if (status != priv->prev_status) {
+ priv->diff_status |= status ^ priv->prev_status;
+ wake_up_interruptible(&priv->delta_msr_wait);
+ priv->prev_status = status;
}

- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
+ /*
+ * Although the device uses a bitmask and hence can have multiple
+ * errors on a packet - the order here sets the priority the error is
+ * returned to the tty layer.
+ */
+ flag = TTY_NORMAL;
+ if (packet[1] & FTDI_RS_OE) {
+ flag = TTY_OVERRUN;
+ dbg("OVERRRUN error");
}
-
- if (urb != port->read_urb)
- dev_err(&port->dev, "%s - Not my urb!\n", __func__);
-
- if (status) {
- /* This will happen at close every time so it is a dbg not an
- err */
- dbg("(this is ok on close) nonzero read bulk status received: %d", status);
- goto out;
+ if (packet[1] & FTDI_RS_BI) {
+ flag = TTY_BREAK;
+ dbg("BREAK received");
+ usb_serial_handle_break(port);
+ }
+ if (packet[1] & FTDI_RS_PE) {
+ flag = TTY_PARITY;
+ dbg("PARITY error");
+ }
+ if (packet[1] & FTDI_RS_FE) {
+ flag = TTY_FRAME;
+ dbg("FRAMING error");
}

- /* count data bytes, but not status bytes */
- countread = urb->actual_length;
- countread -= 2 * DIV_ROUND_UP(countread, priv->max_packet_size);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes += countread;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
-
- ftdi_process_read(&priv->rx_work.work);
-out:
- tty_kref_put(tty);
-} /* ftdi_read_bulk_callback */
+ /*
+ * The per character mucking around with sysrq path it too slow, so
+ * shortcircuit it in the 99.9999999% of cases where the USB serial is
+ * not a console anyway.
+ */
+ ch = packet + 2;
+ len -= 2;
+ if (!port->console || !port->sysrq)
+ tty_insert_flip_string(tty, ch, len);
+ else {
+ /* Push data to tty */
+ for (i = 0; i < len; i++, ch++) {
+ if (!usb_serial_handle_sysrq_char(tty, port, *ch))
+ tty_insert_flip_char(tty, *ch, flag);
+ }
+ }
+}

-static void ftdi_process_read(struct work_struct *work)
-{ /* ftdi_process_read */
- struct ftdi_private *priv =
- container_of(work, struct ftdi_private, rx_work.work);
- struct usb_serial_port *port = priv->port;
- struct urb *urb;
+static void ftdi_process_read(struct usb_serial_port *port)
+{
+ struct urb *urb = port->read_urb;
struct tty_struct *tty;
- char error_flag;
- unsigned char *data;
-
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ char *data = (char *)urb->transfer_buffer;
int i;
- int result;
+ int len;
int need_flip;
- int packet_offset;
- unsigned long flags;

- dbg("%s - port %d", __func__, port->number);
-
- if (port->port.count <= 0)
- return;
+ if (urb->actual_length <= 2)
+ return; /* status only */

- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
+ tty = tty_port_tty_get(&port->port);
+ if (!tty)
return;
- }

- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
- }
-
- urb = port->read_urb;
- if (!urb) {
- dbg("%s - bad read_urb pointer - exiting", __func__);
- goto out;
- }
-
- data = urb->transfer_buffer;
-
- if (priv->rx_processed) {
- dbg("%s - already processed: %d bytes, %d remain", __func__,
- priv->rx_processed,
- urb->actual_length - priv->rx_processed);
- } else {
- /* The first two bytes of every read packet are status */
- if (urb->actual_length > 2)
- usb_serial_debug_data(debug, &port->dev, __func__,
- urb->actual_length, data);
- else
- dbg("Status only: %03oo %03oo", data[0], data[1]);
- }
-
-
- /* TO DO -- check for hung up line and handle appropriately: */
- /* send hangup */
- /* See acm.c - you do a tty_hangup - eg tty_hangup(tty) */
- /* if CD is dropped and the line is not CLOCAL then we should hangup */
-
- need_flip = 0;
- for (packet_offset = priv->rx_processed;
- packet_offset < urb->actual_length; packet_offset += priv->max_packet_size) {
- int length;
-
- /* Compare new line status to the old one, signal if different/
- N.B. packet may be processed more than once, but differences
- are only processed once. */
- char new_status = data[packet_offset + 0] &
- FTDI_STATUS_B0_MASK;
- if (new_status != priv->prev_status) {
- priv->diff_status |=
- new_status ^ priv->prev_status;
- wake_up_interruptible(&priv->delta_msr_wait);
- priv->prev_status = new_status;
- }
+ /* FIXME: check priv? */

- length = min_t(u32, priv->max_packet_size, urb->actual_length-packet_offset)-2;
- if (length < 0) {
- dev_err(&port->dev, "%s - bad packet length: %d\n",
- __func__, length+2);
- length = 0;
- }
-
- if (priv->rx_flags & THROTTLED) {
- dbg("%s - throttled", __func__);
- break;
- }
- if (tty_buffer_request_room(tty, length) < length) {
- /* break out & wait for throttling/unthrottling to
- happen */
- dbg("%s - receive room low", __func__);
- break;
- }
-
- /* Handle errors and break */
- error_flag = TTY_NORMAL;
- /* Although the device uses a bitmask and hence can have
- multiple errors on a packet - the order here sets the
- priority the error is returned to the tty layer */
-
- if (data[packet_offset+1] & FTDI_RS_OE) {
- error_flag = TTY_OVERRUN;
- dbg("OVERRRUN error");
- }
- if (data[packet_offset+1] & FTDI_RS_BI) {
- error_flag = TTY_BREAK;
- dbg("BREAK received");
- usb_serial_handle_break(port);
- }
- if (data[packet_offset+1] & FTDI_RS_PE) {
- error_flag = TTY_PARITY;
- dbg("PARITY error");
- }
- if (data[packet_offset+1] & FTDI_RS_FE) {
- error_flag = TTY_FRAME;
- dbg("FRAMING error");
- }
- if (length > 0) {
- for (i = 2; i < length+2; i++) {
- /* Note that the error flag is duplicated for
- every character received since we don't know
- which character it applied to */
- if (!usb_serial_handle_sysrq_char(tty, port,
- data[packet_offset + i]))
- tty_insert_flip_char(tty,
- data[packet_offset + i],
- error_flag);
- }
+ for (i = 0; i < urb->actual_length; i += priv->max_packet_size) {
+ len = min_t(int, urb->actual_length - i, priv->max_packet_size);
+ if (len > 2) {
+ ftdi_process_packet(tty, port, priv, &data[i], len);
need_flip = 1;
}
+ }

-#ifdef NOT_CORRECT_BUT_KEEPING_IT_FOR_NOW
- /* if a parity error is detected you get status packets forever
- until a character is sent without a parity error.
- This doesn't work well since the application receives a
- never ending stream of bad data - even though new data
- hasn't been sent. Therefore I (bill) have taken this out.
- However - this might make sense for framing errors and so on
- so I am leaving the code in for now.
- */
- else {
- if (error_flag != TTY_NORMAL) {
- dbg("error_flag is not normal");
- /* In this case it is just status - if that is
- an error send a bad character */
- if (tty->flip.count >= TTY_FLIPBUF_SIZE)
- tty_flip_buffer_push(tty);
- tty_insert_flip_char(tty, 0xff, error_flag);
- need_flip = 1;
- }
- }
-#endif
- } /* "for(packet_offset=0..." */
-
- /* Low latency */
if (need_flip)
tty_flip_buffer_push(tty);
+ tty_kref_put(tty);
+}

- if (packet_offset < urb->actual_length) {
- /* not completely processed - record progress */
- priv->rx_processed = packet_offset;
- dbg("%s - incomplete, %d bytes processed, %d remain",
- __func__, packet_offset,
- urb->actual_length - packet_offset);
- /* check if we were throttled while processing */
- spin_lock_irqsave(&priv->rx_lock, flags);
- if (priv->rx_flags & THROTTLED) {
- priv->rx_flags |= ACTUALLY_THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- dbg("%s - deferring remainder until unthrottled",
- __func__);
- goto out;
- }
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- /* if the port is closed stop trying to read */
- if (port->port.count > 0)
- /* delay processing of remainder */
- schedule_delayed_work(&priv->rx_work, 1);
- else
- dbg("%s - port is closed", __func__);
- goto out;
- }
+static void ftdi_read_bulk_callback(struct urb *urb)
+{
+ struct usb_serial_port *port = urb->context;
+ unsigned char *data = urb->transfer_buffer;
+ int status = urb->status;
+ unsigned long flags;

- /* urb is completely processed */
- priv->rx_processed = 0;
+ dbg("%s - port %d", __func__, port->number);

- /* if the port is closed stop trying to read */
- if (port->port.count > 0) {
- /* Continue trying to always read */
- usb_fill_bulk_urb(port->read_urb, port->serial->dev,
- usb_rcvbulkpipe(port->serial->dev,
- port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
+ /* FIXME: remove? */
+ if (urb->number_of_packets > 0) {
+ dev_err(&port->dev, "%s transfer_buffer_length %d "
+ "actual_length %d number of packets %d\n", __func__,
+ urb->transfer_buffer_length,
+ urb->actual_length, urb->number_of_packets);
+ dev_err(&port->dev, "%s transfer_flags %x\n", __func__,
+ urb->transfer_flags);
+ }

- result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
- if (result)
- dev_err(&port->dev,
- "%s - failed resubmitting read urb, error %d\n",
- __func__, result);
+ if (unlikely(status != 0)) {
+ dbg("%s - nonzero read bulk status received: %d",
+ __func__, status);
+ return;
}
-out:
- tty_kref_put(tty);
-} /* ftdi_process_read */

+ /* FIXME: check urb != port->read_urb? */
+
+ usb_serial_debug_data(debug, &port->dev, __func__,
+ urb->actual_length, data);
+
+ ftdi_process_read(port);
+
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = port->throttle_req;
+ if (!port->throttled) {
+ spin_unlock_irqrestore(&port->lock, flags);
+ ftdi_submit_read_urb(port, GFP_ATOMIC);
+ } else
+ spin_unlock_irqrestore(&port->lock, flags);
+}

static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
{
@@ -2605,33 +2469,30 @@ static int ftdi_ioctl(struct tty_struct *tty, struct file *file,
static void ftdi_throttle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags |= THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttle_req = 1;
+ spin_unlock_irqrestore(&port->lock, flags);
}

-
-static void ftdi_unthrottle(struct tty_struct *tty)
+void ftdi_unthrottle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
- int actually_throttled;
+ int was_throttled;
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- actually_throttled = priv->rx_flags & ACTUALLY_THROTTLED;
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ was_throttled = port->throttled;
+ port->throttled = port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

- if (actually_throttled)
- schedule_delayed_work(&priv->rx_work, 0);
+ if (was_throttled && !test_bit(ASYNCB_CLOSING, &port->port.flags))
+ ftdi_submit_read_urb(port, GFP_KERNEL);
}

static int __init ftdi_init(void)
--
1.6.4.2

2009-10-03 13:07:50

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Fri, 02 Oct 2009 16:00:41 -0700
[email protected] (Eric W. Biederman) wrote:

>
>
> I have seen a hang in:
> /bin/stty (changing the baud rate)
> set_termios
> tty_wait_until_sent
> tty_chars_in_buffer
> ftdi_chars_in_buffer
>
> Where the driver wedged for a serial port and no progress
> was made.
>
> This happened to me several times with 2.6.31. My initial
> hypothesis was this was a hardware error (as it only happened
> on single piece of hardware). With all of the driver problems
> I suspect it could be a driver bug.

Driver bug I would think - or setup. If you've genuinely got the port
flow controlled then a request to set the termios after the I/O will wait
until a signal or carrier change (or indeed forever) quite correctly.

2009-10-03 13:16:50

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Sat, 3 Oct 2009 13:42:29 +0200
Johan Hovold <[email protected]> wrote:

> On Fri, Oct 02, 2009 at 05:33:08PM +0100, Alan Cox wrote:
> > For upstream how does this look as a tidy up
>
> I found a couple of issues:
>
> - The patch breaks unthrottling, as it checks THROTTLE before resubmitting
> urb whereas unthrottle checks ACTUALLY_UNTROTTLED which is never set.
>
> - The countread stuff should also be removed from
> ftdi_read_bulk_callback as they where only used for updating
> rx_bytes,
>
> And, obviously, this doesn't solve the problem of tty_flip_buffer_push
> being called from interrupt context (but I assume that was never the
> intention).

Calling tty_flip_buffer_push from an interrupt is perfectly acceptable
providing tty->low_latency isn't set: which it isn't.

> I've actually been working on cleaning up ftdi_sio inspired by the
> generic driver. I also threw out the work queue, but had not noticed the
> unused rx_byte yet. I also used the generic drivers scheme of not
> pushing to tty until unthrottled, but as you point out this is not really
> needed anymore.

The generic driver is a very bad example to follow in some areas but
this looks a big improvement. There are some patches reworking the
generic code to use kfifo on the output side which make it vastly better.
Not sure where the relevant google submissions went ?

> - spin_lock_irqsave(&priv->rx_lock, flags);
> - priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
> - spin_unlock_irqrestore(&priv->rx_lock, flags);
> + spin_lock_irqsave(&port->lock, flags);
> + port->throttled = 0;
> + spin_unlock_irqrestore(&port->lock, flags);

If you only have a single bit use the set_bit/clear_bit/test_and_xxx_bit
stuff as it's faster on most boxes

> + * The per character mucking around with sysrq path it too slow, so
> + * shortcircuit it in the 99.9999999% of cases where the USB serial is
> + * not a console anyway.
> + */
> + ch = packet + 2;
> + len -= 2;
> + if (!port->console || !port->sysrq)

You need && flag == TTY_NORMAL ?

Definitely a move in the right direction

2009-10-03 13:26:00

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Am Samstag, 3. Oktober 2009 15:18:07 schrieb Alan Cox:
> The generic driver is a very bad example to follow in some areas but
> this looks a big improvement. There are some patches reworking the
> generic code to use kfifo on the output side which make it vastly better.
> Not sure where the relevant google submissions went ?

They have been included into rc1.

> > -?????spin_lock_irqsave(&priv->rx_lock, flags);
> > -?????priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
> > -?????spin_unlock_irqrestore(&priv->rx_lock, flags);
> > +?????spin_lock_irqsave(&port->lock, flags);
> > +?????port->throttled = 0;
> > +?????spin_unlock_irqrestore(&port->lock, flags);
>
> If you only have a single bit use the set_bit/clear_bit/test_and_xxx_bit
> stuff as it's faster on most boxes

I think we cannot do with less than two flags, whose transition has
to be atomic here.

Regards
Oliver

2009-10-03 13:29:34

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Am Samstag, 3. Oktober 2009 14:28:48 schrieb Johan Hovold:
> +void ftdi_unthrottle(struct tty_struct *tty)
[..]
> +???????spin_lock_irqsave(&port->lock, flags);
> +???????was_throttled = port->throttled;
> +???????port->throttled = port->throttle_req = 0;
> +???????spin_unlock_irqrestore(&port->lock, flags);
> ?
> -???????if (actually_throttled)
> -???????????????schedule_delayed_work(&priv->rx_work, 0);
> +???????if (was_throttled && !test_bit(ASYNCB_CLOSING, &port->port.flags))
> +???????????????ftdi_submit_read_urb(port, GFP_KERNEL);
> ?}

Did you check whether this races with resume()?
If so, you'll need to submit with the spinlock held.

Regards
Oliver

2009-10-03 14:05:23

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Sat, Oct 03, 2009 at 02:18:07PM +0100, Alan Cox wrote:
> On Sat, 3 Oct 2009 13:42:29 +0200
> Johan Hovold <[email protected]> wrote:
> > On Fri, Oct 02, 2009 at 05:33:08PM +0100, Alan Cox wrote:
> > And, obviously, this doesn't solve the problem of tty_flip_buffer_push
> > being called from interrupt context (but I assume that was never the
> > intention).
>
> Calling tty_flip_buffer_push from an interrupt is perfectly acceptable
> providing tty->low_latency isn't set: which it isn't.

Of course -- the "with low latency set" part fell out (or, was implicit
;-) ). Your patch, however, still has low_latency set when it calls
tty_flip_buffer_push and that's the problem.

> > - spin_lock_irqsave(&priv->rx_lock, flags);
> > - priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
> > - spin_unlock_irqrestore(&priv->rx_lock, flags);
> > + spin_lock_irqsave(&port->lock, flags);
> > + port->throttled = 0;
> > + spin_unlock_irqrestore(&port->lock, flags);
>
> If you only have a single bit use the set_bit/clear_bit/test_and_xxx_bit
> stuff as it's faster on most boxes

The generic driver uses two fields in usb_serial_port for
throttled/trottle_req, whereas ftdi_sio, whiteheat, aircable, cypress
and perhaps a couple more have private a flag field for THROTTLED and
ACTUALLY_THROTTLED.

How about unifying them to all use a single flag field (with two flags)
in usb_serial_port?

> > + * The per character mucking around with sysrq path it too slow, so
> > + * shortcircuit it in the 99.9999999% of cases where the USB serial is
> > + * not a console anyway.
> > + */
> > + ch = packet + 2;
> > + len -= 2;
> > + if (!port->console || !port->sysrq)
>
> You need && flag == TTY_NORMAL ?

You tell me. :-) Are we interested in them unless port->console is set?

> Definitely a move in the right direction

Thanks,
Johan

2009-10-03 14:41:15

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Sat, Oct 03, 2009 at 02:28:48PM +0200, Johan Hovold wrote:
> Duh. :) Here's an update that uses both throttle flags which is of
> course required.

A minor update which processes every packet including status only ones.

/Johan

From: Johan Hovold <[email protected]>
Date: Sat, 3 Oct 2009 12:30:13 +0200
Subject: [PATCH] USB: ftdi_sio: Rewrite.

- Remove work queue.
- Use urb status to determine when port is closed rather than port
count. (Fixes stalled reads due to open race on 2.6.31).
- Re-structure read processing.
- Use tty_insert_flip_string instead of per character push unless
console.
- Always process before throttling.
- Handle late call to unthrottle during serial_close by checking
ASYNCB_CLOSING.
- Remove rx_flags and lock and use flags in usb_serial_port instead.
- Remove unused rx_bytes counter.
---
drivers/usb/serial/ftdi_sio.c | 431 ++++++++++++++---------------------------
1 files changed, 146 insertions(+), 285 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 0ac2c2f..287f2df 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -76,12 +76,7 @@ struct ftdi_private {
unsigned long last_dtr_rts; /* saved modem control outputs */
wait_queue_head_t delta_msr_wait; /* Used for TIOCMIWAIT */
char prev_status, diff_status; /* Used for TIOCMIWAIT */
- __u8 rx_flags; /* receive state flags (throttling) */
- spinlock_t rx_lock; /* spinlock for receive state */
- struct delayed_work rx_work;
struct usb_serial_port *port;
- int rx_processed;
- unsigned long rx_bytes;

__u16 interface; /* FT2232C, FT2232H or FT4232H port interface
(0 for FT232/245) */
@@ -737,10 +732,6 @@ static const char *ftdi_chip_name[] = {
/* Constants for read urb and write urb */
#define BUFSZ 512

-/* rx_flags */
-#define THROTTLED 0x01
-#define ACTUALLY_THROTTLED 0x02
-
/* Used for TIOCMIWAIT */
#define FTDI_STATUS_B0_MASK (FTDI_RS0_CTS | FTDI_RS0_DSR | FTDI_RS0_RI | FTDI_RS0_RLSD)
#define FTDI_STATUS_B1_MASK (FTDI_RS_BI)
@@ -763,7 +754,7 @@ static int ftdi_write_room(struct tty_struct *tty);
static int ftdi_chars_in_buffer(struct tty_struct *tty);
static void ftdi_write_bulk_callback(struct urb *urb);
static void ftdi_read_bulk_callback(struct urb *urb);
-static void ftdi_process_read(struct work_struct *work);
+static void ftdi_process_read(struct usb_serial_port *port);
static void ftdi_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old);
static int ftdi_tiocmget(struct tty_struct *tty, struct file *file);
@@ -1526,7 +1517,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
}

kref_init(&priv->kref);
- spin_lock_init(&priv->rx_lock);
spin_lock_init(&priv->tx_lock);
init_waitqueue_head(&priv->delta_msr_wait);
/* This will push the characters through immediately rather
@@ -1548,7 +1538,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
port->read_urb->transfer_buffer_length = BUFSZ;
}

- INIT_DELAYED_WORK(&priv->rx_work, ftdi_process_read);
priv->port = port;

/* Free port's existing write urb and transfer buffer. */
@@ -1685,6 +1674,26 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port)
return 0;
}

+int ftdi_submit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
+{
+ struct urb *urb = port->read_urb;
+ struct usb_serial *serial = port->serial;
+ int result;
+
+ usb_fill_bulk_urb(urb, serial->dev,
+ usb_rcvbulkpipe(serial->dev,
+ port->bulk_in_endpointAddress),
+ urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ ftdi_read_bulk_callback, port);
+ result = usb_submit_urb(urb, mem_flags);
+ if (result)
+ dev_err(&port->dev,
+ "%s - failed submitting read urb, error %d\n",
+ __func__, result);
+ return result;
+}
+
static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
{ /* ftdi_open */
struct usb_device *dev = port->serial->dev;
@@ -1699,9 +1708,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_lock_irqsave(&priv->tx_lock, flags);
priv->tx_bytes = 0;
spin_unlock_irqrestore(&priv->tx_lock, flags);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes = 0;
- spin_unlock_irqrestore(&priv->rx_lock, flags);

write_latency_timer(port);

@@ -1721,23 +1727,14 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
ftdi_set_termios(tty, port, tty->termios);

/* Not throttled */
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = 0;
+ port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

/* Start reading from the device */
- priv->rx_processed = 0;
- usb_fill_bulk_urb(port->read_urb, dev,
- usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
- result = usb_submit_urb(port->read_urb, GFP_KERNEL);
- if (result)
- dev_err(&port->dev,
- "%s - failed submitting read urb, error %d\n",
- __func__, result);
- else
+ result = ftdi_submit_read_urb(port, GFP_KERNEL);
+ if (!result)
kref_get(&priv->kref);

return result;
@@ -1783,10 +1780,6 @@ static void ftdi_close(struct usb_serial_port *port)

dbg("%s", __func__);

-
- /* cancel any scheduled reading */
- cancel_delayed_work_sync(&priv->rx_work);
-
/* shutdown our bulk read */
usb_kill_urb(port->read_urb);
kref_put(&priv->kref, ftdi_sio_priv_release);
@@ -2009,271 +2002,142 @@ static int ftdi_chars_in_buffer(struct tty_struct *tty)
return buffered;
}

-static void ftdi_read_bulk_callback(struct urb *urb)
+static int ftdi_process_packet(struct tty_struct *tty,
+ struct usb_serial_port *port, struct ftdi_private *priv,
+ char *packet, int len)
{
- struct usb_serial_port *port = urb->context;
- struct tty_struct *tty;
- struct ftdi_private *priv;
- unsigned long countread;
- unsigned long flags;
- int status = urb->status;
-
- if (urb->number_of_packets > 0) {
- dev_err(&port->dev, "%s transfer_buffer_length %d "
- "actual_length %d number of packets %d\n", __func__,
- urb->transfer_buffer_length,
- urb->actual_length, urb->number_of_packets);
- dev_err(&port->dev, "%s transfer_flags %x\n", __func__,
- urb->transfer_flags);
- }
+ int i;
+ char status;
+ char flag;
+ char *ch;

dbg("%s - port %d", __func__, port->number);

- if (port->port.count <= 0)
- return;
-
- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
- return;
+ if (len < 2) {
+ dbg("malformed packet");
+ return 0;
}

- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
+ /* Compare new line status to the old one, signal if different/
+ N.B. packet may be processed more than once, but differences
+ are only processed once. */
+ status = packet[0] & FTDI_STATUS_B0_MASK;
+ if (status != priv->prev_status) {
+ priv->diff_status |= status ^ priv->prev_status;
+ wake_up_interruptible(&priv->delta_msr_wait);
+ priv->prev_status = status;
}

- if (urb != port->read_urb)
- dev_err(&port->dev, "%s - Not my urb!\n", __func__);
-
- if (status) {
- /* This will happen at close every time so it is a dbg not an
- err */
- dbg("(this is ok on close) nonzero read bulk status received: %d", status);
- goto out;
+ /*
+ * Although the device uses a bitmask and hence can have multiple
+ * errors on a packet - the order here sets the priority the error is
+ * returned to the tty layer.
+ */
+ flag = TTY_NORMAL;
+ if (packet[1] & FTDI_RS_OE) {
+ flag = TTY_OVERRUN;
+ dbg("OVERRRUN error");
}
-
- /* count data bytes, but not status bytes */
- countread = urb->actual_length;
- countread -= 2 * DIV_ROUND_UP(countread, priv->max_packet_size);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes += countread;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
-
- ftdi_process_read(&priv->rx_work.work);
-out:
- tty_kref_put(tty);
-} /* ftdi_read_bulk_callback */
-
-
-static void ftdi_process_read(struct work_struct *work)
-{ /* ftdi_process_read */
- struct ftdi_private *priv =
- container_of(work, struct ftdi_private, rx_work.work);
- struct usb_serial_port *port = priv->port;
- struct urb *urb;
- struct tty_struct *tty;
- char error_flag;
- unsigned char *data;
-
- int i;
- int result;
- int need_flip;
- int packet_offset;
- unsigned long flags;
-
- dbg("%s - port %d", __func__, port->number);
-
- if (port->port.count <= 0)
- return;
-
- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
- return;
+ if (packet[1] & FTDI_RS_BI) {
+ flag = TTY_BREAK;
+ dbg("BREAK received");
+ usb_serial_handle_break(port);
}
-
- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
+ if (packet[1] & FTDI_RS_PE) {
+ flag = TTY_PARITY;
+ dbg("PARITY error");
}
-
- urb = port->read_urb;
- if (!urb) {
- dbg("%s - bad read_urb pointer - exiting", __func__);
- goto out;
+ if (packet[1] & FTDI_RS_FE) {
+ flag = TTY_FRAME;
+ dbg("FRAMING error");
}

- data = urb->transfer_buffer;
-
- if (priv->rx_processed) {
- dbg("%s - already processed: %d bytes, %d remain", __func__,
- priv->rx_processed,
- urb->actual_length - priv->rx_processed);
- } else {
- /* The first two bytes of every read packet are status */
- if (urb->actual_length > 2)
- usb_serial_debug_data(debug, &port->dev, __func__,
- urb->actual_length, data);
- else
- dbg("Status only: %03oo %03oo", data[0], data[1]);
+ len -= 2;
+ if (!len)
+ return 0; /* status only */
+ ch = packet + 2;
+ /*
+ * The per character mucking around with sysrq path it too slow, so
+ * shortcircuit it in the 99.9999999% of cases where the USB serial is
+ * not a console anyway.
+ */
+ if (!port->console || !port->sysrq)
+ tty_insert_flip_string(tty, ch, len);
+ else {
+ for (i = 0; i < len; i++, ch++) {
+ if (!usb_serial_handle_sysrq_char(tty, port, *ch))
+ tty_insert_flip_char(tty, *ch, flag);
+ }
}
+ return len;
+}

+static void ftdi_process_read(struct usb_serial_port *port)
+{
+ struct urb *urb = port->read_urb;
+ struct tty_struct *tty;
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ char *data = (char *)urb->transfer_buffer;
+ int i;
+ int len;
+ int count = 0;

- /* TO DO -- check for hung up line and handle appropriately: */
- /* send hangup */
- /* See acm.c - you do a tty_hangup - eg tty_hangup(tty) */
- /* if CD is dropped and the line is not CLOCAL then we should hangup */
-
- need_flip = 0;
- for (packet_offset = priv->rx_processed;
- packet_offset < urb->actual_length; packet_offset += priv->max_packet_size) {
- int length;
-
- /* Compare new line status to the old one, signal if different/
- N.B. packet may be processed more than once, but differences
- are only processed once. */
- char new_status = data[packet_offset + 0] &
- FTDI_STATUS_B0_MASK;
- if (new_status != priv->prev_status) {
- priv->diff_status |=
- new_status ^ priv->prev_status;
- wake_up_interruptible(&priv->delta_msr_wait);
- priv->prev_status = new_status;
- }
+ tty = tty_port_tty_get(&port->port);
+ if (!tty)
+ return;

- length = min_t(u32, priv->max_packet_size, urb->actual_length-packet_offset)-2;
- if (length < 0) {
- dev_err(&port->dev, "%s - bad packet length: %d\n",
- __func__, length+2);
- length = 0;
- }
+ /* FIXME: check priv? */

- if (priv->rx_flags & THROTTLED) {
- dbg("%s - throttled", __func__);
- break;
- }
- if (tty_buffer_request_room(tty, length) < length) {
- /* break out & wait for throttling/unthrottling to
- happen */
- dbg("%s - receive room low", __func__);
- break;
- }
+ for (i = 0; i < urb->actual_length; i += priv->max_packet_size) {
+ len = min_t(int, urb->actual_length - i, priv->max_packet_size);
+ count += ftdi_process_packet(tty, port, priv, &data[i], len);
+ }

- /* Handle errors and break */
- error_flag = TTY_NORMAL;
- /* Although the device uses a bitmask and hence can have
- multiple errors on a packet - the order here sets the
- priority the error is returned to the tty layer */
+ if (count)
+ tty_flip_buffer_push(tty);
+ tty_kref_put(tty);
+}

- if (data[packet_offset+1] & FTDI_RS_OE) {
- error_flag = TTY_OVERRUN;
- dbg("OVERRRUN error");
- }
- if (data[packet_offset+1] & FTDI_RS_BI) {
- error_flag = TTY_BREAK;
- dbg("BREAK received");
- usb_serial_handle_break(port);
- }
- if (data[packet_offset+1] & FTDI_RS_PE) {
- error_flag = TTY_PARITY;
- dbg("PARITY error");
- }
- if (data[packet_offset+1] & FTDI_RS_FE) {
- error_flag = TTY_FRAME;
- dbg("FRAMING error");
- }
- if (length > 0) {
- for (i = 2; i < length+2; i++) {
- /* Note that the error flag is duplicated for
- every character received since we don't know
- which character it applied to */
- if (!usb_serial_handle_sysrq_char(tty, port,
- data[packet_offset + i]))
- tty_insert_flip_char(tty,
- data[packet_offset + i],
- error_flag);
- }
- need_flip = 1;
- }
+static void ftdi_read_bulk_callback(struct urb *urb)
+{
+ struct usb_serial_port *port = urb->context;
+ unsigned char *data = urb->transfer_buffer;
+ int status = urb->status;
+ unsigned long flags;

-#ifdef NOT_CORRECT_BUT_KEEPING_IT_FOR_NOW
- /* if a parity error is detected you get status packets forever
- until a character is sent without a parity error.
- This doesn't work well since the application receives a
- never ending stream of bad data - even though new data
- hasn't been sent. Therefore I (bill) have taken this out.
- However - this might make sense for framing errors and so on
- so I am leaving the code in for now.
- */
- else {
- if (error_flag != TTY_NORMAL) {
- dbg("error_flag is not normal");
- /* In this case it is just status - if that is
- an error send a bad character */
- if (tty->flip.count >= TTY_FLIPBUF_SIZE)
- tty_flip_buffer_push(tty);
- tty_insert_flip_char(tty, 0xff, error_flag);
- need_flip = 1;
- }
- }
-#endif
- } /* "for(packet_offset=0..." */
+ dbg("%s - port %d", __func__, port->number);

- /* Low latency */
- if (need_flip)
- tty_flip_buffer_push(tty);
+ /* FIXME: remove? */
+ if (urb->number_of_packets > 0) {
+ dev_err(&port->dev, "%s transfer_buffer_length %d "
+ "actual_length %d number of packets %d\n", __func__,
+ urb->transfer_buffer_length,
+ urb->actual_length, urb->number_of_packets);
+ dev_err(&port->dev, "%s transfer_flags %x\n", __func__,
+ urb->transfer_flags);
+ }

- if (packet_offset < urb->actual_length) {
- /* not completely processed - record progress */
- priv->rx_processed = packet_offset;
- dbg("%s - incomplete, %d bytes processed, %d remain",
- __func__, packet_offset,
- urb->actual_length - packet_offset);
- /* check if we were throttled while processing */
- spin_lock_irqsave(&priv->rx_lock, flags);
- if (priv->rx_flags & THROTTLED) {
- priv->rx_flags |= ACTUALLY_THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- dbg("%s - deferring remainder until unthrottled",
- __func__);
- goto out;
- }
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- /* if the port is closed stop trying to read */
- if (port->port.count > 0)
- /* delay processing of remainder */
- schedule_delayed_work(&priv->rx_work, 1);
- else
- dbg("%s - port is closed", __func__);
- goto out;
+ if (unlikely(status != 0)) {
+ dbg("%s - nonzero read bulk status received: %d",
+ __func__, status);
+ return;
}

- /* urb is completely processed */
- priv->rx_processed = 0;
+ /* FIXME: check urb != port->read_urb? */

- /* if the port is closed stop trying to read */
- if (port->port.count > 0) {
- /* Continue trying to always read */
- usb_fill_bulk_urb(port->read_urb, port->serial->dev,
- usb_rcvbulkpipe(port->serial->dev,
- port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
+ usb_serial_debug_data(debug, &port->dev, __func__,
+ urb->actual_length, data);

- result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
- if (result)
- dev_err(&port->dev,
- "%s - failed resubmitting read urb, error %d\n",
- __func__, result);
- }
-out:
- tty_kref_put(tty);
-} /* ftdi_process_read */
+ ftdi_process_read(port);

+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = port->throttle_req;
+ if (!port->throttled) {
+ spin_unlock_irqrestore(&port->lock, flags);
+ ftdi_submit_read_urb(port, GFP_ATOMIC);
+ } else
+ spin_unlock_irqrestore(&port->lock, flags);
+}

static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
{
@@ -2605,33 +2469,30 @@ static int ftdi_ioctl(struct tty_struct *tty, struct file *file,
static void ftdi_throttle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags |= THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttle_req = 1;
+ spin_unlock_irqrestore(&port->lock, flags);
}

-
-static void ftdi_unthrottle(struct tty_struct *tty)
+void ftdi_unthrottle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
- int actually_throttled;
+ int was_throttled;
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- actually_throttled = priv->rx_flags & ACTUALLY_THROTTLED;
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ was_throttled = port->throttled;
+ port->throttled = port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

- if (actually_throttled)
- schedule_delayed_work(&priv->rx_work, 0);
+ if (was_throttled && !test_bit(ASYNCB_CLOSING, &port->port.flags))
+ ftdi_submit_read_urb(port, GFP_KERNEL);
}

static int __init ftdi_init(void)
--
1.6.4.2

2009-10-03 16:32:57

by Alan

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

> Of course -- the "with low latency set" part fell out (or, was implicit
> ;-) ). Your patch, however, still has low_latency set when it calls
> tty_flip_buffer_push and that's the problem.

I don't think it does - I removed all the low latency setting.

> How about unifying them to all use a single flag field (with two flags)
> in usb_serial_port?

May make sense.

>
> > > + * The per character mucking around with sysrq path it too slow, so
> > > + * shortcircuit it in the 99.9999999% of cases where the USB serial is
> > > + * not a console anyway.
> > > + */
> > > + ch = packet + 2;
> > > + len -= 2;
> > > + if (!port->console || !port->sysrq)
> >
> > You need && flag == TTY_NORMAL ?
>
> You tell me. :-) Are we interested in them unless port->console is set?

Yes - we don;t care about the sysrq but we care about error characters
being reported ot the line discipline properly.

2009-10-03 16:47:08

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Sat, Oct 03, 2009 at 05:33:24PM +0100, Alan Cox wrote:
> > Of course -- the "with low latency set" part fell out (or, was implicit
> > ;-) ). Your patch, however, still has low_latency set when it calls
> > tty_flip_buffer_push and that's the problem.
>
> I don't think it does - I removed all the low latency setting.

It's still in the patch you posted (and ASYNC_LOW_LATENCY is left
unmodifed, that is set, by your patch):

@@ -1700,9 +1697,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_lock_irqsave(&priv->tx_lock, flags);
priv->tx_bytes = 0;
spin_unlock_irqrestore(&priv->tx_lock, flags);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes = 0;
- spin_unlock_irqrestore(&priv->rx_lock, flags);

if (tty)
tty->low_latency = (priv->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
@@ -1730,7 +1724,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_unlock_irqrestore(&priv->rx_lock, flags);

/* Start reading from the device */
- priv->rx_processed = 0;
usb_fill_bulk_urb(port->read_urb, dev,
usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
port->read_urb->transfer_buffer,

But nevermind, we agree it should go. :-)

2009-10-03 23:52:20

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

2009-10-04 19:49:22

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

> > > > + * The per character mucking around with sysrq path it too slow, so
> > > > + * shortcircuit it in the 99.9999999% of cases where the USB serial is
> > > > + * not a console anyway.
> > > > + */
> > > > + ch = packet + 2;
> > > > + len -= 2;
> > > > + if (!port->console || !port->sysrq)
> > >
> > > You need && flag == TTY_NORMAL ?
> >
> > You tell me. :-) Are we interested in them unless port->console is set?
>
> Yes - we don;t care about the sysrq but we care about error characters
> being reported ot the line discipline properly.

Wasn't thinking.. Here's an update which always passes error characters.
It also uses ASYNCB_INITIALIZED (instead of ASYNCB_CLOSING) in unthrottle.

Thanks,
Johan

From: Johan Hovold <[email protected]>
Date: Sun, 4 Oct 2009 21:41:07 +0200
Subject: [PATCH] USB: ftdi_sio: Rewrite.

- Remove work queue.
- Use urb status to determine when port is closed rather than port
count. (Fixes stalled reads due to open race on 2.6.31).
- Re-structure read processing.
- Use tty_insert_flip_string instead of per character push when
possible.
- Always process before throttling.
- Handle late call to unthrottle during serial_close by checking
ASYNCB_INITIALIZED.
- Remove rx_flags and lock and use flags in usb_serial_port instead.
- Remove unused rx_bytes counter.
- Remove obsolete error checks.
---
drivers/usb/serial/ftdi_sio.c | 418 +++++++++++++----------------------------
1 files changed, 130 insertions(+), 288 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 0ac2c2f..eb6d978 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -76,12 +76,7 @@ struct ftdi_private {
unsigned long last_dtr_rts; /* saved modem control outputs */
wait_queue_head_t delta_msr_wait; /* Used for TIOCMIWAIT */
char prev_status, diff_status; /* Used for TIOCMIWAIT */
- __u8 rx_flags; /* receive state flags (throttling) */
- spinlock_t rx_lock; /* spinlock for receive state */
- struct delayed_work rx_work;
struct usb_serial_port *port;
- int rx_processed;
- unsigned long rx_bytes;

__u16 interface; /* FT2232C, FT2232H or FT4232H port interface
(0 for FT232/245) */
@@ -737,10 +732,6 @@ static const char *ftdi_chip_name[] = {
/* Constants for read urb and write urb */
#define BUFSZ 512

-/* rx_flags */
-#define THROTTLED 0x01
-#define ACTUALLY_THROTTLED 0x02
-
/* Used for TIOCMIWAIT */
#define FTDI_STATUS_B0_MASK (FTDI_RS0_CTS | FTDI_RS0_DSR | FTDI_RS0_RI | FTDI_RS0_RLSD)
#define FTDI_STATUS_B1_MASK (FTDI_RS_BI)
@@ -763,7 +754,7 @@ static int ftdi_write_room(struct tty_struct *tty);
static int ftdi_chars_in_buffer(struct tty_struct *tty);
static void ftdi_write_bulk_callback(struct urb *urb);
static void ftdi_read_bulk_callback(struct urb *urb);
-static void ftdi_process_read(struct work_struct *work);
+static void ftdi_process_read(struct usb_serial_port *port);
static void ftdi_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old);
static int ftdi_tiocmget(struct tty_struct *tty, struct file *file);
@@ -1526,7 +1517,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
}

kref_init(&priv->kref);
- spin_lock_init(&priv->rx_lock);
spin_lock_init(&priv->tx_lock);
init_waitqueue_head(&priv->delta_msr_wait);
/* This will push the characters through immediately rather
@@ -1548,7 +1538,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
port->read_urb->transfer_buffer_length = BUFSZ;
}

- INIT_DELAYED_WORK(&priv->rx_work, ftdi_process_read);
priv->port = port;

/* Free port's existing write urb and transfer buffer. */
@@ -1685,6 +1674,26 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port)
return 0;
}

+int ftdi_submit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
+{
+ struct urb *urb = port->read_urb;
+ struct usb_serial *serial = port->serial;
+ int result;
+
+ usb_fill_bulk_urb(urb, serial->dev,
+ usb_rcvbulkpipe(serial->dev,
+ port->bulk_in_endpointAddress),
+ urb->transfer_buffer,
+ urb->transfer_buffer_length,
+ ftdi_read_bulk_callback, port);
+ result = usb_submit_urb(urb, mem_flags);
+ if (result)
+ dev_err(&port->dev,
+ "%s - failed submitting read urb, error %d\n",
+ __func__, result);
+ return result;
+}
+
static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
{ /* ftdi_open */
struct usb_device *dev = port->serial->dev;
@@ -1699,9 +1708,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
spin_lock_irqsave(&priv->tx_lock, flags);
priv->tx_bytes = 0;
spin_unlock_irqrestore(&priv->tx_lock, flags);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes = 0;
- spin_unlock_irqrestore(&priv->rx_lock, flags);

write_latency_timer(port);

@@ -1721,23 +1727,14 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
ftdi_set_termios(tty, port, tty->termios);

/* Not throttled */
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = 0;
+ port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

/* Start reading from the device */
- priv->rx_processed = 0;
- usb_fill_bulk_urb(port->read_urb, dev,
- usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
- result = usb_submit_urb(port->read_urb, GFP_KERNEL);
- if (result)
- dev_err(&port->dev,
- "%s - failed submitting read urb, error %d\n",
- __func__, result);
- else
+ result = ftdi_submit_read_urb(port, GFP_KERNEL);
+ if (!result)
kref_get(&priv->kref);

return result;
@@ -1783,10 +1780,6 @@ static void ftdi_close(struct usb_serial_port *port)

dbg("%s", __func__);

-
- /* cancel any scheduled reading */
- cancel_delayed_work_sync(&priv->rx_work);
-
/* shutdown our bulk read */
usb_kill_urb(port->read_urb);
kref_put(&priv->kref, ftdi_sio_priv_release);
@@ -2009,271 +2002,122 @@ static int ftdi_chars_in_buffer(struct tty_struct *tty)
return buffered;
}

-static void ftdi_read_bulk_callback(struct urb *urb)
+static int ftdi_process_packet(struct tty_struct *tty,
+ struct usb_serial_port *port, struct ftdi_private *priv,
+ char *packet, int len)
{
- struct usb_serial_port *port = urb->context;
- struct tty_struct *tty;
- struct ftdi_private *priv;
- unsigned long countread;
- unsigned long flags;
- int status = urb->status;
-
- if (urb->number_of_packets > 0) {
- dev_err(&port->dev, "%s transfer_buffer_length %d "
- "actual_length %d number of packets %d\n", __func__,
- urb->transfer_buffer_length,
- urb->actual_length, urb->number_of_packets);
- dev_err(&port->dev, "%s transfer_flags %x\n", __func__,
- urb->transfer_flags);
- }
+ int i;
+ char status;
+ char flag;
+ char *ch;

dbg("%s - port %d", __func__, port->number);

- if (port->port.count <= 0)
- return;
-
- tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
- return;
+ if (len < 2) {
+ dbg("malformed packet");
+ return 0;
}

- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
+ /* Compare new line status to the old one, signal if different/
+ N.B. packet may be processed more than once, but differences
+ are only processed once. */
+ status = packet[0] & FTDI_STATUS_B0_MASK;
+ if (status != priv->prev_status) {
+ priv->diff_status |= status ^ priv->prev_status;
+ wake_up_interruptible(&priv->delta_msr_wait);
+ priv->prev_status = status;
}

- if (urb != port->read_urb)
- dev_err(&port->dev, "%s - Not my urb!\n", __func__);
-
- if (status) {
- /* This will happen at close every time so it is a dbg not an
- err */
- dbg("(this is ok on close) nonzero read bulk status received: %d", status);
- goto out;
+ /*
+ * Although the device uses a bitmask and hence can have multiple
+ * errors on a packet - the order here sets the priority the error is
+ * returned to the tty layer.
+ */
+ flag = TTY_NORMAL;
+ if (packet[1] & FTDI_RS_OE) {
+ flag = TTY_OVERRUN;
+ dbg("OVERRRUN error");
+ }
+ if (packet[1] & FTDI_RS_BI) {
+ flag = TTY_BREAK;
+ dbg("BREAK received");
+ usb_serial_handle_break(port);
+ }
+ if (packet[1] & FTDI_RS_PE) {
+ flag = TTY_PARITY;
+ dbg("PARITY error");
+ }
+ if (packet[1] & FTDI_RS_FE) {
+ flag = TTY_FRAME;
+ dbg("FRAMING error");
}

- /* count data bytes, but not status bytes */
- countread = urb->actual_length;
- countread -= 2 * DIV_ROUND_UP(countread, priv->max_packet_size);
- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_bytes += countread;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
-
- ftdi_process_read(&priv->rx_work.work);
-out:
- tty_kref_put(tty);
-} /* ftdi_read_bulk_callback */
-
+ len -= 2;
+ if (!len)
+ return 0; /* status only */
+ ch = packet + 2;
+
+ if (!(port->console && port->sysrq) && flag == TTY_NORMAL)
+ tty_insert_flip_string(tty, ch, len);
+ else {
+ for (i = 0; i < len; i++, ch++) {
+ if (!usb_serial_handle_sysrq_char(tty, port, *ch))
+ tty_insert_flip_char(tty, *ch, flag);
+ }
+ }
+ return len;
+}

-static void ftdi_process_read(struct work_struct *work)
-{ /* ftdi_process_read */
- struct ftdi_private *priv =
- container_of(work, struct ftdi_private, rx_work.work);
- struct usb_serial_port *port = priv->port;
- struct urb *urb;
+static void ftdi_process_read(struct usb_serial_port *port)
+{
+ struct urb *urb = port->read_urb;
struct tty_struct *tty;
- char error_flag;
- unsigned char *data;
-
+ struct ftdi_private *priv = usb_get_serial_port_data(port);
+ char *data = (char *)urb->transfer_buffer;
int i;
- int result;
- int need_flip;
- int packet_offset;
- unsigned long flags;
-
- dbg("%s - port %d", __func__, port->number);
-
- if (port->port.count <= 0)
- return;
+ int len;
+ int count = 0;

tty = tty_port_tty_get(&port->port);
- if (!tty) {
- dbg("%s - bad tty pointer - exiting", __func__);
+ if (!tty)
return;
- }
-
- priv = usb_get_serial_port_data(port);
- if (!priv) {
- dbg("%s - bad port private data pointer - exiting", __func__);
- goto out;
- }
-
- urb = port->read_urb;
- if (!urb) {
- dbg("%s - bad read_urb pointer - exiting", __func__);
- goto out;
- }

- data = urb->transfer_buffer;
-
- if (priv->rx_processed) {
- dbg("%s - already processed: %d bytes, %d remain", __func__,
- priv->rx_processed,
- urb->actual_length - priv->rx_processed);
- } else {
- /* The first two bytes of every read packet are status */
- if (urb->actual_length > 2)
- usb_serial_debug_data(debug, &port->dev, __func__,
- urb->actual_length, data);
- else
- dbg("Status only: %03oo %03oo", data[0], data[1]);
+ for (i = 0; i < urb->actual_length; i += priv->max_packet_size) {
+ len = min_t(int, urb->actual_length - i, priv->max_packet_size);
+ count += ftdi_process_packet(tty, port, priv, &data[i], len);
}

-
- /* TO DO -- check for hung up line and handle appropriately: */
- /* send hangup */
- /* See acm.c - you do a tty_hangup - eg tty_hangup(tty) */
- /* if CD is dropped and the line is not CLOCAL then we should hangup */
-
- need_flip = 0;
- for (packet_offset = priv->rx_processed;
- packet_offset < urb->actual_length; packet_offset += priv->max_packet_size) {
- int length;
-
- /* Compare new line status to the old one, signal if different/
- N.B. packet may be processed more than once, but differences
- are only processed once. */
- char new_status = data[packet_offset + 0] &
- FTDI_STATUS_B0_MASK;
- if (new_status != priv->prev_status) {
- priv->diff_status |=
- new_status ^ priv->prev_status;
- wake_up_interruptible(&priv->delta_msr_wait);
- priv->prev_status = new_status;
- }
-
- length = min_t(u32, priv->max_packet_size, urb->actual_length-packet_offset)-2;
- if (length < 0) {
- dev_err(&port->dev, "%s - bad packet length: %d\n",
- __func__, length+2);
- length = 0;
- }
-
- if (priv->rx_flags & THROTTLED) {
- dbg("%s - throttled", __func__);
- break;
- }
- if (tty_buffer_request_room(tty, length) < length) {
- /* break out & wait for throttling/unthrottling to
- happen */
- dbg("%s - receive room low", __func__);
- break;
- }
-
- /* Handle errors and break */
- error_flag = TTY_NORMAL;
- /* Although the device uses a bitmask and hence can have
- multiple errors on a packet - the order here sets the
- priority the error is returned to the tty layer */
-
- if (data[packet_offset+1] & FTDI_RS_OE) {
- error_flag = TTY_OVERRUN;
- dbg("OVERRRUN error");
- }
- if (data[packet_offset+1] & FTDI_RS_BI) {
- error_flag = TTY_BREAK;
- dbg("BREAK received");
- usb_serial_handle_break(port);
- }
- if (data[packet_offset+1] & FTDI_RS_PE) {
- error_flag = TTY_PARITY;
- dbg("PARITY error");
- }
- if (data[packet_offset+1] & FTDI_RS_FE) {
- error_flag = TTY_FRAME;
- dbg("FRAMING error");
- }
- if (length > 0) {
- for (i = 2; i < length+2; i++) {
- /* Note that the error flag is duplicated for
- every character received since we don't know
- which character it applied to */
- if (!usb_serial_handle_sysrq_char(tty, port,
- data[packet_offset + i]))
- tty_insert_flip_char(tty,
- data[packet_offset + i],
- error_flag);
- }
- need_flip = 1;
- }
-
-#ifdef NOT_CORRECT_BUT_KEEPING_IT_FOR_NOW
- /* if a parity error is detected you get status packets forever
- until a character is sent without a parity error.
- This doesn't work well since the application receives a
- never ending stream of bad data - even though new data
- hasn't been sent. Therefore I (bill) have taken this out.
- However - this might make sense for framing errors and so on
- so I am leaving the code in for now.
- */
- else {
- if (error_flag != TTY_NORMAL) {
- dbg("error_flag is not normal");
- /* In this case it is just status - if that is
- an error send a bad character */
- if (tty->flip.count >= TTY_FLIPBUF_SIZE)
- tty_flip_buffer_push(tty);
- tty_insert_flip_char(tty, 0xff, error_flag);
- need_flip = 1;
- }
- }
-#endif
- } /* "for(packet_offset=0..." */
-
- /* Low latency */
- if (need_flip)
+ if (count)
tty_flip_buffer_push(tty);
+ tty_kref_put(tty);
+}

- if (packet_offset < urb->actual_length) {
- /* not completely processed - record progress */
- priv->rx_processed = packet_offset;
- dbg("%s - incomplete, %d bytes processed, %d remain",
- __func__, packet_offset,
- urb->actual_length - packet_offset);
- /* check if we were throttled while processing */
- spin_lock_irqsave(&priv->rx_lock, flags);
- if (priv->rx_flags & THROTTLED) {
- priv->rx_flags |= ACTUALLY_THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- dbg("%s - deferring remainder until unthrottled",
- __func__);
- goto out;
- }
- spin_unlock_irqrestore(&priv->rx_lock, flags);
- /* if the port is closed stop trying to read */
- if (port->port.count > 0)
- /* delay processing of remainder */
- schedule_delayed_work(&priv->rx_work, 1);
- else
- dbg("%s - port is closed", __func__);
- goto out;
- }
-
- /* urb is completely processed */
- priv->rx_processed = 0;
+static void ftdi_read_bulk_callback(struct urb *urb)
+{
+ struct usb_serial_port *port = urb->context;
+ int status = urb->status;
+ unsigned long flags;

- /* if the port is closed stop trying to read */
- if (port->port.count > 0) {
- /* Continue trying to always read */
- usb_fill_bulk_urb(port->read_urb, port->serial->dev,
- usb_rcvbulkpipe(port->serial->dev,
- port->bulk_in_endpointAddress),
- port->read_urb->transfer_buffer,
- port->read_urb->transfer_buffer_length,
- ftdi_read_bulk_callback, port);
+ dbg("%s - port %d", __func__, port->number);

- result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
- if (result)
- dev_err(&port->dev,
- "%s - failed resubmitting read urb, error %d\n",
- __func__, result);
+ if (status) {
+ dbg("%s - nonzero read bulk status received: %d",
+ __func__, status);
+ return;
}
-out:
- tty_kref_put(tty);
-} /* ftdi_process_read */

+ usb_serial_debug_data(debug, &port->dev, __func__,
+ urb->actual_length, urb->transfer_buffer);
+ ftdi_process_read(port);
+
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = port->throttle_req;
+ if (!port->throttled) {
+ spin_unlock_irqrestore(&port->lock, flags);
+ ftdi_submit_read_urb(port, GFP_ATOMIC);
+ } else
+ spin_unlock_irqrestore(&port->lock, flags);
+}

static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
{
@@ -2605,33 +2449,31 @@ static int ftdi_ioctl(struct tty_struct *tty, struct file *file,
static void ftdi_throttle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- priv->rx_flags |= THROTTLED;
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttle_req = 1;
+ spin_unlock_irqrestore(&port->lock, flags);
}

-
-static void ftdi_unthrottle(struct tty_struct *tty)
+void ftdi_unthrottle(struct tty_struct *tty)
{
struct usb_serial_port *port = tty->driver_data;
- struct ftdi_private *priv = usb_get_serial_port_data(port);
- int actually_throttled;
+ int was_throttled;
unsigned long flags;

dbg("%s - port %d", __func__, port->number);

- spin_lock_irqsave(&priv->rx_lock, flags);
- actually_throttled = priv->rx_flags & ACTUALLY_THROTTLED;
- priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
- spin_unlock_irqrestore(&priv->rx_lock, flags);
+ spin_lock_irqsave(&port->lock, flags);
+ was_throttled = port->throttled;
+ port->throttled = port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);

- if (actually_throttled)
- schedule_delayed_work(&priv->rx_work, 0);
+ /* Resubmit urb if throttled and open. */
+ if (was_throttled && test_bit(ASYNCB_INITIALIZED, &port->port.flags))
+ ftdi_submit_read_urb(port, GFP_KERNEL);
}

static int __init ftdi_init(void)
--
1.6.4.2

2009-10-04 23:40:14

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Johan Hovold <[email protected]> writes:

>> > > > + * The per character mucking around with sysrq path it too slow, so
>> > > > + * shortcircuit it in the 99.9999999% of cases where the USB serial is
>> > > > + * not a console anyway.
>> > > > + */
>> > > > + ch = packet + 2;
>> > > > + len -= 2;
>> > > > + if (!port->console || !port->sysrq)
>> > >
>> > > You need && flag == TTY_NORMAL ?
>> >
>> > You tell me. :-) Are we interested in them unless port->console is set?
>>
>> Yes - we don;t care about the sysrq but we care about error characters
>> being reported ot the line discipline properly.
>
> Wasn't thinking.. Here's an update which always passes error characters.
> It also uses ASYNCB_INITIALIZED (instead of ASYNCB_CLOSING) in unthrottle.

To test this what should I be applying this on top of?
There have been a lot of patches flying around.

Eric

2009-10-05 07:03:10

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

On Sun, Oct 04, 2009 at 04:39:22PM -0700, Eric W. Biederman wrote:
> To test this what should I be applying this on top of?
> There have been a lot of patches flying around.

Only my initial removal of low_latency patch.

Thanks,
Johan

2009-11-17 18:35:17

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Alan Cox <[email protected]> writes:

> On Fri, 02 Oct 2009 16:00:41 -0700
> [email protected] (Eric W. Biederman) wrote:
>
>>
>>
>> I have seen a hang in:
>> /bin/stty (changing the baud rate)
>> set_termios
>> tty_wait_until_sent
>> tty_chars_in_buffer
>> ftdi_chars_in_buffer
>>
>> Where the driver wedged for a serial port and no progress
>> was made.
>>
>> This happened to me several times with 2.6.31. My initial
>> hypothesis was this was a hardware error (as it only happened
>> on single piece of hardware). With all of the driver problems
>> I suspect it could be a driver bug.
>
> Driver bug I would think - or setup. If you've genuinely got the port
> flow controlled then a request to set the termios after the I/O will wait
> until a signal or carrier change (or indeed forever) quite correctly.

I have finally tracked this one down. But I'm not certain if there is
anything we can do in software to make things better.

Boiled down. ftdi_chars_in_buffer is essentially priv->tx_outstanding_bytes.
tx_outstanding_bytes is incremented when an urb request is sent and
tx_outstanding_bytes is decremented when an urb request completes.

It turns out I have a moderately noisy usb line, and so I occasionally
get messages like:

ehci_hcd 0000:00:0a.1: detected XactErr len 0/7 retry 31

Which as best as I can tell result in the urb getting abandoned and
neither completed nor canceled (because we have hit the maximum
retry count and they still don't succeed).

Which appears to result in tx_outstanding_bytes getting stuck at
some positive number.

Is it possible to handle this more gracefully in software?
Is it possible to handle this in a way that makes it clear there
was a hardware error that we could not recover from. A little
debug level error doesn't usually even make it to the log.

Eric

2009-11-17 18:41:32

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Am Dienstag, 17. November 2009 19:35:07 schrieb Eric W. Biederman:
> Boiled down. ftdi_chars_in_buffer is essentially
> priv->tx_outstanding_bytes. tx_outstanding_bytes is incremented when an
> urb request is sent and tx_outstanding_bytes is decremented when an urb
> request completes.
>
> It turns out I have a moderately noisy usb line, and so I occasionally
> get messages like:
>
> ehci_hcd 0000:00:0a.1: detected XactErr len 0/7 retry 31
>
> Which as best as I can tell result in the urb getting abandoned and
> neither completed nor canceled (because we have hit the maximum
> retry count and they still don't succeed).

The URB should be finished with an error code in urb->status.
ftdi_write_bulk_callback() does decrement the counter even in
the error case.

> Which appears to result in tx_outstanding_bytes getting stuck at
> some positive number.

Do you see this message

if (status) {
dbg("nonzero write bulk status received: %d", status);
return;
}

if you enable debugging output?

Regards
Oliver

2009-11-17 18:56:40

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Oliver Neukum <[email protected]> writes:

> Am Dienstag, 17. November 2009 19:35:07 schrieb Eric W. Biederman:
>> Boiled down. ftdi_chars_in_buffer is essentially
>> priv->tx_outstanding_bytes. tx_outstanding_bytes is incremented when an
>> urb request is sent and tx_outstanding_bytes is decremented when an urb
>> request completes.
>>
>> It turns out I have a moderately noisy usb line, and so I occasionally
>> get messages like:
>>
>> ehci_hcd 0000:00:0a.1: detected XactErr len 0/7 retry 31
>>
>> Which as best as I can tell result in the urb getting abandoned and
>> neither completed nor canceled (because we have hit the maximum
>> retry count and they still don't succeed).
>
> The URB should be finished with an error code in urb->status.
> ftdi_write_bulk_callback() does decrement the counter even in
> the error case.
>
>> Which appears to result in tx_outstanding_bytes getting stuck at
>> some positive number.
>
> Do you see this message
>
> if (status) {
> dbg("nonzero write bulk status received: %d", status);
> return;
> }
>
> if you enable debugging output?

I will have to give that a try. The problem isn't that easy to reproduce.
But I do have a 100% correlation between urb retry counts and hangs
in tty_wait_until_sent.

Eric

2009-11-17 20:06:06

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Attachments:

(No filename) (2.68 kB)
dmesg-hang1.txt (105.74 kB)
Download all attachments

2009-11-18 01:08:52

[permalink] [raw]

Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

Attachments:

(No filename) (9.62 kB)
usb-debug.diff2 (3.13 kB)
Download all attachments

2009-11-18 03:11:01