[PATCH] 2.5.31-serport

* [PATCH] 2.5.31-serport
@ 2002-08-30 21:39 Russell King
  2002-08-30 22:03 ` David S. Miller
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Russell King @ 2002-08-30 21:39 UTC (permalink / raw)
  To: Vojtech Pavlik, James Simmons; +Cc: LKML

This patch appears not to be in 2.5.32, but applies cleanly.

serport/serio presently has several problems.  This is by no means a
definitive list (for example, the locking problems that hch reported
back in 2.4.0-test time):

1. Insomnia.  When someone closes the serport device, it wakes up a
   thread in serport_ldisc_read().  Unfortunately, if serport->serio.type
   is non-zero and we haven't received a signal, we re-call schedule()
   without changing current->state.  So schedule returns, and repeat
   for ever.
 
   A better sleep solution would be:
   
        wait_event_interruptible((&serport->wait, serport->serio.type); 

   which contains all the knoweldge of handling sleeping.

   Someone might also think about what serport_serio_close() is actually
   trying to do.  If its intended to cause the thread in ldisc_read exit,
   it should set serport->serio.type to 0.

2. What happens if I open and try to read from this port while something
   has the serport_ldisc attached?  I suspect that you'll create nice
   loop of serio devices in serio.c and an infinite loop when you try to
   traverse the list to remove a different serio device.
   
3. Naming:

#ifdef CONFIG_DEVFS_FS
        sprintf(name, tty->driver.name, minor(tty->device) - tty->driver.minor_start);
#else
        sprintf(name, "%s%d", tty->driver.name, minor(tty->device) - tty->driver.minor_start);
#endif
   
   should probably be using tty_name(), rather than trying to construct
   the name itself.

4. Stack overflows.  There seems to be confusion about how long a tty name
   can be, and we seem to add characters to the name ("/serio0") and then
   squash it into a smaller buffer.

        char ttyname[64];
...     
        strcpy(ttyname, tty->driver.name);
...     
        sprintf(serport->phys, "%s%d/serio0", ttyname, minor(tty->device) - tty$   
   However, serport->phys is declared as:

struct serport {
        char phys[32];
};
   
   I'd suggest using strncpy() and snprintf() here, and adjusting the size of
   those strings so they're sensible.

This patch fixes (1) and (3) completey.  (4) needs the sizes reviewed and fixed.
(2) is NOT fixed by this patch.

 drivers/input/serio/serio.c   |    8 ++++++++
 drivers/input/serio/serport.c |   26 ++++++++------------------
 2 files changed, 16 insertions, 18 deletions

diff -ur orig/drivers/input/serio/serio.c linux/drivers/input/serio/serio.c

--- orig/drivers/input/serio/serio.c	Fri Aug  2 21:13:28 2002
+++ linux/drivers/input/serio/serio.c	Sun Aug 11 16:15:00 2002
@@ -122,6 +122,8 @@
 
 void serio_register_port(struct serio *serio)
 {
+	BUG_ON(serio->next);
+
 	serio->next = serio_list;	
 	serio_list = serio;
 	serio_find_dev(serio);
@@ -134,6 +136,8 @@
         while (*serioptr && (*serioptr != serio)) serioptr = &((*serioptr)->next);
         *serioptr = (*serioptr)->next;
 
+	serio->next = NULL;
+
 	if (serio->dev && serio->dev->disconnect)
 		serio->dev->disconnect(serio);
 }
@@ -142,6 +146,8 @@
 {
 	struct serio *serio = serio_list;
 
+	BUG_ON(dev->next);
+
 	dev->next = serio_dev;	
 	serio_dev = dev;
 
@@ -159,6 +165,8 @@
 
         while (*devptr && (*devptr != dev)) devptr = &((*devptr)->next);
         *devptr = (*devptr)->next;
+
+	dev->next = NULL;
 
 	while (serio) {
 		if (serio->dev == dev && dev->disconnect)
diff -ur orig/drivers/input/serio/serport.c linux/drivers/input/serio/serport.c
--- orig/drivers/input/serio/serport.c	Sat Jul 27 13:55:18 2002
+++ linux/drivers/input/serio/serport.c	Sun Aug 11 16:33:59 2002
@@ -96,11 +96,14 @@
 	serport->tty = tty;
 	tty->disc_data = serport;
 
-	strcpy(ttyname, tty->driver.name);
-	for (i = 0; ttyname[i] != 0 && ttyname[i] != '/'; i++);
+	strncpy(ttyname, tty->driver.name, sizeof(ttyname) - 1);
+
+	for (i = 0; ttyname[i] != 0 && ttyname[i] != '/' &&
+		    i < sizeof(ttyname) - 1; i++);
 	ttyname[i] = 0;
 
-	sprintf(serport->phys, "%s%d/serio0", ttyname, minor(tty->device) - tty->driver.minor_start);
+	snprintf(serport->phys, sizeof(serport->phys), "%s%d/serio0",
+		ttyname, minor(tty->device) - tty->driver.minor_start);
 
 	serport->serio.name = serport_name;
 	serport->serio.phys = serport->phys;
@@ -161,26 +164,13 @@
 static ssize_t serport_ldisc_read(struct tty_struct * tty, struct file * file, unsigned char * buf, size_t nr)
 {
 	struct serport *serport = (struct serport*) tty->disc_data;
-	DECLARE_WAITQUEUE(wait, current);
 	char name[32];
 
-#ifdef CONFIG_DEVFS_FS
-	sprintf(name, tty->driver.name, minor(tty->device) - tty->driver.minor_start);
-#else
-	sprintf(name, "%s%d", tty->driver.name, minor(tty->device) - tty->driver.minor_start);
-#endif
-
 	serio_register_port(&serport->serio);
 
-	printk(KERN_INFO "serio: Serial port %s\n", name);
-
-	add_wait_queue(&serport->wait, &wait);
-	set_current_state(TASK_INTERRUPTIBLE);
-
-	while(serport->serio.type && !signal_pending(current)) schedule();
+	printk(KERN_INFO "serio: Serial port %s\n", tty_name(tty, name));
 
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(&serport->wait, &wait);
+	wait_event_interruptible(serport->wait, serport->serio.type != 0);
 
 	serio_unregister_port(&serport->serio);
 

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