Re: Weird Mouse Behaviour with 2.6

Re: Weird Mouse Behaviour with 2.6

[Eric Brower]

On Apr 3, 2005 12:32 PM, Tomas Cernaj <tcernaj@gmx.de> wrote:
> Am Sonntag, den 03.04.2005, 12:09 +0200 schrieb Tomas Cernaj:
> > David S. Miller <davem@davemloft.net> wrote:
> >
> > [... mouse problem on ultras ...]
> >
> > > Yes, there is some problem initializing the serial line that the
> > > keyboard and mouse use.  The break characters that get detected
> > > when the baud rate is incorrect are not showing up for some reason.
> > >
> > > What kind of machine is this?  It only happens with certain serial
> > > controllers, not all, which is why your machine type is important.

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

Re: Weird Mouse Behaviour with 2.6

[David S. Miller]

On Mon, 4 Apr 2005 11:11:01 -0700
Eric Brower <ebrower@gmail.com> wrote:

> 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.

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);

Re: Weird Mouse Behaviour with 2.6

[Eric Brower]

On Apr 4, 2005 12:21 PM, David S. Miller <davem@davemloft.net> wrote:
> On Mon, 4 Apr 2005 11:11:01 -0700
> Eric Brower <ebrower@gmail.com> wrote:
> 
> > 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.
> 
> 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.

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

Re: Weird Mouse Behaviour with 2.6

[David S. Miller]

On Mon, 4 Apr 2005 13:22:37 -0700
Eric Brower <ebrower@gmail.com> wrote:

> It seems to me that serial_core.c::uart_wait_until_sent() should
> assert if port->timeout is 0

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

Re: Weird Mouse Behaviour with 2.6

[David S. Miller]

Ok, let's kill this bug already :-)

We were neglecting to invoke sunsu_change_speed() when first
setting up the keyboard and mouse ports.  That's why things
were not working.

This should fix it and I'll push this upsteam.

[SPARC64]: In sunsu driver, make sure to fully init chip for kbd/ms

We were forgetting to call sunsu_change_speed().  The reason
that replugging in the mouse cable "fixes things" is that
causes a BREAK interrupt which in turn caused a call to
sunsu_change_speed() which would get the chip setup properly.

Signed-off-by: David S. Miller <davem@davemloft.net>

--- a/drivers/serial/sunsu.c	2005-04-20 10:17:40.000000000 -0700
+++ b/drivers/serial/sunsu.c	2005-04-21 22:01:41.000000000 -0700
@@ -1285,6 +1285,7 @@
 
 static int __init sunsu_kbd_ms_init(struct uart_sunsu_port *up, int channel)
 {
+	int quot, baud;
 #ifdef CONFIG_SERIO
 	struct serio *serio;
 #endif
@@ -1293,10 +1294,14 @@
 	up->port.type = PORT_UNKNOWN;
 	up->port.uartclk = (SU_BASE_BAUD * 16);
 
-	if (up->su_type = SU_PORT_KBD)
+	if (up->su_type = SU_PORT_KBD) {
 		up->cflag = B1200 | CS8 | CLOCAL | CREAD;
-	else
+		baud = 1200;
+	} else {
 		up->cflag = B4800 | CS8 | CLOCAL | CREAD;
+		baud = 4800;
+	}
+	quot = up->port.uartclk / (16 * baud);
 
 	sunsu_autoconfig(up);
 	if (up->port.type = PORT_UNKNOWN)
@@ -1337,6 +1342,8 @@
 	}
 #endif
 
+	sunsu_change_speed(&up->port, up->cflag, 0, quot);
+
 	sunsu_startup(&up->port);
 	return 0;
 }

Re: Weird Mouse Behaviour with 2.6

[Tomas Cernaj]

Am Donnerstag, den 21.04.2005, 22:08 -0700 schrieb David S. Miller:
> Ok, let's kill this bug already :-)
> 
> We were neglecting to invoke sunsu_change_speed() when first
> setting up the keyboard and mouse ports.  That's why things
> were not working.
> 
> This should fix it and I'll push this upsteam.
> 
> [SPARC64]: In sunsu driver, make sure to fully init chip for kbd/ms
> 
> We were forgetting to call sunsu_change_speed().  The reason
> that replugging in the mouse cable "fixes things" is that
> causes a BREAK interrupt which in turn caused a call to
> sunsu_change_speed() which would get the chip setup properly.

[... skipped patch ...]

Now it's working fine, many thanks!

Tomas