Weird Mouse Behaviour with 2.6

Not sure if this is related, but I've been tracking down serial (console) issues on an E250 with 2.6.12rc1, which uses sunsab as the console.

I can't claim I fully understand the new serial/console code yet, but the following seems broken:

In tty_ioctl.c tty_wait_until_sent() if the timeout variable is set to zero (which many callers do explicitly) it gets reassigned to MAX_SCHEDULE_TIMEOUT (this is LONG_MAX). If there are no characters waiting to be sent (!tty->driver->chars_in_buffer(tty)) we drop out of our loop and supply the timeout variable to uart_wait_until_sent (tty->driver->wait_until_sent). The problem is uart_wait_until_sent is specified with a timeout argument of type int, not long. This becomes -1 in uart_wait_until_sent, which I don't think is intended. If your port->timeout value in this function is also zero (as seems the case with sunsab), this seems doubly bad and leads to massive mdelay times in uart_wait_until_sent. This will appear to you as a hung getty.

My current workaround, until i understand the lay of the land a bit better, is the following. I'd wager the better bet is to modify uart_wait_until_sent to take a "long" (or unsigned long) argument for timeout rather than an "int" arg. This works on x86 only as a side-effect.

=== tty_ioctl.c 1.19 vs edited ==--- 1.19/drivers/char/tty_ioctl.c 2005-01-10 17:29:36 -08:00
+++ edited/tty_ioctl.c 2005-04-01 16:14:31 -08:00 @@ -58,8 +58,10 @@

                set_current_state(TASK_INTERRUPTIBLE);
                if (signal_pending(current))
                        goto stop_waiting;
-               if (!tty->driver->chars_in_buffer(tty))
+               if (!tty->driver->chars_in_buffer(tty)) {
+                       timeout = 0;
                        break;
+               }
                timeout = schedule_timeout(timeout);
        } while (timeout);
        if (tty->driver->wait_until_sent)

To complicate things, my GCC [gcc (GCC) 3.3.5 (Debian 1:3.3.5-5)] doesn't seem to properly initialize char_time to 0 with the syntax "unsigned long char_time, expire;" in uart_wait_until_sent() or something is getting trashed (I've not investigated that much yet).

Like I said, I don't quite understand the new serial subsystem yet, but it seems like a lot of side-effects are in play.

E

Good catch. That third argument should be "unsigned long timeout" indeed. I've pointed this out in private email to Russell, Linus and Andrew.

uart_update_timeout() calls done by the driver (in this case sunsab.c) should be updating the port->timeout value properly. Indeed, sunsab.c fails to call uart_update_timeout() at all.

Hmmm, I wonder if the port->uartclk we use in sunsab.c needs to be multiplied by 16, like sunsu.c does.

Anyways, this patch below should be a step in the right direction.

=== drivers/serial/sunsab.c 1.38 vs edited ==--- 1.38/drivers/serial/sunsab.c 2004-12-08 21:31:16 -08:00
+++ edited/drivers/serial/sunsab.c 2005-04-04 12:17:25 -07:00
@@ -682,7 +682,8 @@

/* Internal routine, port->lock is held and local interrupts are disabled. */ static void sunsab_convert_to_sab(struct uart_sunsab_port *up, unsigned int cflag,

-				  unsigned int iflag, int baud)
+				  unsigned int iflag, unsigned int baud,
+				  unsigned int quot)

{
unsigned int ebrg;
unsigned char dafo;
@@ -766,6 +767,9 @@
up->port.ignore_status_mask |= (SAB82532_ISR0_RPF |
SAB82532_ISR0_TCD);

+ uart_update_timeout(&up->port, cflag,
+ (up->port.uartclk / (16 * quot)));
+
/* Now bang the new settings into the chip. */ sunsab_cec_wait(up);
sunsab_tec_wait(up);
@@ -784,10 +788,11 @@
{
struct uart_sunsab_port *up = (struct uart_sunsab_port *) port; unsigned long flags;
- int baud = uart_get_baud_rate(port, termios, old, 0, 4000000);
+ unsigned int baud = uart_get_baud_rate(port, termios, old, 0, 4000000);
+ unsigned int quot = uart_get_divisor(port, baud);

spin_lock_irqsave(&up->port.lock, flags);
- sunsab_convert_to_sab(up, termios->c_cflag, termios->c_iflag, baud);
+ sunsab_convert_to_sab(up, termios->c_cflag, termios->c_iflag, baud, quot);
spin_unlock_irqrestore(&up->port.lock, flags);
}

@@ -880,7 +885,7 @@
{
struct uart_sunsab_port *up = &sunsab_ports[con->index]; unsigned long flags;
- int baud;
+ unsigned int baud, quot;

printk("Console: ttyS%d (SAB82532)\n",
(sunsab_reg.minor - 64) + con->index);
@@ -926,7 +931,8 @@
SAB82532_IMR1_XPR;
writeb(up->interrupt_mask1, &up->regs->w.imr1);

- sunsab_convert_to_sab(up, con->cflag, 0, baud);
+ quot = uart_get_divisor(&up->port, baud);
+ sunsab_convert_to_sab(up, con->cflag, 0, baud, quot);
sunsab_set_mctrl(&up->port, TIOCM_DTR | TIOCM_RTS);

spin_unlock_irqrestore(&up->port.lock, flags);

I'll give your patch a shot in a little while-- I was playing around with uart_update_timeout in sunsab.c as well. It seems to me that serial_core.c::uart_wait_until_sent() should assert if port->timeout is 0 or bracket the following math

      char_time = (port->timeout - HZ/50) / port->fifosize;

otherwise we overflow. Since I'm not too familiar with the assumptions of this API, I don't know if an assert (BUG) or a check (perhaps "if (port->timeout >= HZ/50)") is appropriate.

E

I definitely agree it should BUG() on this. We would have caught this problem a long time ago if it did.