| Submitter | Milton Miller |
|---|---|
| Date | Jan. 8, 2009, 12:14 p.m. |
| Message ID | <hvc-console-29-3-1@bga.com> |
| Download | mbox | patch |
| Permalink | /patch/17342/ |
| State | Accepted |
| Commit | da9dc13289fa58dced12f2baff51dfb87c339ba3 |
| Headers | show |
Comments
> v2.6.16-rc1 via 33f0f88f1c51ae5c2d593d26960c760ea154c2e2 > [PATCH] TTY layer buffering revamp > > added this new api. No - tty_flip_buffer_push is from 2.1.66 and with the same constraints from the day it was added. > Having the flag set for purely polled drivers will save delaying > the work when receiving input for 1 jiffie. > > > Index: work.git/drivers/char/hvc_console.c > =================================================================== > --- work.git.orig/drivers/char/hvc_console.c 2009-01-08 03:01:24.000000000 -0600 > +++ work.git/drivers/char/hvc_console.c 2009-01-08 03:01:51.000000000 -0600 > @@ -318,7 +318,8 @@ static int hvc_open(struct tty_struct *t > } /* else count == 0 */ > > tty->driver_data = hp; > - tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */ > + if (!hp->irq_requested) > + tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */ > > hp->tty = tty; Looks good to me
On Jan 8, 2009, at 6:36 AM, Alan Cox wrote: >> v2.6.16-rc1 via 33f0f88f1c51ae5c2d593d26960c760ea154c2e2 >> [PATCH] TTY layer buffering revamp >> >> added this new api. > > No - tty_flip_buffer_push is from 2.1.66 and with the same constraints > from the day it was added. > Yes but wrappers were added and this this and many ohter drivers were converted to use them: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git; a=commitdiff;h=33f0f88f1c51ae5c2d593d26960c760ea154c2e2 And they were slightly buggy leading to the buggy workaround. > >> Having the flag set for purely polled drivers will save delaying >> the work when receiving input for 1 jiffie. >> >> >> Index: work.git/drivers/char/hvc_console.c >> =================================================================== >> --- work.git.orig/drivers/char/hvc_console.c 2009-01-08 >> 03:01:24.000000000 -0600 >> +++ work.git/drivers/char/hvc_console.c 2009-01-08 03:01:51.000000000 >> -0600 >> @@ -318,7 +318,8 @@ static int hvc_open(struct tty_struct *t >> } /* else count == 0 */ >> >> tty->driver_data = hp; >> - tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */ >> + if (!hp->irq_requested) >> + tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */ >> >> hp->tty = tty; > > Looks good to me Thanks, I guess that is an Ack? milton
Patch
Index: work.git/drivers/char/hvc_console.c =================================================================== --- work.git.orig/drivers/char/hvc_console.c 2009-01-08 03:01:24.000000000 -0600 +++ work.git/drivers/char/hvc_console.c 2009-01-08 03:01:51.000000000 -0600 @@ -318,7 +318,8 @@ static int hvc_open(struct tty_struct *t } /* else count == 0 */ tty->driver_data = hp; - tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */ + if (!hp->irq_requested) + tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */ hp->tty = tty;
hvc_console is setting low_latency unconditionally, but some clients are interrupt driven and will call hvc_poll from irq context. This will cause tty_flip_buffer_push to be called from irq context, and it very clearly states it must not be called from IRQ when low_latency is specified. Looking back through history: v2.6.16-rc1 via 33f0f88f1c51ae5c2d593d26960c760ea154c2e2 [PATCH] TTY layer buffering revamp added this new api. v2.6.16-rc3 via 8977d929e49021d9a6e031310aab01fa72f849c2 [PATCH] tty buffering stall fix claims to fix a stall discovered with hvc_console v2.6.16-rc5 via fb5c594c2acc441f0d2d8f457484a0e0e9285db3 [PATCH] Fix race condition in hvc console. said set this flag to avoid a stall problem, and was merged through the powerpc arch tree. Without searching for email discussions, it would appear to be an overlapping "fix", but one that did not consider all users. --- This version continues to set low_latency if irqs are not flagged to be in use, as requested by paulus. Do all hvc drivers identify the interrupt this way? (A quick look at hvc_iucv says they lock to bh and are not irq driven, the rest would have used the irq before that patch). Having the flag set for purely polled drivers will save delaying the work when receiving input for 1 jiffie.