Re: Fix locking in input

Re: Fix locking in input

[Manfred Spraul]

Paul wrote:

>Pavel Machek writes:
>
>> input uses "volatile signed char" as a shared variable between normal
>> and interrupt threads (look at _sendbyte()). Thats bad idea, this
>> switches it to atomic_t.
>
>This change looks unnecessary to me - we aren't trying to increment or
>decrement the variable, just set it and read it.  Reading and writing
>individual bytes is atomic on any platform we care about.
>  
>
I think one platform (early ARM?) cannot access bytes directly, and 
implement the access with read 16-bit, change 8-bit, write back 16 bit. 
Reading/writing pointers or longs is atomic.

Pavel: Do you know that atomic_set and atomic_read aren't memory barriers?
I.e.

-	psmouse->ack = 0;
+	atomic_set(&psmouse->ack, 0);
 	psmouse->acking = 1;

It's not guaranteed that all cpus will see psmouse->ack=0 before psmouse->acking=1. And adding the required memory barriers usually makes the code completely unreadable, thus I usually give up and switch to a spinlock.

--
	Manfred

Re: Fix locking in input

[Russell King]

On Sun, Nov 23, 2003 at 11:24:02PM +0100, Manfred Spraul wrote:
> I think one platform (early ARM?) cannot access bytes directly, and 
> implement the access with read 16-bit, change 8-bit, write back 16 bit. 

Nope.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: Fix locking in input

[Manfred Spraul]

Russell King wrote:

>On Sun, Nov 23, 2003 at 11:24:02PM +0100, Manfred Spraul wrote:
>  
>
>>I think one platform (early ARM?) cannot access bytes directly, and 
>>implement the access with read 16-bit, change 8-bit, write back 16 bit. 
>>    
>>
>
>Nope.
>
It seems it's Alpha:

On Thu, 28 Dec 2000, Linus wrote:

>FreeBSD doesn't try to be portable any more, but Linux does, and there are
>architectures where 8- and 16-bit accesses aren't *atomic* but have to be
>done with read-modify-write cycles.
>
>And even for fields like "age", where we don't care whether the age itself
>is 100% accurate, we _do_ care that the fields close-by don't get strange
>effects from updating "age". We used to have exactly this problem on alpha
>back in the 2.1.x timeframe.
>


--
    Manfred

Re: Fix locking in input

[Timothy Miller]

Manfred Spraul wrote:
> Russell King wrote:
> 
>> On Sun, Nov 23, 2003 at 11:24:02PM +0100, Manfred Spraul wrote:
>>  
>>
>>> I think one platform (early ARM?) cannot access bytes directly, and 
>>> implement the access with read 16-bit, change 8-bit, write back 16 
>>> bit.   
>>
>>
>> Nope.
>>
> It seems it's Alpha:
> 
> On Thu, 28 Dec 2000, Linus wrote:
> 
>> FreeBSD doesn't try to be portable any more, but Linux does, and there 
>> are
>> architectures where 8- and 16-bit accesses aren't *atomic* but have to be
>> done with read-modify-write cycles.
>>
>> And even for fields like "age", where we don't care whether the age 
>> itself
>> is 100% accurate, we _do_ care that the fields close-by don't get strange
>> effects from updating "age". We used to have exactly this problem on 
>> alpha
>> back in the 2.1.x timeframe.
>>


This is MOSTLY true, but not entirely.  As I recall, Alpha has two 
addressing modes: Sparse and Dense.

The mode you're referring to is dense addressing where 32-bit and 64-bit 
accesses are atomic, but 16 and 8 bit accesses require RMW.

Sparse mode allows for atomic 8 and 16 bit accesses, but the access size 
is specified in the address using a rather bizarre binary encoding that 
doesn't look much like a regular address value.  You would probably not 
want to use this unless you absolutely HAD to use some I/O device that 
required atomic byte or word accesses.

I'm sure someone else will (or already has) pipe(d) in with a more 
accurate explanation.  :)

Re: Fix locking in input

[Linus Torvalds]

On Wed, 3 Dec 2003, Timothy Miller wrote:
>
> This is MOSTLY true, but not entirely.  As I recall, Alpha has two
> addressing modes: Sparse and Dense.

Nope. Alpha only does "dense" addressing on a CPU level.

What the _system_ designers on most PCI systems did was to map the PCI
address space twice: once through a "dense" mapping and once through a
"sparse" mapping. In the sparse mapping the low address bits give byte
enable information, while in the dense mapping you can onyl do 32-bit and
64-bit aligned operations.

But the original alpha CPU couldn't do the sparse mapping - in particular
the above only worked with non-cached accesses.

So "real memory" was always dense, and could not atomically be accessed
with byte/word writes (you could use the atomic "load-locked" +
"store-conditional" to do them, but at a huge performance loss, and
obviously the compilers never did that).

		Linus

Re: Fix locking in input

[Pavel Machek]

Hi!

> >>input uses "volatile signed char" as a shared variable between normal
> >>and interrupt threads (look at _sendbyte()). Thats bad idea, this
> >>switches it to atomic_t.
> >
> >This change looks unnecessary to me - we aren't trying to increment or
> >decrement the variable, just set it and read it.  Reading and writing
> >individual bytes is atomic on any platform we care about.
> > 
> >
> I think one platform (early ARM?) cannot access bytes directly, and 
> implement the access with read 16-bit, change 8-bit, write back 16 bit. 
> Reading/writing pointers or longs is atomic.
> 
> Pavel: Do you know that atomic_set and atomic_read aren't memory barriers?
> I.e.
> 
> -	psmouse->ack = 0;
> +	atomic_set(&psmouse->ack, 0);
> 	psmouse->acking = 1;
> 
> It's not guaranteed that all cpus will see psmouse->ack=0 before 
> psmouse->acking=1. And adding the required memory barriers usually makes 
> the code completely unreadable, thus I usually give up and switch to a 
> spinlock.

Would simply adding mb() there fix it? It is not performance-critical
in any way...
								Pavel

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Fix locking in input

[Pavel Machek]

Hi!

input uses "volatile signed char" as a shared variable between normal
and interrupt threads (look at _sendbyte()). Thats bad idea, this
switches it to atomic_t.

I did not dare to store -1 o atomic ariable, that's why I switched -1
to +2.

I hope I do not break compilation somewhere, otherwise its trivial...

								Pavel

--- tmp/linux/drivers/input/keyboard/98kbd.c	2003-04-21 22:31:32.000000000 +0200
+++ linux/drivers/input/keyboard/98kbd.c	2003-11-22 16:36:39.000000000 +0100
@@ -100,7 +100,7 @@
 	char phys[32];
 	unsigned char cmdbuf[4];
 	unsigned char cmdcnt;
-	signed char ack;
+	atomic_t ack;
 	unsigned char shift;
 	struct {
 		unsigned char scancode;
@@ -118,10 +118,10 @@
 
 	switch (data) {
 		case KBD98_RET_ACK:
-			kbd98->ack = 1;
+			atomic_set(&kbd98->ack, 1);
 			return;
 		case KBD98_RET_NAK:
-			kbd98->ack = -1;
+			atomic_set(&kbd98->ack, 2);
 			return;
 	}
 
@@ -220,14 +220,14 @@
 static int kbd98_sendbyte(struct kbd98 *kbd98, unsigned char byte)
 {
 	int timeout = 10000; /* 100 msec */
-	kbd98->ack = 0;
+	atomic_set(&kbd98->ack, 0);
 
 	if (serio_write(kbd98->serio, byte))
 		return -1;
 
-	while (!kbd98->ack && timeout--) udelay(10);
+	while (!atomic_read(&kbd98->ack) && timeout--) udelay(10);
 
-	return -(kbd98->ack <= 0);
+	return -(atomic_read(&kbd98->ack) == 2);
 }
 
 /*
--- tmp/linux/drivers/input/keyboard/atkbd.c	2003-10-26 13:08:37.000000000 +0100
+++ linux/drivers/input/keyboard/atkbd.c	2003-11-22 16:36:30.000000000 +0100
@@ -145,7 +145,7 @@
 	unsigned char set;
 	unsigned char release;
 	int lastkey;
-	volatile signed char ack;
+	atomic_t ack;
 	unsigned char emul;
 	unsigned short id;
 	unsigned char write;
@@ -214,10 +214,10 @@
 
 	switch (code) {
 		case ATKBD_RET_ACK:
-			atkbd->ack = 1;
+			atomic_set(&atkbd->ack, 1);
 			goto out;
 		case ATKBD_RET_NAK:
-			atkbd->ack = -1;
+			atomic_set(&atkbd->ack, 2);
 			goto out;
 	}
 
@@ -294,7 +294,7 @@
 static int atkbd_sendbyte(struct atkbd *atkbd, unsigned char byte)
 {
 	int timeout = 20000; /* 200 msec */
-	atkbd->ack = 0;
+	atomic_set(&atkbd->ack, 0);
 
 #ifdef ATKBD_DEBUG
 	printk(KERN_DEBUG "atkbd.c: Sent: %02x\n", byte);
@@ -302,9 +302,9 @@
 	if (serio_write(atkbd->serio, byte))
 		return -1;
 
-	while (!atkbd->ack && timeout--) udelay(10);
+	while (!atomic_read(&atkbd->ack) && timeout--) udelay(10);
 
-	return -(atkbd->ack <= 0);
+	return -(atomic_read(&atkbd->ack) == 2);
 }
 
 /*
--- tmp/linux/drivers/input/mouse/psmouse-base.c	2003-09-28 22:05:48.000000000 +0200
+++ linux/drivers/input/mouse/psmouse-base.c	2003-11-22 16:15:12.000000000 +0100
@@ -121,13 +121,14 @@
 	if (psmouse->acking) {
 		switch (data) {
 			case PSMOUSE_RET_ACK:
-				psmouse->ack = 1;
+				atomic_set(&psmouse->ack, 1);
 				break;
 			case PSMOUSE_RET_NAK:
-				psmouse->ack = -1;
+				atomic_set(&psmouse->ack, 2);
 				break;
 			default:
-				psmouse->ack = 1;	/* Workaround for mice which don't ACK the Get ID command */
+				/* Workaround for mice which don't ACK the Get ID command */
+				atomic_set(&psmouse->ack, 1);
 				if (psmouse->cmdcnt)
 					psmouse->cmdbuf[--psmouse->cmdcnt] = data;
 				break;
@@ -197,7 +198,7 @@
 static int psmouse_sendbyte(struct psmouse *psmouse, unsigned char byte)
 {
 	int timeout = 10000; /* 100 msec */
-	psmouse->ack = 0;
+	atomic_set(&psmouse->ack, 0);
 	psmouse->acking = 1;
 
 	if (serio_write(psmouse->serio, byte)) {
@@ -205,9 +206,9 @@
 		return -1;
 	}
 
-	while (!psmouse->ack && timeout--) udelay(10);
+	while (!atomic_read(&psmouse->ack) && timeout--) udelay(10);
 
-	return -(psmouse->ack <= 0);
+	return -(atomic_read(&psmouse->ack) == 2);
 }
 
 /*
--- tmp/linux/drivers/input/mouse/psmouse.h	2003-09-28 22:05:48.000000000 +0200
+++ linux/drivers/input/mouse/psmouse.h	2003-11-22 16:08:01.000000000 +0100
@@ -37,7 +37,7 @@
 	unsigned long last;
 	unsigned char state;
 	char acking;
-	volatile char ack;
+	atomic_t ack;	/* This is being accessed without locking, at least make sure we do not run into alignment problems */
 	char error;
 	char devname[64];
 	char phys[32];

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: Fix locking in input

[Paul Mackerras]

Pavel Machek writes:

> input uses "volatile signed char" as a shared variable between normal
> and interrupt threads (look at _sendbyte()). Thats bad idea, this
> switches it to atomic_t.

This change looks unnecessary to me - we aren't trying to increment or
decrement the variable, just set it and read it.  Reading and writing
individual bytes is atomic on any platform we care about.

Paul.

Re: Fix locking in input

[Pavel Machek]

Hi!

> > input uses "volatile signed char" as a shared variable between normal
> > and interrupt threads (look at _sendbyte()). Thats bad idea, this
> > switches it to atomic_t.
> > 
> > I did not dare to store -1 o atomic ariable, that's why I switched -1
> > to +2.
> > 
> > I hope I do not break compilation somewhere, otherwise its trivial...
> 
> It is not.
> 
> > @@ -220,14 +220,14 @@
> >  static int kbd98_sendbyte(struct kbd98 *kbd98, unsigned char byte)
> >  {
> >  	int timeout = 10000; /* 100 msec */
> > -	kbd98->ack = 0;
> > +	atomic_set(&kbd98->ack, 0);
> >  
> >  	if (serio_write(kbd98->serio, byte))
> >  		return -1;
> >  
> > -	while (!kbd98->ack && timeout--) udelay(10);
> > +	while (!atomic_read(&kbd98->ack) && timeout--) udelay(10);
> >  
> > -	return -(kbd98->ack <= 0);
> > +	return -(atomic_read(&kbd98->ack) == 2);
> >  }
> 
> <= 0 is not same as == 2. You need atomic_read(&kbd98->ack) == 0 || ... == 2 to
> get same behavior. No ack means error.

Yep, right. This should fix it.
								Pavel

--- linux.orig/drivers/input/keyboard/atkbd.c	2003-11-24 11:30:30.000000000 +0100
+++ linux/drivers/input/keyboard/atkbd.c	2003-11-24 10:31:28.000000000 +0100
@@ -145,7 +145,7 @@
 	unsigned char set;
 	unsigned char release;
 	int lastkey;
-	volatile signed char ack;
+	atomic_t ack;
 	unsigned char emul;
 	unsigned short id;
 	unsigned char write;
@@ -214,10 +214,10 @@
 
 	switch (code) {
 		case ATKBD_RET_ACK:
-			atkbd->ack = 1;
+			atomic_set(&atkbd->ack, 1);
 			goto out;
 		case ATKBD_RET_NAK:
-			atkbd->ack = -1;
+			atomic_set(&atkbd->ack, 2);
 			goto out;
 	}
 
@@ -294,7 +294,7 @@
 static int atkbd_sendbyte(struct atkbd *atkbd, unsigned char byte)
 {
 	int timeout = 20000; /* 200 msec */
-	atkbd->ack = 0;
+	atomic_set(&atkbd->ack, 0);
 
 #ifdef ATKBD_DEBUG
 	printk(KERN_DEBUG "atkbd.c: Sent: %02x\n", byte);
@@ -302,9 +302,9 @@
 	if (serio_write(atkbd->serio, byte))
 		return -1;
 
-	while (!atkbd->ack && timeout--) udelay(10);
+	while (!atomic_read(&atkbd->ack) && timeout--) udelay(10);
 
-	return -(atkbd->ack <= 0);
+	return -(atomic_read(&atkbd->ack) != 1);
 }
 
 /*
--- linux.orig/drivers/input/keyboard/98kbd.c	2003-11-24 11:30:30.000000000 +0100
+++ linux/drivers/input/keyboard/98kbd.c	2003-11-24 11:29:07.000000000 +0100
@@ -100,7 +100,7 @@
 	char phys[32];
 	unsigned char cmdbuf[4];
 	unsigned char cmdcnt;
-	signed char ack;
+	atomic_t ack;
 	unsigned char shift;
 	struct {
 		unsigned char scancode;
@@ -118,10 +118,10 @@
 
 	switch (data) {
 		case KBD98_RET_ACK:
-			kbd98->ack = 1;
+			atomic_set(&kbd98->ack, 1);
 			return;
 		case KBD98_RET_NAK:
-			kbd98->ack = -1;
+			atomic_set(&kbd98->ack, 2);
 			return;
 	}
 
@@ -220,14 +220,14 @@
 static int kbd98_sendbyte(struct kbd98 *kbd98, unsigned char byte)
 {
 	int timeout = 10000; /* 100 msec */
-	kbd98->ack = 0;
+	atomic_set(&kbd98->ack, 0);
 
 	if (serio_write(kbd98->serio, byte))
 		return -1;
 
-	while (!kbd98->ack && timeout--) udelay(10);
+	while (!atomic_read(&kbd98->ack) && timeout--) udelay(10);
 
-	return -(kbd98->ack <= 0);
+	return -(atomic_read(&kbd98->ack) != 1);
 }
 
 /*
--- linux.orig/drivers/input/mouse/psmouse-base.c	2003-11-24 11:30:30.000000000 +0100
+++ linux/drivers/input/mouse/psmouse-base.c	2003-11-24 11:29:30.000000000 +0100
@@ -121,13 +121,14 @@
 	if (psmouse->acking) {
 		switch (data) {
 			case PSMOUSE_RET_ACK:
-				psmouse->ack = 1;
+				atomic_set(&psmouse->ack, 1);
 				break;
 			case PSMOUSE_RET_NAK:
-				psmouse->ack = -1;
+				atomic_set(&psmouse->ack, 2);
 				break;
 			default:
-				psmouse->ack = 1;	/* Workaround for mice which don't ACK the Get ID command */
+				/* Workaround for mice which don't ACK the Get ID command */
+				atomic_set(&psmouse->ack, 1);
 				if (psmouse->cmdcnt)
 					psmouse->cmdbuf[--psmouse->cmdcnt] = data;
 				break;
@@ -197,7 +198,7 @@
 static int psmouse_sendbyte(struct psmouse *psmouse, unsigned char byte)
 {
 	int timeout = 10000; /* 100 msec */
-	psmouse->ack = 0;
+	atomic_set(&psmouse->ack, 0);
 	psmouse->acking = 1;
 
 	if (serio_write(psmouse->serio, byte)) {
@@ -205,9 +206,9 @@
 		return -1;
 	}
 
-	while (!psmouse->ack && timeout--) udelay(10);
+	while (!atomic_read(&psmouse->ack) && timeout--) udelay(10);
 
-	return -(psmouse->ack <= 0);
+	return -(atomic_read(&psmouse->ack) != 1);
 }
 
 /*
Index: linux/drivers/input/mouse/psmouse.h
===================================================================
--- linux.orig/drivers/input/mouse/psmouse.h	2003-11-24 11:30:30.000000000 +0100
+++ linux/drivers/input/mouse/psmouse.h	2003-11-24 11:29:43.000000000 +0100
@@ -37,7 +37,7 @@
 	unsigned long last;
 	unsigned char state;
 	char acking;
-	volatile char ack;
+	atomic_t ack;	/* This is being accessed without locking, at least make sure we do not run into alignment problems */
 	char error;
 	char devname[64];
 	char phys[32];


-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]