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);
-
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/
This archive was generated by hypermail 2b29 : Sat Aug 31 2002 - 22:00:31 EST