locking in psmouse

locking in psmouse

[Pavel Machek]

Hi!

psmouse-base.c does not have any locking. For example psmouse_command
could race with data coming from the mouse, resulting in problem. This
should fix it.
								Pavel

--- clean/drivers/input/mouse/psmouse.h	2004-04-05 10:45:16.000000000 +0200
+++ linux/drivers/input/mouse/psmouse.h	2004-04-28 23:26:36.000000000 +0200
@@ -47,7 +47,7 @@
 	unsigned long last;
 	unsigned char state;
 	char acking;
-	volatile char ack;
+	char ack;
 	char error;
 	char devname[64];
 	char phys[32];
--- clean/drivers/input/mouse/psmouse-base.c	2004-04-05 10:45:16.000000000 +0200
+++ linux/drivers/input/mouse/psmouse-base.c	2004-04-28 23:24:51.000000000 +0200
@@ -53,6 +53,8 @@
 __obsolete_setup("psmouse_resetafter=");
 __obsolete_setup("psmouse_rate=");
 
+static spinlock_t psmouse_lock = SPIN_LOCK_UNLOCKED;
+
 static char *psmouse_protocols[] = { "None", "PS/2", "PS2++", "PS2T++", "GenPS/2", "ImPS/2", "ImExPS/2", "SynPS/2"};
 
 /*
@@ -124,6 +126,7 @@
 {
 	struct psmouse *psmouse = serio->private;
 
+	spin_lock_irq(&psmouse_lock);
 	if (psmouse->state == PSMOUSE_IGNORE)
 		goto out;
 
@@ -208,6 +211,7 @@
 		goto out;
 	}
 out:
+	spin_unlock_irq(&psmouse_lock);
 	return IRQ_HANDLED;
 }
 
@@ -215,6 +219,8 @@
  * psmouse_sendbyte() sends a byte to the mouse, and waits for acknowledge.
  * It doesn't handle retransmission, though it could - because when there would
  * be need for retransmissions, the mouse has to be replaced anyway.
+ *
+ * Must be called with psmouse_lock held.
  */
 
 static int psmouse_sendbyte(struct psmouse *psmouse, unsigned char byte)
@@ -228,7 +234,11 @@
 		return -1;
 	}
 
-	while (!psmouse->ack && timeout--) udelay(10);
+	while (!psmouse->ack && timeout--) {
+		spin_unlock_irq(&psmouse_lock);
+		udelay(10);
+		spin_lock_irq(&psmouse_lock);
+	}
 
 	return -(psmouse->ack <= 0);
 }
@@ -243,8 +253,9 @@
 	int timeout = 500000; /* 500 msec */
 	int send = (command >> 12) & 0xf;
 	int receive = (command >> 8) & 0xf;
-	int i;
+	int i, ret = 0;
 
+	spin_lock_irq(&psmouse_lock);
 	psmouse->cmdcnt = receive;
 
 	if (command == PSMOUSE_CMD_RESET_BAT)
@@ -257,11 +268,11 @@
 
 	if (command & 0xff)
 		if (psmouse_sendbyte(psmouse, command & 0xff))
-			return (psmouse->cmdcnt = 0) - 1;
+			goto error;
 
 	for (i = 0; i < send; i++)
 		if (psmouse_sendbyte(psmouse, param[i]))
-			return (psmouse->cmdcnt = 0) - 1;
+			goto error;
 
 	while (psmouse->cmdcnt && timeout--) {
 
@@ -275,16 +286,21 @@
 			break;
 		}
 
+		spin_unlock_irq(&psmouse_lock);
 		udelay(1);
+		spin_lock_irq(&psmouse_lock);
 	}
 
 	for (i = 0; i < receive; i++)
 		param[i] = psmouse->cmdbuf[(receive - 1) - i];
 
-	if (psmouse->cmdcnt)
-		return (psmouse->cmdcnt = 0) - 1;
-
-	return 0;
+	if (psmouse->cmdcnt) {
+	error:
+		psmouse->cmdcnt = 0;
+		ret = - 1;
+	}
+	spin_unlock_irq(&psmouse_lock);
+	return ret;
 }
 
 
@@ -575,7 +591,9 @@
 {
 	struct psmouse *psmouse = serio->private;
 
+	spin_lock_irq(&psmouse_lock);
 	psmouse->state = PSMOUSE_CMD_MODE;
+	spin_unlock_irq(&psmouse_lock);
 
 	if (psmouse->ptport) {
 		if (psmouse->ptport->deactivate)
@@ -588,7 +606,9 @@
 	if (psmouse->disconnect)
 		psmouse->disconnect(psmouse);
 
+	spin_lock_irq(&psmouse_lock);
 	psmouse->state = PSMOUSE_IGNORE;
+	spin_unlock_irq(&psmouse_lock);
 
 	input_unregister_device(&psmouse->dev);
 	serio_close(serio);
@@ -675,10 +695,13 @@
 		return -1;
 	}
 
+	spin_lock_irq(&psmouse_lock);
 	old_type = psmouse->type;
 
 	psmouse->state = PSMOUSE_CMD_MODE;
 	psmouse->type = psmouse->acking = psmouse->cmdcnt = psmouse->pktcnt = 0;
+	spin_unlock_irq(&psmouse_lock);
+
 	if (psmouse->reconnect) {
 	       if (psmouse->reconnect(psmouse))
 			return -1;

-- 
934a471f20d6580d5aad759bf0d97ddc

Re: locking in psmouse

[Dmitry Torokhov]

On Wednesday 28 April 2004 04:30 pm, Pavel Machek wrote:
> Hi!
> 
> psmouse-base.c does not have any locking. For example psmouse_command
> could race with data coming from the mouse, resulting in problem. This
> should fix it.
> 								Pavel

Although I am not arguing that locking might be needed in psmouse module I
am somewhat confused how it will help in case of data stream coming from the
mouse... If mouse sent a byte before the kernel issue a command then it will
be delivered by KBC controller and will be processed by the interrupt handler,
probably messing up detection process. That's why as soon as we decide that
the device behind PS/2 port is some kind of mouse we disable the stream mode.

-- 
Dmitry

Re: locking in psmouse

[Pavel Machek]

Hi!

> > psmouse-base.c does not have any locking. For example psmouse_command
> > could race with data coming from the mouse, resulting in problem. This
> > should fix it.
> 
> Although I am not arguing that locking might be needed in psmouse module I
> am somewhat confused how it will help in case of data stream coming from the
> mouse... If mouse sent a byte before the kernel issue a command then it will
> be delivered by KBC controller and will be processed by the interrupt handler,
> probably messing up detection process. That's why as soon as we decide that
> the device behind PS/2 port is some kind of mouse we disable the stream mode.

Does that mean that mouse can not talk while we are sending commands
to it? That would help a bit.

Anyway, locking still seems to be needed: 

        while (psmouse->cmdcnt && timeout--) {

                if (psmouse->cmdcnt == 1 && command == PSMOUSE_CMD_RESET_BAT &&
                                timeout > 100000) /* do not run in a endless loop */
                        timeout = 100000; /* 1 sec */

                if (psmouse->cmdcnt == 1 && command == PSMOUSE_CMD_GETID &&
                    psmouse->cmdbuf[1] != 0xab && psmouse->cmdbuf[1] != 0xac) {
                        psmouse->cmdcnt = 0;
                        break;
                }

                spin_unlock_irq(&psmouse_lock);
                udelay(1);
                spin_lock_irq(&psmouse_lock);
        }

racing with

        if (psmouse->cmdcnt) {
                psmouse->cmdbuf[--psmouse->cmdcnt] = data;
                goto out;
        }

now... if each runs on different CPU, it can be possible that
psmouse->cmdcnt is seen as 1 but data are not yet in
psmouse->cmdbuf... Locking seems neccessary here.
							Pavel
-- 
934a471f20d6580d5aad759bf0d97ddc

Re: locking in psmouse

[Dmitry Torokhov]

Hi Pavel,

On Thursday 29 April 2004 04:58 am, Pavel Machek wrote:
> Hi!
> 
> > > psmouse-base.c does not have any locking. For example psmouse_command
> > > could race with data coming from the mouse, resulting in problem. This
> > > should fix it.
> > 
> > Although I am not arguing that locking might be needed in psmouse module I
> > am somewhat confused how it will help in case of data stream coming from the
> > mouse... If mouse sent a byte before the kernel issue a command then it will
> > be delivered by KBC controller and will be processed by the interrupt handler,
> > probably messing up detection process. That's why as soon as we decide that
> > the device behind PS/2 port is some kind of mouse we disable the stream mode.
> 
> Does that mean that mouse can not talk while we are sending commands
> to it? That would help a bit.
> 

Yes, check psmouse_probe. As soon as PSMOUSE_CMD_RESET_DIS acknowledged mouse
should cease sending motion data. That still leaves possibility of screwing up
detection if you are moving mouse while psmouse doing PSMOUSE_CMD_GETID.
But I don't think we can do much about it as we'd like to know that the device
is some kind of a mouse before we start messing with it.

> Anyway, locking still seems to be needed: 
> 
>         while (psmouse->cmdcnt && timeout--) {
> 
>                 if (psmouse->cmdcnt == 1 && command == PSMOUSE_CMD_RESET_BAT &&
>                                 timeout > 100000) /* do not run in a endless loop */
>                         timeout = 100000; /* 1 sec */
> 
>                 if (psmouse->cmdcnt == 1 && command == PSMOUSE_CMD_GETID &&
>                     psmouse->cmdbuf[1] != 0xab && psmouse->cmdbuf[1] != 0xac) {
>                         psmouse->cmdcnt = 0;
>                         break;
>                 }
> 
>                 spin_unlock_irq(&psmouse_lock);
>                 udelay(1);
>                 spin_lock_irq(&psmouse_lock);
>         }
> 
> racing with
> 
>         if (psmouse->cmdcnt) {
>                 psmouse->cmdbuf[--psmouse->cmdcnt] = data;
>                 goto out;
>         }
> 
> now... if each runs on different CPU, it can be possible that
> psmouse->cmdcnt is seen as 1 but data are not yet in
> psmouse->cmdbuf... Locking seems neccessary here.

I see.. but this particular case can be resolved but rearranging the code to
write command response first and then decrementing the counter... and putting
a barrier? Or just make cmdcnt atomic... spin_lock_irq feels heavier than
absolutely necessary.

-- 
Dmitry

Re: locking in psmouse

[Pavel Machek]

Hi!

> > Anyway, locking still seems to be needed: 
> > 
> >         while (psmouse->cmdcnt && timeout--) {
> > 
> >                 if (psmouse->cmdcnt == 1 && command == PSMOUSE_CMD_RESET_BAT &&
> >                                 timeout > 100000) /* do not run in a endless loop */
> >                         timeout = 100000; /* 1 sec */
> > 
> >                 if (psmouse->cmdcnt == 1 && command == PSMOUSE_CMD_GETID &&
> >                     psmouse->cmdbuf[1] != 0xab && psmouse->cmdbuf[1] != 0xac) {
> >                         psmouse->cmdcnt = 0;
> >                         break;
> >                 }
> > 
> >                 spin_unlock_irq(&psmouse_lock);
> >                 udelay(1);
> >                 spin_lock_irq(&psmouse_lock);
> >         }
> > 
> > racing with
> > 
> >         if (psmouse->cmdcnt) {
> >                 psmouse->cmdbuf[--psmouse->cmdcnt] = data;
> >                 goto out;
> >         }
> > 
> > now... if each runs on different CPU, it can be possible that
> > psmouse->cmdcnt is seen as 1 but data are not yet in
> > psmouse->cmdbuf... Locking seems neccessary here.
> 
> I see.. but this particular case can be resolved but rearranging the code to
> write command response first and then decrementing the counter... and putting
> a barrier? Or just make cmdcnt atomic... spin_lock_irq feels heavier than
> absolutely necessary.

cmdcnt would have to be atomic_t, ack too, state too, and
you'd have to be very carefull with memory barriers...
I guess spinlock is better solution.
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: locking in psmouse

[Vojtech Pavlik]

On Thu, Apr 29, 2004 at 07:40:17AM -0500, Dmitry Torokhov wrote:

> On Thursday 29 April 2004 04:58 am, Pavel Machek wrote:
> > Hi!
> > 
> > > > psmouse-base.c does not have any locking. For example psmouse_command
> > > > could race with data coming from the mouse, resulting in problem. This
> > > > should fix it.
> > > 
> > > Although I am not arguing that locking might be needed in psmouse module I
> > > am somewhat confused how it will help in case of data stream coming from the
> > > mouse... If mouse sent a byte before the kernel issue a command then it will
> > > be delivered by KBC controller and will be processed by the interrupt handler,
> > > probably messing up detection process. That's why as soon as we decide that
> > > the device behind PS/2 port is some kind of mouse we disable the stream mode.
> > 
> > Does that mean that mouse can not talk while we are sending commands
> > to it? That would help a bit.
> > 
> 
> Yes, check psmouse_probe. As soon as PSMOUSE_CMD_RESET_DIS acknowledged mouse
> should cease sending motion data. That still leaves possibility of screwing up
> detection if you are moving mouse while psmouse doing PSMOUSE_CMD_GETID.
> But I don't think we can do much about it as we'd like to know that the device
> is some kind of a mouse before we start messing with it.

I've updated the atkbd_command/atkbd_interrupt mechanism so that even
bytes coming from the keyboard when we're issuing a command shouldn't
disturb us. I've tested by banging my head at the keyboard while
plugging it in. ;)

Something like that might be worth implementing for psmouse as well.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: locking in psmouse

[Pavel Machek]

Hi!

> > > > > psmouse-base.c does not have any locking. For example psmouse_command
> > > > > could race with data coming from the mouse, resulting in problem. This
> > > > > should fix it.
> > > > 
> > > > Although I am not arguing that locking might be needed in psmouse module I
> > > > am somewhat confused how it will help in case of data stream coming from the
> > > > mouse... If mouse sent a byte before the kernel issue a command then it will
> > > > be delivered by KBC controller and will be processed by the interrupt handler,
> > > > probably messing up detection process. That's why as soon as we decide that
> > > > the device behind PS/2 port is some kind of mouse we disable the stream mode.
> > > 
> > > Does that mean that mouse can not talk while we are sending commands
> > > to it? That would help a bit.
> > > 
> > 
> > Yes, check psmouse_probe. As soon as PSMOUSE_CMD_RESET_DIS acknowledged mouse
> > should cease sending motion data. That still leaves possibility of screwing up
> > detection if you are moving mouse while psmouse doing PSMOUSE_CMD_GETID.
> > But I don't think we can do much about it as we'd like to know that the device
> > is some kind of a mouse before we start messing with it.
> 
> I've updated the atkbd_command/atkbd_interrupt mechanism so that even
> bytes coming from the keyboard when we're issuing a command shouldn't
> disturb us. I've tested by banging my head at the keyboard while
> plugging it in. ;)
> 
> Something like that might be worth implementing for psmouse as well.

Well, psmouse case should be just converting int foo:1 into
set_bit(&flags, BIT_FOO), no?

But I guess autorepeat and higher layers of keyboard might be
"interesting".
							Pavel 

-- 
934a471f20d6580d5aad759bf0d97ddc

Re: locking in psmouse

[Vojtech Pavlik]

On Sun, Jun 06, 2004 at 08:51:59PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > > psmouse-base.c does not have any locking. For example psmouse_command
> > > > > > could race with data coming from the mouse, resulting in problem. This
> > > > > > should fix it.
> > > > > 
> > > > > Although I am not arguing that locking might be needed in psmouse module I
> > > > > am somewhat confused how it will help in case of data stream coming from the
> > > > > mouse... If mouse sent a byte before the kernel issue a command then it will
> > > > > be delivered by KBC controller and will be processed by the interrupt handler,
> > > > > probably messing up detection process. That's why as soon as we decide that
> > > > > the device behind PS/2 port is some kind of mouse we disable the stream mode.
> > > > 
> > > > Does that mean that mouse can not talk while we are sending commands
> > > > to it? That would help a bit.
> > > > 
> > > 
> > > Yes, check psmouse_probe. As soon as PSMOUSE_CMD_RESET_DIS acknowledged mouse
> > > should cease sending motion data. That still leaves possibility of screwing up
> > > detection if you are moving mouse while psmouse doing PSMOUSE_CMD_GETID.
> > > But I don't think we can do much about it as we'd like to know that the device
> > > is some kind of a mouse before we start messing with it.
> > 
> > I've updated the atkbd_command/atkbd_interrupt mechanism so that even
> > bytes coming from the keyboard when we're issuing a command shouldn't
> > disturb us. I've tested by banging my head at the keyboard while
> > plugging it in. ;)
> > 
> > Something like that might be worth implementing for psmouse as well.
> 
> Well, psmouse case should be just converting int foo:1 into
> set_bit(&flags, BIT_FOO), no?

Not that easy. It wasn't as easy for atkbd either. This isn't just a
case of locking - it's making sure you always know whether the next byte
is data from the keyboard/mouse or a command response.

Only after getting the first ACK you can be sure that the keyboard/mouse
will not be sending any scancodes/movement data.

> But I guess autorepeat and higher layers of keyboard might be
> "interesting".

We'll get there ...

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: locking in psmouse

[Vojtech Pavlik]

On Thu, Apr 29, 2004 at 11:58:30AM +0200, Pavel Machek wrote:

> > > psmouse-base.c does not have any locking. For example psmouse_command
> > > could race with data coming from the mouse, resulting in problem. This
> > > should fix it.
> > 
> > Although I am not arguing that locking might be needed in psmouse module I
> > am somewhat confused how it will help in case of data stream coming from the
> > mouse... If mouse sent a byte before the kernel issue a command then it will
> > be delivered by KBC controller and will be processed by the interrupt handler,
> > probably messing up detection process. That's why as soon as we decide that
> > the device behind PS/2 port is some kind of mouse we disable the stream mode.

Any PS/2 device is supposed to stop talking as soon as it gets a command
until it's done with it. Supposed to. Namely USB Legacy emulation breaks
this rule.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR