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

[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?]

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

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.

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?]