locking in psmouse

PATCH Pavel Machek 3 participants 9 messages Apr 28, 2004 – Jun 6, 2004 Input drivers

Share & tools

Pavel Machek

author

2004-04-28 21:40 UTC #0 lore raw

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

drivers/input/mouse/psmouse.h +1 -1

@@ -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];

drivers/input/mouse/psmouse-base.c +31 -8

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

Signature · 1 line

934a471f20d6580d5aad759bf0d97ddc

Dmitry Torokhov

2004-04-29 04:47 UTC #1 lore raw

Quoted 6 lines · 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

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.

Signature · 1 line

Dmitry

Pavel Machek

author

2004-04-29 09:58 UTC #2 lore raw

Hi!

Quoted 10 lines

Quoted 3 lines
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

Signature · 1 line

934a471f20d6580d5aad759bf0d97ddc

Dmitry Torokhov

2004-04-29 12:40 UTC #3 lore raw

Hi Pavel,

Quoted 13 lines · Pavel Machek

Hi!
Quoted 10 lines
Quoted 3 lines
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.

Quoted 3 lines

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.

Quoted 21 lines

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

Quoted 8 lines

        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.

Signature · 1 line

Dmitry

Pavel Machek

author

2004-04-29 20:13 UTC #4 lore raw

Hi!

Quoted 26 lines

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

Quoted 8 lines

Quoted 3 lines
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

Signature · 1 line

64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms

Vojtech Pavlik

2004-06-06 17:46 UTC #5 lore raw

In reply to Dmitry Torokhov

Quoted 18 lines · Dmitry Torokhov

Quoted 13 lines · Pavel Machek
Hi!
Quoted 10 lines
Quoted 3 lines
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.
Quoted 3 lines
Does that mean that mouse can not talk while we are sending commands to it? That would help a bit.

Quoted 5 lines

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.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs, SuSE CR

Pavel Machek

author

2004-06-06 18:53 UTC #6 lore raw

Hi!

Quoted 21 lines

Quoted 15 lines
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
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.
Quoted 3 lines
Does that mean that mouse can not talk while we are sending commands to it? That would help a bit.
Quoted 5 lines
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.

Quoted 6 lines

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

Signature · 1 line

934a471f20d6580d5aad759bf0d97ddc

Vojtech Pavlik

2004-06-06 19:33 UTC #7 lore raw

Quoted 28 lines · Pavel Machek

Hi!
Quoted 26 lines
Quoted 15 lines
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
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.
Quoted 3 lines
Does that mean that mouse can not talk while we are sending commands to it? That would help a bit.
Quoted 5 lines
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. ;)

Quoted 4 lines

Quoted 1 line
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.

Quoted 2 lines

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

We'll get there ...

Signature · 2 lines

Vojtech Pavlik
SuSE Labs, SuSE CR

Vojtech Pavlik

2004-04-29 16:02 UTC #8 lore raw

In reply to Pavel Machek

Quoted 10 lines · Pavel Machek

Quoted 10 lines
Quoted 3 lines
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.

Signature · 2 lines

Vojtech Pavlik
SuSE Labs, SuSE CR