Messages in this thread |  | | Date | Sat, 31 Aug 2002 00:40:10 +0200 | From | Vojtech Pavlik <> | Subject | Re: [PATCH] 2.5.31-serport |
| |
On Fri, Aug 30, 2002 at 10:39:11PM +0100, Russell King wrote: > 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.
I'll answer to this as soon as I get some sleep. No pun originally intended.
> 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.
Just to make sure - the scenario would be: inputattach opening /dev/ttyS0, setting N_MOUSE ldisc. Mouse driver opening that serio, etc, etc. Then I run minicom on /dev/ttyS0?
> 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.
Definitely. Not sure if that existed at the time this was written. Probably yes, though.
> 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.
Indeed.
> 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)
This should better be fixed by switching to <linux/list.h> for the linked lists.
> 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); >
-- Vojtech Pavlik SuSE Labs - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |