[PATCH] escc: explicitly mark variable containing incoming keyboard character as volatile
Otherwise the compiler may not understand that a read from the assigned buffer is comming from memory-mapped IO space and fail to update accordingly.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- drivers/escc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/escc.c b/drivers/escc.c index 392625e..d9e1990 100644 --- a/drivers/escc.c +++ b/drivers/escc.c @@ -284,7 +284,7 @@ keyboard_dataready(void) unsigned char keyboard_readdata(void) { - unsigned char ch; + volatile unsigned char ch;
while (!keyboard_dataready()) { }
On Fri, Oct 18, 2019 at 08:48:53AM +0100, Mark Cave-Ayland wrote:
Otherwise the compiler may not understand that a read from the assigned buffer is comming from memory-mapped IO space and fail to update accordingly.
I think you should not tell the compiler that whatever writes to ch writes the same value multiple times in sequence, instead?
Segher
On 18/10/2019 10:52, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 08:48:53AM +0100, Mark Cave-Ayland wrote:
Otherwise the compiler may not understand that a read from the assigned buffer is comming from memory-mapped IO space and fail to update accordingly.
I think you should not tell the compiler that whatever writes to ch writes the same value multiple times in sequence, instead?
Can you explain a bit more about what you mean? The issue seems to be related to this:
static volatile unsigned char *kbd_dev;
void kbd_init(phys_addr_t base) { kbd_dev = (unsigned char *)ofmem_map_io(base, 2 * 4); kbd_dev += 4; }
... ...
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { }
do { ch = kbd_dev[2] & 0xff;
... }
Without the volatile marking on ch it seems with optimisation enabled that ch gets optimised into a register causing strange intermittent keyboard behaviour. I guess since kbd_dev is a pointer to something of unknown size, the compiler doesn't understand that kbd_dev[2] should also be considered volatile?
ATB,
Mark.
Hi!
On Fri, Oct 18, 2019 at 04:53:26PM +0100, Mark Cave-Ayland wrote:
On 18/10/2019 10:52, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 08:48:53AM +0100, Mark Cave-Ayland wrote:
Otherwise the compiler may not understand that a read from the assigned buffer is comming from memory-mapped IO space and fail to update accordingly.
I think you should not tell the compiler that whatever writes to ch writes the same value multiple times in sequence, instead?
Can you explain a bit more about what you mean? The issue seems to be related to this:
static volatile unsigned char *kbd_dev;
void kbd_init(phys_addr_t base) { kbd_dev = (unsigned char *)ofmem_map_io(base, 2 * 4); kbd_dev += 4; }
... ...
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; ...}
Without the volatile marking on ch it seems with optimisation enabled that ch gets optimised into a register causing strange intermittent keyboard behaviour. I guess since kbd_dev is a pointer to something of unknown size, the compiler doesn't understand that kbd_dev[2] should also be considered volatile?
No, the compiler understands that fine.
How is ch *used*?
[ It's a lot better to treat I/O as I/O, accessed via access functions, than to treat it as memory, exposed in C as some variables. I/O is not memory, and you cannot access it exactly the same way, normally. But that aside. ]
Segher
On 18/10/2019 17:23, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 04:53:26PM +0100, Mark Cave-Ayland wrote:
On 18/10/2019 10:52, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 08:48:53AM +0100, Mark Cave-Ayland wrote:
Otherwise the compiler may not understand that a read from the assigned buffer is comming from memory-mapped IO space and fail to update accordingly.
I think you should not tell the compiler that whatever writes to ch writes the same value multiple times in sequence, instead?
Can you explain a bit more about what you mean? The issue seems to be related to this:
static volatile unsigned char *kbd_dev;
void kbd_init(phys_addr_t base) { kbd_dev = (unsigned char *)ofmem_map_io(base, 2 * 4); kbd_dev += 4; }
... ...
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; ...}
Without the volatile marking on ch it seems with optimisation enabled that ch gets optimised into a register causing strange intermittent keyboard behaviour. I guess since kbd_dev is a pointer to something of unknown size, the compiler doesn't understand that kbd_dev[2] should also be considered volatile?
No, the compiler understands that fine.
How is ch *used*?
[ It's a lot better to treat I/O as I/O, accessed via access functions, than to treat it as memory, exposed in C as some variables. I/O is not memory, and you cannot access it exactly the same way, normally. But that aside. ]
It's used to return a single character read from the serial port (along with some special key detection and keycode translation). The complete code looks like this:
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { }
do { ch = kbd_dev[2] & 0xff; if (ch == 99) shiftstate |= 1; else if (ch == 110) shiftstate |= 2; else if (ch == 227) shiftstate &= ~1; else if (ch == 238) shiftstate &= ~2; //printk("getch: %d\n", ch); } // If release, wait for key press while ((ch & 0x80) == 0x80 || ch == 238 || ch == 227); //printk("getch rel: %d\n", ch); ch &= 0x7f; if (shiftstate) ch = sunkbd_keycode_shifted[ch]; else ch = sunkbd_keycode[ch]; //printk("getch xlate: %d\n", ch);
return ch; }
The kbd_dev pointer is allocated dynamically when mapping the physical device address.
ATB,
Mark.
On 20/10/2019 13:22, Mark Cave-Ayland wrote:
On 18/10/2019 17:23, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 04:53:26PM +0100, Mark Cave-Ayland wrote:
On 18/10/2019 10:52, Segher Boessenkool wrote:
On Fri, Oct 18, 2019 at 08:48:53AM +0100, Mark Cave-Ayland wrote:
Otherwise the compiler may not understand that a read from the assigned buffer is comming from memory-mapped IO space and fail to update accordingly.
I think you should not tell the compiler that whatever writes to ch writes the same value multiple times in sequence, instead?
Can you explain a bit more about what you mean? The issue seems to be related to this:
static volatile unsigned char *kbd_dev;
void kbd_init(phys_addr_t base) { kbd_dev = (unsigned char *)ofmem_map_io(base, 2 * 4); kbd_dev += 4; }
... ...
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; ...}
Without the volatile marking on ch it seems with optimisation enabled that ch gets optimised into a register causing strange intermittent keyboard behaviour. I guess since kbd_dev is a pointer to something of unknown size, the compiler doesn't understand that kbd_dev[2] should also be considered volatile?
No, the compiler understands that fine.
How is ch *used*?
[ It's a lot better to treat I/O as I/O, accessed via access functions, than to treat it as memory, exposed in C as some variables. I/O is not memory, and you cannot access it exactly the same way, normally. But that aside. ]
It's used to return a single character read from the serial port (along with some special key detection and keycode translation). The complete code looks like this:
unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; if (ch == 99) shiftstate |= 1; else if (ch == 110) shiftstate |= 2; else if (ch == 227) shiftstate &= ~1; else if (ch == 238) shiftstate &= ~2; //printk("getch: %d\n", ch); } // If release, wait for key press while ((ch & 0x80) == 0x80 || ch == 238 || ch == 227); //printk("getch rel: %d\n", ch); ch &= 0x7f; if (shiftstate) ch = sunkbd_keycode_shifted[ch]; else ch = sunkbd_keycode[ch]; //printk("getch xlate: %d\n", ch); return ch;}
The kbd_dev pointer is allocated dynamically when mapping the physical device address.
Hi Segher,
Any further comments? I'd like to get these last couple of patchsets applied fairly soon since it's QEMU freeze starting next week.
ATB,
Mark.
On Fri, Oct 25, 2019 at 08:42:13AM +0100, Mark Cave-Ayland wrote:
On 20/10/2019 13:22, Mark Cave-Ayland wrote:
On 18/10/2019 17:23, Segher Boessenkool wrote: unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; if (ch == 99) shiftstate |= 1; else if (ch == 110) shiftstate |= 2; else if (ch == 227) shiftstate &= ~1; else if (ch == 238) shiftstate &= ~2; //printk("getch: %d\n", ch); } // If release, wait for key press while ((ch & 0x80) == 0x80 || ch == 238 || ch == 227); //printk("getch rel: %d\n", ch); ch &= 0x7f; if (shiftstate) ch = sunkbd_keycode_shifted[ch]; else ch = sunkbd_keycode[ch]; //printk("getch xlate: %d\n", ch); return ch;}
I don't see how this would loop, given that kbd_dev is a pointer to volatile.
Any further comments? I'd like to get these last couple of patchsets applied fairly soon since it's QEMU freeze starting next week.
I'd use your workaround then? But something else is going on, it would be good to figure out what.
Segher
On 25/10/2019 11:28, Segher Boessenkool wrote:
On Fri, Oct 25, 2019 at 08:42:13AM +0100, Mark Cave-Ayland wrote:
On 20/10/2019 13:22, Mark Cave-Ayland wrote:
On 18/10/2019 17:23, Segher Boessenkool wrote: unsigned char keyboard_readdata(void) { volatile unsigned char ch;
while (!keyboard_dataready()) { } do { ch = kbd_dev[2] & 0xff; if (ch == 99) shiftstate |= 1; else if (ch == 110) shiftstate |= 2; else if (ch == 227) shiftstate &= ~1; else if (ch == 238) shiftstate &= ~2; //printk("getch: %d\n", ch); } // If release, wait for key press while ((ch & 0x80) == 0x80 || ch == 238 || ch == 227); //printk("getch rel: %d\n", ch); ch &= 0x7f; if (shiftstate) ch = sunkbd_keycode_shifted[ch]; else ch = sunkbd_keycode[ch]; //printk("getch xlate: %d\n", ch); return ch;}
I don't see how this would loop, given that kbd_dev is a pointer to volatile.
Any further comments? I'd like to get these last couple of patchsets applied fairly soon since it's QEMU freeze starting next week.
I'd use your workaround then? But something else is going on, it would be good to figure out what.
Okay I've gone with this approach for now, probably one to dig into when I have a bit more time. Applied to master.
ATB,
Mark.
-
Mark Cave-Ayland -
Segher Boessenkool