linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] new tty buffering locking fix
@ 2006-01-26 20:33 Paul Fulghum
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Fulghum @ 2006-01-26 20:33 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Linux Kernel Mailing List, Alan Cox

Change locking in the new tty buffering facility
from using tty->read_lock, which is currently
ignored by drivers and thus ineffective.
New locking uses a new tty buffering specific lock
enforced centrally in the tty buffering code.

Two drivers (esp and cyclades) are updated to use
the tty buffering functions instead of accessing
tty buffering internals directly. This is required
for the new locking to work.

Minor checks for NULL buffers added to
tty_prepare_flip_string/tty_prepare_flip_string_flags

Signed-off-by:Paul Fulghum <paulkf@microgate.com>

--- linux-2.6.16-rc1/drivers/char/tty_io.c	2006-01-25 13:46:06.000000000 -0600
+++ linux-2.6.16-rc1-mg/drivers/char/tty_io.c	2006-01-25 13:50:15.000000000 -0600
@@ -253,6 +253,7 @@ static void tty_buffer_free_all(struct t
 
 static void tty_buffer_init(struct tty_struct *tty)
 {
+	spin_lock_init(&tty->buf.lock);
 	tty->buf.head = NULL;
 	tty->buf.tail = NULL;
 	tty->buf.free = NULL;
@@ -266,6 +267,7 @@ static struct tty_buffer *tty_buffer_all
 	p->used = 0;
 	p->size = size;
 	p->next = NULL;
+	p->active = 0;
 	p->char_buf_ptr = (char *)(p->data);
 	p->flag_buf_ptr = (unsigned char *)p->char_buf_ptr + size;
 /* 	printk("Flip create %p\n", p); */
@@ -312,25 +314,36 @@ static struct tty_buffer *tty_buffer_fin
 
 int tty_buffer_request_room(struct tty_struct *tty, size_t size)
 {
-	struct tty_buffer *b = tty->buf.tail, *n;
-	int left = 0;
+	struct tty_buffer *b, *n;
+	int left;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->buf.lock, flags);
 
 	/* OPTIMISATION: We could keep a per tty "zero" sized buffer to
 	   remove this conditional if its worth it. This would be invisible
 	   to the callers */
-	if(b != NULL)
+	if ((b = tty->buf.tail) != NULL) {
 		left = b->size - b->used;
-	if(left >= size)
-		return size;
-	/* This is the slow path - looking for new buffers to use */
-	n = tty_buffer_find(tty, size);
-	if(n == NULL)
-		return left;
-	if(b != NULL)
-		b->next = n;
-	else
-		tty->buf.head = n;
-	tty->buf.tail = n;
+		b->active = 1;
+	} else
+		left = 0;
+
+	if (left < size) {
+		/* This is the slow path - looking for new buffers to use */
+		if ((n = tty_buffer_find(tty, size)) != NULL) {
+			if (b != NULL) {
+				b->next = n;
+				b->active = 0;
+			} else
+				tty->buf.head = n;
+			tty->buf.tail = n;
+			n->active = 1;
+		} else
+			size = left;
+	}
+
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return size;
 }
 
@@ -396,10 +409,12 @@ EXPORT_SYMBOL_GPL(tty_insert_flip_string
 int tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars, size_t size)
 {
 	int space = tty_buffer_request_room(tty, size);
-	struct tty_buffer *tb = tty->buf.tail;
-	*chars = tb->char_buf_ptr + tb->used;
-	memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
-	tb->used += space;
+	if (likely(space)) {
+		struct tty_buffer *tb = tty->buf.tail;
+		*chars = tb->char_buf_ptr + tb->used;
+		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
+		tb->used += space;
+	}
 	return space;
 }
 
@@ -416,10 +431,12 @@ EXPORT_SYMBOL_GPL(tty_prepare_flip_strin
 int tty_prepare_flip_string_flags(struct tty_struct *tty, unsigned char **chars, char **flags, size_t size)
 {
 	int space = tty_buffer_request_room(tty, size);
-	struct tty_buffer *tb = tty->buf.tail;
-	*chars = tb->char_buf_ptr + tb->used;
-	*flags = tb->flag_buf_ptr + tb->used;
-	tb->used += space;
+	if (likely(space)) {
+		struct tty_buffer *tb = tty->buf.tail;
+		*chars = tb->char_buf_ptr + tb->used;
+		*flags = tb->flag_buf_ptr + tb->used;
+		tb->used += space;
+	}
 	return space;
 }
 
@@ -2747,20 +2764,20 @@ static void flush_to_ldisc(void *private
 		schedule_delayed_work(&tty->buf.work, 1);
 		goto out;
 	}
-	spin_lock_irqsave(&tty->read_lock, flags);
-	while((tbuf = tty->buf.head) != NULL) {
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	while((tbuf = tty->buf.head) != NULL && !tbuf->active) {
 		tty->buf.head = tbuf->next;
 		if (tty->buf.head == NULL)
 			tty->buf.tail = NULL;
-		spin_unlock_irqrestore(&tty->read_lock, flags);
+		spin_unlock_irqrestore(&tty->buf.lock, flags);
 		/* printk("Process buffer %p for %d\n", tbuf, tbuf->used); */
 		disc->receive_buf(tty, tbuf->char_buf_ptr,
 				       tbuf->flag_buf_ptr,
 				       tbuf->used);
-		spin_lock_irqsave(&tty->read_lock, flags);
+		spin_lock_irqsave(&tty->buf.lock, flags);
 		tty_buffer_free(tty, tbuf);
 	}
-	spin_unlock_irqrestore(&tty->read_lock, flags);
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
 out:
 	tty_ldisc_deref(disc);
 }
@@ -2852,6 +2869,12 @@ EXPORT_SYMBOL(tty_get_baud_rate);
 
 void tty_flip_buffer_push(struct tty_struct *tty)
 {
+	unsigned long flags;
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	if (tty->buf.tail != NULL)
+		tty->buf.tail->active = 0;
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
+
 	if (tty->low_latency)
 		flush_to_ldisc((void *) tty);
 	else
--- linux-2.6.16-rc1/include/linux/tty.h	2006-01-17 09:31:30.000000000 -0600
+++ linux-2.6.16-rc1-mg/include/linux/tty.h	2006-01-24 14:12:19.000000000 -0600
@@ -57,6 +57,7 @@ struct tty_buffer {
 	unsigned char *flag_buf_ptr;
 	int used;
 	int size;
+	int active;
 	/* Data points here */
 	unsigned long data[0];
 };
@@ -64,6 +65,7 @@ struct tty_buffer {
 struct tty_bufhead {
 	struct work_struct		work;
 	struct semaphore pty_sem;
+	spinlock_t lock;
 	struct tty_buffer *head;	/* Queue head */
 	struct tty_buffer *tail;	/* Active buffer */
 	struct tty_buffer *free;	/* Free queue head */
--- linux-2.6.16-rc1/include/linux/tty_flip.h	2006-01-24 12:24:47.000000000 -0600
+++ linux-2.6.16-rc1-mg/include/linux/tty_flip.h	2006-01-24 14:46:59.000000000 -0600
@@ -17,7 +17,7 @@ _INLINE_ int tty_insert_flip_char(struct
 				   unsigned char ch, char flag)
 {
 	struct tty_buffer *tb = tty->buf.tail;
-	if (tb && tb->used < tb->size) {
+	if (tb && tb->active && tb->used < tb->size) {
 		tb->flag_buf_ptr[tb->used] = flag;
 		tb->char_buf_ptr[tb->used++] = ch;
 		return 1;
@@ -27,6 +27,11 @@ _INLINE_ int tty_insert_flip_char(struct
 
 _INLINE_ void tty_schedule_flip(struct tty_struct *tty)
 {
+	unsigned long flags;
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	if (tty->buf.tail != NULL)
+		tty->buf.tail->active = 0;
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	schedule_delayed_work(&tty->buf.work, 1);
 }
 
--- linux-2.6.16-rc1/include/linux/kbd_kern.h	2006-01-17 09:31:29.000000000 -0600
+++ linux-2.6.16-rc1-mg/include/linux/kbd_kern.h	2006-01-24 15:38:19.000000000 -0600
@@ -151,6 +151,11 @@ extern unsigned int keymap_count;
 
 static inline void con_schedule_flip(struct tty_struct *t)
 {
+	unsigned long flags;
+	spin_lock_irqsave(&t->buf.lock, flags);
+	if (t->buf.tail != NULL)
+		t->buf.tail->active = 0;
+	spin_unlock_irqrestore(&t->buf.lock, flags);
 	schedule_work(&t->buf.work);
 }
 
--- linux-2.6.16-rc1/drivers/char/esp.c	2006-01-17 09:31:19.000000000 -0600
+++ linux-2.6.16-rc1-mg/drivers/char/esp.c	2006-01-25 09:48:23.000000000 -0600
@@ -359,7 +359,7 @@ static inline void receive_chars_pio(str
 		}
 	}
 
-	schedule_delayed_work(&tty->buf.work, 1);
+	tty_schedule_flip(tty);
 
 	info->stat_flags &= ~ESP_STAT_RX_TIMEOUT;
 	release_pio_buffer(pio_buf);
@@ -426,7 +426,7 @@ static inline void receive_chars_dma_don
 			}
 			tty_insert_flip_char(tty, dma_buffer[num_bytes - 1], statflag);
 		}
-		schedule_delayed_work(&tty->buf.work, 1);
+		tty_schedule_flip(tty);
 	}
 
 	if (dma_bytes != num_bytes) {
--- linux-2.6.16-rc1/drivers/char/cyclades.c	2006-01-26 10:44:39.000000000 -0600
+++ linux-2.6.16-rc1-mg/drivers/char/cyclades.c	2006-01-26 10:43:43.000000000 -0600
@@ -1233,7 +1233,7 @@ cyy_interrupt(int irq, void *dev_id, str
                             }
                              info->idle_stats.recv_idle = jiffies;
                         }
-                        schedule_delayed_work(&tty->buf.work, 1);
+			tty_schedule_flip(tty);
                     }
                     /* end of service */
                     cy_writeb(base_addr+(CyRIR<<index), (save_xir & 0x3f));
@@ -1606,7 +1606,7 @@ cyz_handle_rx(struct cyclades_port *info
 	    }
 #endif
 	    info->idle_stats.recv_idle = jiffies;
-	    schedule_delayed_work(&tty->buf.work, 1);
+	    tty_schedule_flip(tty);
 	}
 	/* Update rx_get */
 	cy_writel(&buf_ctrl->rx_get, new_rx_get);
@@ -1809,7 +1809,7 @@ cyz_handle_cmd(struct cyclades_card *cin
 	if(delta_count)
 	    cy_sched_event(info, Cy_EVENT_DELTA_WAKEUP);
 	if(special_count)
-	    schedule_delayed_work(&tty->buf.work, 1);
+	    tty_schedule_flip(tty);
     }
 }
 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-08 22:05           ` Paul Fulghum
@ 2006-02-08 22:39             ` Olaf Hering
  0 siblings, 0 replies; 11+ messages in thread
From: Olaf Hering @ 2006-02-08 22:39 UTC (permalink / raw)
  To: Paul Fulghum; +Cc: Alan Cox, linux-kernel

 On Wed, Feb 08, Paul Fulghum wrote:

> Olaf: can you please test this with the hvc?

Yes, works for me, tested with -git6

-- 
short story of a lazy sysadmin:
 alias appserv=wotan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-08  1:14         ` Alan Cox
  2006-02-08  2:41           ` Paul Fulghum
@ 2006-02-08 22:05           ` Paul Fulghum
  2006-02-08 22:39             ` Olaf Hering
  1 sibling, 1 reply; 11+ messages in thread
From: Paul Fulghum @ 2006-02-08 22:05 UTC (permalink / raw)
  To: Alan Cox; +Cc: Olaf Hering, linux-kernel

Here is a different approach to prevent a driver
from stalling pending receive data in the flip buffers
by calling a tty buffer allocation function without
a trailing call to a tty schedule function.
This patch does not have the memory penalty of
my previous patch for the worst case usage patterns.

Alan: let me know what you think.
Olaf: can you please test this with the hvc?

I am going to continue testing tomorrow,
but would like some early feedback.

Thanks,
Paul

--- linux-2.6.16-rc2/include/linux/kbd_kern.h	2006-02-08 12:15:15.000000000 -0600
+++ b/include/linux/kbd_kern.h	2006-02-08 12:15:39.000000000 -0600
@@ -153,8 +153,10 @@ static inline void con_schedule_flip(str
 {
 	unsigned long flags;
 	spin_lock_irqsave(&t->buf.lock, flags);
-	if (t->buf.tail != NULL)
+	if (t->buf.tail != NULL) {
 		t->buf.tail->active = 0;
+		t->buf.tail->commit = t->buf.tail->used;
+	}
 	spin_unlock_irqrestore(&t->buf.lock, flags);
 	schedule_work(&t->buf.work);
 }
--- linux-2.6.16-rc2/include/linux/tty_flip.h	2006-02-07 14:14:36.000000000 -0600
+++ b/include/linux/tty_flip.h	2006-02-08 09:46:58.000000000 -0600
@@ -29,8 +29,10 @@ _INLINE_ void tty_schedule_flip(struct t
 {
 	unsigned long flags;
 	spin_lock_irqsave(&tty->buf.lock, flags);
-	if (tty->buf.tail != NULL)
+	if (tty->buf.tail != NULL) {
 		tty->buf.tail->active = 0;
+		tty->buf.tail->commit = tty->buf.tail->used;
+	}
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	schedule_delayed_work(&tty->buf.work, 1);
 }
--- linux-2.6.16-rc2/include/linux/tty.h	2006-02-07 14:14:36.000000000 -0600
+++ b/include/linux/tty.h	2006-02-08 09:45:23.000000000 -0600
@@ -58,6 +58,8 @@ struct tty_buffer {
 	int used;
 	int size;
 	int active;
+	int commit;
+	int read;
 	/* Data points here */
 	unsigned long data[0];
 };
--- linux-2.6.16-rc2/drivers/char/tty_io.c	2006-02-08 12:55:03.000000000 -0600
+++ b/drivers/char/tty_io.c	2006-02-08 13:40:58.000000000 -0600
@@ -268,6 +268,8 @@ static struct tty_buffer *tty_buffer_all
 	p->size = size;
 	p->next = NULL;
 	p->active = 0;
+	p->commit = 0;
+	p->read = 0;
 	p->char_buf_ptr = (char *)(p->data);
 	p->flag_buf_ptr = (unsigned char *)p->char_buf_ptr + size;
 /* 	printk("Flip create %p\n", p); */
@@ -298,6 +300,8 @@ static struct tty_buffer *tty_buffer_fin
 			*tbh = t->next;
 			t->next = NULL;
 			t->used = 0;
+			t->commit = 0;
+			t->read = 0;
 			/* DEBUG ONLY */
 			memset(t->data, '*', size);
 /* 			printk("Flip recycle %p\n", t); */
@@ -335,6 +339,7 @@ int tty_buffer_request_room(struct tty_s
 			if (b != NULL) {
 				b->next = n;
 				b->active = 0;
+				b->commit = b->used;
 			} else
 				tty->buf.head = n;
 			tty->buf.tail = n;
@@ -2752,6 +2757,9 @@ static void flush_to_ldisc(void *private
 	unsigned long 	flags;
 	struct tty_ldisc *disc;
 	struct tty_buffer *tbuf;
+	int count;
+	char *char_buf;
+	unsigned char *flag_buf;
 
 	disc = tty_ldisc_ref(tty);
 	if (disc == NULL)	/*  !TTY_LDISC */
@@ -2765,16 +2773,20 @@ static void flush_to_ldisc(void *private
 		goto out;
 	}
 	spin_lock_irqsave(&tty->buf.lock, flags);
-	while((tbuf = tty->buf.head) != NULL && !tbuf->active) {
+	while((tbuf = tty->buf.head) != NULL) {
+		while ((count = tbuf->commit - tbuf->read) != 0) {
+			char_buf = tbuf->char_buf_ptr + tbuf->read;
+			flag_buf = tbuf->flag_buf_ptr + tbuf->read;
+			tbuf->read += count;
+			spin_unlock_irqrestore(&tty->buf.lock, flags);
+			disc->receive_buf(tty, char_buf, flag_buf, count);
+			spin_lock_irqsave(&tty->buf.lock, flags);
+		}
+		if (tbuf->active)
+			break;
 		tty->buf.head = tbuf->next;
 		if (tty->buf.head == NULL)
 			tty->buf.tail = NULL;
-		spin_unlock_irqrestore(&tty->buf.lock, flags);
-		/* printk("Process buffer %p for %d\n", tbuf, tbuf->used); */
-		disc->receive_buf(tty, tbuf->char_buf_ptr,
-				       tbuf->flag_buf_ptr,
-				       tbuf->used);
-		spin_lock_irqsave(&tty->buf.lock, flags);
 		tty_buffer_free(tty, tbuf);
 	}
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
@@ -2871,8 +2883,10 @@ void tty_flip_buffer_push(struct tty_str
 {
 	unsigned long flags;
 	spin_lock_irqsave(&tty->buf.lock, flags);
-	if (tty->buf.tail != NULL)
+	if (tty->buf.tail != NULL) {
 		tty->buf.tail->active = 0;
+		tty->buf.tail->commit = tty->buf.tail->used;
+	}
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
 
 	if (tty->low_latency)



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-08  2:41           ` Paul Fulghum
@ 2006-02-08  2:56             ` Paul Fulghum
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Fulghum @ 2006-02-08  2:56 UTC (permalink / raw)
  Cc: Alan Cox, Olaf Hering, linux-kernel

Paul Fulghum wrote:
> Alan Cox wrote:
> 
>> Thats going to hurt memory consumption in the worst cases.
> 
> I'll gather some accounting info tomorrow,
> and consider the more pathological cases.

Now that I think about it (ow!), there is a simple way
to cap the memory penalty to a single extra buffer allocation
and still maintain the existing interface behavior.

I'll post a new patch tomorrow.

Thanks,
Paul

--
Paul Fulghum
Microgate Systems, Ltd

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-08  1:14         ` Alan Cox
@ 2006-02-08  2:41           ` Paul Fulghum
  2006-02-08  2:56             ` Paul Fulghum
  2006-02-08 22:05           ` Paul Fulghum
  1 sibling, 1 reply; 11+ messages in thread
From: Paul Fulghum @ 2006-02-08  2:41 UTC (permalink / raw)
  To: Alan Cox; +Cc: Olaf Hering, linux-kernel

Alan Cox wrote:
> Thats going to hurt memory consumption in the worst cases.

I'll gather some accounting info tomorrow,
and consider the more pathological cases.

 > Might be better to document the sane behaviour and enforce it ?

That was my first thought, which prompted my
initial patch of the hvc drivers.

Requiring a call to schedule processing of tty data
after preallocating buffer space is not obvious,
and could result in scheduling work when there is no
data to process.

This semantic twist introduced by my first locking patch
is a hazard for driver writers, and would require an audit
of existing drivers (as demonstrated by Olaf's report).

--
Paul Fulghum
Microgate Systems, Ltd


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-07 21:27       ` Paul Fulghum
@ 2006-02-08  1:14         ` Alan Cox
  2006-02-08  2:41           ` Paul Fulghum
  2006-02-08 22:05           ` Paul Fulghum
  0 siblings, 2 replies; 11+ messages in thread
From: Alan Cox @ 2006-02-08  1:14 UTC (permalink / raw)
  To: Paul Fulghum; +Cc: Olaf Hering, linux-kernel

On Maw, 2006-02-07 at 15:27 -0600, Paul Fulghum wrote:
> Olaf:
> 
> Try this patch and let me know how it works.
> (it is against 2.6.16-rc2-git1)
> 
> This change prevents claiming a partially filled
> buffer if it has already been committed by the
> driver calling a tty scheduling function, but not
> yet processed by the tty layer.

Thats going to hurt memory consumption in the worst cases. Might be
better to document the sane behaviour and enforce it ?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-07 17:11     ` Olaf Hering
  2006-02-07 18:47       ` Paul Fulghum
@ 2006-02-07 21:27       ` Paul Fulghum
  2006-02-08  1:14         ` Alan Cox
  1 sibling, 1 reply; 11+ messages in thread
From: Paul Fulghum @ 2006-02-07 21:27 UTC (permalink / raw)
  To: Olaf Hering; +Cc: Alan Cox, linux-kernel

Olaf:

Try this patch and let me know how it works.
(it is against 2.6.16-rc2-git1)

This change prevents claiming a partially filled
buffer if it has already been committed by the
driver calling a tty scheduling function, but not
yet processed by the tty layer.

Individual drivers can no longer stall committed
data by calling any of the tty buffering functions.
(example: calling tty_buffer_request_room
and not immediately adding receive data with an
associated tty scheduling call)

This remains relatively efficient. 
1. partially filled active buffer can be reused
2. already allocated empty buffer can be reused

Thanks,
Paul


--- linux-2.6.16-rc2/drivers/char/tty_io.c	2006-02-07 13:18:40.000000000 -0600
+++ b/drivers/char/tty_io.c	2006-02-07 13:46:28.000000000 -0600
@@ -324,8 +324,14 @@ int tty_buffer_request_room(struct tty_s
 	   remove this conditional if its worth it. This would be invisible
 	   to the callers */
 	if ((b = tty->buf.tail) != NULL) {
-		left = b->size - b->used;
-		b->active = 1;
+		if (!b->active) {
+			if (!b->used) {
+				left = b->size;
+				b->active = 1;
+			} else
+				left = 0;
+		} else
+			left = b->size - b->used;
 	} else
 		left = 0;
 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-07 17:11     ` Olaf Hering
@ 2006-02-07 18:47       ` Paul Fulghum
  2006-02-07 21:27       ` Paul Fulghum
  1 sibling, 0 replies; 11+ messages in thread
From: Paul Fulghum @ 2006-02-07 18:47 UTC (permalink / raw)
  To: Olaf Hering; +Cc: Alan Cox, linux-kernel

On Tue, 2006-02-07 at 18:11 +0100, Olaf Hering wrote:
>  On Tue, Feb 07, Paul Fulghum wrote:
> 
> > Try the below patches (for testing only, I'm not suggesting
> > these as a final fix yet) and let me know if it fixes it.
> 
> Yes, this works for me.

OK, good.

I'm working on a change to the locking patch that
fixes this without modifying individual drivers
such as hvc_console. I'll post something later today.

-- 
Paul Fulghum
Microgate Systems, Ltd


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-07 16:21   ` Paul Fulghum
@ 2006-02-07 17:11     ` Olaf Hering
  2006-02-07 18:47       ` Paul Fulghum
  2006-02-07 21:27       ` Paul Fulghum
  0 siblings, 2 replies; 11+ messages in thread
From: Olaf Hering @ 2006-02-07 17:11 UTC (permalink / raw)
  To: Paul Fulghum; +Cc: Alan Cox, linux-kernel

 On Tue, Feb 07, Paul Fulghum wrote:

> Try the below patches (for testing only, I'm not suggesting
> these as a final fix yet) and let me know if it fixes it.

Yes, this works for me.

-- 
short story of a lazy sysadmin:
 alias appserv=wotan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
  2006-02-07 12:34 ` Olaf Hering
@ 2006-02-07 16:21   ` Paul Fulghum
  2006-02-07 17:11     ` Olaf Hering
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Fulghum @ 2006-02-07 16:21 UTC (permalink / raw)
  To: Olaf Hering; +Cc: Alan Cox, linux-kernel

On Tue, 2006-02-07 at 13:34 +0100, Olaf Hering wrote:
> > [PATCH] new tty buffering locking fix

> This patch breaks the hvc console, no input is accepted, even with
> init=/bin/sash? Any idea what this driver needs to do now?
> I wonder if it worked ok on -mm.

I think this is happening:

1. hvc calls tty_buffer_request_room, claiming a
   tty buffer for use by the driver

2. hvc adds data to the tty buffer

3. hvc calls tty_schedule_flip or tty_flip_buffer_push,
   which releases the buffer for processing

4. *before* the buffer can be processed, hvc again calls
   tty_buffer_request_room, reclaiming the same buffer
   which still has free room

5. hvc does not have more data to input, so it does
   not call tty_schedule_flip or tty_flip_buffer_push

Now the tty buffer contains some data but can't be processed
because it is still marked as in use by the driver.

Try the below patches (for testing only, I'm not suggesting
these as a final fix yet) and let me know if it fixes it.

Thanks,
Paul


--- linux-2.6.16-rc2/drivers/char/hvc_console.c	2006-02-06 13:50:39.000000000 -0600
+++ b/drivers/char/hvc_console.c	2006-02-07 10:10:18.000000000 -0600
@@ -613,6 +613,7 @@ static int hvc_poll(struct hvc_struct *h
 				tty_hangup(tty);
 				spin_lock_irqsave(&hp->lock, flags);
 			}
+			tty_schedule_flip(tty);
 			break;
 		}
 		for (i = 0; i < n; ++i) {
@@ -633,8 +634,7 @@ static int hvc_poll(struct hvc_struct *h
 			tty_insert_flip_char(tty, buf[i], 0);
 		}
 
-		if (count)
-			tty_schedule_flip(tty);
+		tty_schedule_flip(tty);
 
 		/*
 		 * Account for the total amount read in one loop, and if above
--- linux-2.6.16-rc2/drivers/char/hvcs.c	2006-02-06 13:50:39.000000000 -0600
+++ b/drivers/char/hvcs.c	2006-02-07 10:09:57.000000000 -0600
@@ -469,8 +469,7 @@ static int hvcs_io(struct hvcs_struct *h
 
 	spin_unlock_irqrestore(&hvcsd->lock, flags);
 	/* This is synch because tty->low_latency == 1 */
-	if(got)
-		tty_flip_buffer_push(tty);
+	tty_flip_buffer_push(tty);
 
 	if (!got) {
 		/* Do this _after_ the flip_buffer_push */



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] new tty buffering locking fix
       [not found] <200602032312.k13NCbWb012991@hera.kernel.org>
@ 2006-02-07 12:34 ` Olaf Hering
  2006-02-07 16:21   ` Paul Fulghum
  0 siblings, 1 reply; 11+ messages in thread
From: Olaf Hering @ 2006-02-07 12:34 UTC (permalink / raw)
  To: Paul Fulghum, Alan Cox; +Cc: linux-kernel

 On Fri, Feb 03, Linux Kernel Mailing List wrote:

> [PATCH] new tty buffering locking fix
> 
> Change locking in the new tty buffering facility from using tty->read_lock,
> which is currently ignored by drivers and thus ineffective.  New locking
> uses a new tty buffering specific lock enforced centrally in the tty
> buffering code.
> 
> Two drivers (esp and cyclades) are updated to use the tty buffering
> functions instead of accessing tty buffering internals directly.  This is
> required for the new locking to work.
> 
> Minor checks for NULL buffers added to
> tty_prepare_flip_string/tty_prepare_flip_string_flags
> 
> Signed-off-by: Paul Fulghum <paulkf@microgate.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> 
>  drivers/char/cyclades.c  |    6 +--
>  drivers/char/esp.c       |    4 +-
>  drivers/char/tty_io.c    |   77 ++++++++++++++++++++++++++++++-----------------
>  include/linux/kbd_kern.h |    5 +++
>  include/linux/tty.h      |    2 +
>  include/linux/tty_flip.h |    7 +++-
>  6 files changed, 68 insertions(+), 33 deletions(-)

This patch breaks the hvc console, no input is accepted, even with
init=/bin/sash? Any idea what this driver needs to do now?
I wonder if it worked ok on -mm.

-- 
short story of a lazy sysadmin:
 alias appserv=wotan

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2006-02-08 22:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-01-26 20:33 [PATCH] new tty buffering locking fix Paul Fulghum
     [not found] <200602032312.k13NCbWb012991@hera.kernel.org>
2006-02-07 12:34 ` Olaf Hering
2006-02-07 16:21   ` Paul Fulghum
2006-02-07 17:11     ` Olaf Hering
2006-02-07 18:47       ` Paul Fulghum
2006-02-07 21:27       ` Paul Fulghum
2006-02-08  1:14         ` Alan Cox
2006-02-08  2:41           ` Paul Fulghum
2006-02-08  2:56             ` Paul Fulghum
2006-02-08 22:05           ` Paul Fulghum
2006-02-08 22:39             ` Olaf Hering

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