Possible bug in keyboard.c (2.6.10)

Possible bug in keyboard.c (2.6.10)

[Sasa Stevanovic]

Hi,

I had some problems with my laptop's onetouch keys and it eventually led me to 
keyboard.c file from 2.6.10 kernel (Vojtech Pavlik and others).  There may be 
a bug in the file, please read below.

Well, actually, when all omnibook/messages/setkeycodes/hotkeys/xev/showkey etc 
stuff is stripped off, what remains is that x86_keycodes array has only first 
240 members initialized, while remaining 16 are set to 0 due to [256]:

static unsigned short x86_keycodes[256] = { <only 240 here> };

(For my scenario, workaround was possible.)

I am not sure if this is a bug or not; it worked in 2.4.18 without workaround. 
Might be that someone wanted to prevent reading invalid memory.  There are 
many versions of the file/array definition found on the web, none of which has 
a comment about this.

Please also use my email address <mg94c18@alas.matf.bg.ac.yu> if you respond 
to this.  I am a member but not sure for how long (depends on number of 
messages/day).

Thanks,
Sasa

Re: Possible bug in keyboard.c (2.6.10)

[Dmitry Torokhov]

On Wednesday 26 January 2005 22:16, Sasa Stevanovic wrote:
> Hi,
> 
> I had some problems with my laptop's onetouch keys and it eventually led me to 
> keyboard.c file from 2.6.10 kernel (Vojtech Pavlik and others).  There may be 
> a bug in the file, please read below.
> 
> Well, actually, when all omnibook/messages/setkeycodes/hotkeys/xev/showkey etc 
> stuff is stripped off, what remains is that x86_keycodes array has only first 
> 240 members initialized, while remaining 16 are set to 0 due to [256]:
> 
> static unsigned short x86_keycodes[256] = { <only 240 here> };
> 
> (For my scenario, workaround was possible.)
> 
> I am not sure if this is a bug or not; it worked in 2.4.18 without workaround. 
> Might be that someone wanted to prevent reading invalid memory.  There are 
> many versions of the file/array definition found on the web, none of which has 
> a comment about this.
> 

>From Vojetch Pavilk:
> I'm sorry, but X only understands the RAW PS/2 protocol, and that one
>  can only transport keycodes up to 240.
>  
>  For keycodes above 240, XFree86 would either need to use the MediumRAW
>  mode, or use event devices for parsing the keyboard.
http://www.ussg.iu.edu/hypermail/linux/kernel/0406.0/0544.html

You still did not describe what kind of problems you are having with your
keys.

-- 
Dmitry

Re: Possible bug in keyboard.c (2.6.10)

[Sasa Stevanovic]

On Wed, 26 Jan 2005, Dmitry Torokhov wrote:

> On Wednesday 26 January 2005 22:16, Sasa Stevanovic wrote:
>> Hi,
>>
>> I had some problems with my laptop's onetouch keys and it eventually led me to
>> keyboard.c file from 2.6.10 kernel (Vojtech Pavlik and others).  There may be
>> a bug in the file, please read below.
>>
>> Well, actually, when all omnibook/messages/setkeycodes/hotkeys/xev/showkey etc
>> stuff is stripped off, what remains is that x86_keycodes array has only first
>> 240 members initialized, while remaining 16 are set to 0 due to [256]:
>>
>> static unsigned short x86_keycodes[256] = { <only 240 here> };
>>
>> (For my scenario, workaround was possible.)
>>
>> I am not sure if this is a bug or not; it worked in 2.4.18 without workaround.
>> Might be that someone wanted to prevent reading invalid memory.  There are
>> many versions of the file/array definition found on the web, none of which has
>> a comment about this.
>>
>
> From Vojetch Pavilk:
>> I'm sorry, but X only understands the RAW PS/2 protocol, and that one
>>  can only transport keycodes up to 240.
>>
>>  For keycodes above 240, XFree86 would either need to use the MediumRAW
>>  mode, or use event devices for parsing the keyboard.
> http://www.ussg.iu.edu/hypermail/linux/kernel/0406.0/0544.html
>
> You still did not describe what kind of problems you are having with your
> keys.
>
> -- 
> Dmitry
>

That's ok, I did not post the message to solve my problem, I posted because I 
though it was a possible bug.  I already worked around the problem by using 
setkeycodes to use keycodes (say) 233 and 234 instead of 243 and 244, then I 
also changed the upper-layer software to expect 233 and 234 and execute 
actions that would normally be executed if 243 or 244 was pressed.

Thank you for the information and the useful link.

P.S.  The problem was

atkbd.c: Unknown key pressed (translated set 2, code 0xf4 on isa0060/serio0). 
atkbd.c: Use 'setkeycodes e074 <keycode>' to make it known

Then after using setkeycodes e074 244 that's correct code for my computer 
(Omnibook XE3 GF), I had

keyboard.c: can't emulate rawmode for keycode 244

Sasa

Re: Possible bug in keyboard.c (2.6.10)

[Andries Brouwer]

On Thu, Jan 27, 2005 at 04:16:14AM +0100, Sasa Stevanovic wrote:

> I had some problems with my laptop's onetouch keys and it eventually led me 
> to keyboard.c file from 2.6.10 kernel (Vojtech Pavlik and others).  There 
> may be a bug in the file, please read below.
> 
> Well, actually, when all omnibook/messages/setkeycodes/hotkeys/xev/showkey 
> etc stuff is stripped off, what remains is that x86_keycodes array has only 
> first 240 members initialized, while remaining 16 are set to 0 due to [256]:
> 
> static unsigned short x86_keycodes[256] = { <only 240 here> };
> 
> (For my scenario, workaround was possible.)
> 
> I am not sure if this is a bug or not; it worked in 2.4.18 without 
> workaround. Might be that someone wanted to prevent reading invalid memory. 
> There are many versions of the file/array definition found on the web, none 
> of which has a comment about this.

You only express surprise at the initialization but do not
give details about what goes wrong for you.

If you want to test what scancodes your keyboard produces,
it is better to boot 2.4. From 2.6 one gets some peculiar
synthetic "raw" mode where keys have been translated back
and forth a few times via non-invertible mappings.

What happens today can be read in input/keyboard/atkbd.c:
First the scancode sent by the keyboard is "untranslated"
using the array atkbd_unxlate_table[]. For example, the
Home key produces e0 1c and is untranslated to 128+90.
Then the untranslated value is found in atkbd_set2_keycode[]
and we find atkbd_set2_keycode[128+90] = 96.
In raw mode this 96 must be converted back, and that is
done in keyboard.c:emulate_raw() where x86_keycodes[96] = 284
and 284 = 256 + 28, and we produce e0 1c.
(Most numbers in decimal, scancodes in hex.)

So x86_keycodes[] only needs values at indices 240-255
when these occur as keycodes, and they don't, I think.
(There is a 255 at indices 128+18 and 128+89, corresponding to
scancodes e0 2a / e0 36 - fake LShift / fake RShift.
In "raw" mode these just vanish into thin air.)

On the other hand, the array atkbd_set2_keycode[] can be set
by the user, and then x86_keycodes[] just produces garbage.

In short - raw mode in 2.6 is badly broken.

Andries


(For usb, see usb_kbd_keycode[] in usbkbd.c)

Re: Possible bug in keyboard.c (2.6.10)

[Roman Zippel]

Hi,

On Thu, 27 Jan 2005, Andries Brouwer wrote:

> In short - raw mode in 2.6 is badly broken.

I think not just that. The whole keyboard input layer needs some serious 
review. Just the complete lack of any locking is frightening, I'd really 
like to know how the input layer could become the standard. I tried to 
find a few times to find any discussion about the input layer design, but 
I couldn't find anything.

Some of my favourites in the input layer:
- the keyboard sound/led handling: the keyboard driver basically fakes 
events for the device and input_event() is "clever" enough to also tell 
the device about it. This is quite an abuse of event system for general 
device/driver communication.
- a single input device structure for all types: this structure is quite 
big, where most of its contents is irrelevant for most devices.
- fine grained matching/filtering: I have no idea why the input layer has 
to do this down to the single event instead of just the event type.

Vojtech, could you please explain a bit the reason for the above and what 
are your plans to e.g. fix the locking?

bye, Roman

Re: Possible bug in keyboard.c (2.6.10)

[Vojtech Pavlik]

On Fri, Jan 28, 2005 at 01:39:08AM +0100, Roman Zippel wrote:
> Hi,
> 
> On Thu, 27 Jan 2005, Andries Brouwer wrote:
> 
> > In short - raw mode in 2.6 is badly broken.
> 
> I think not just that. The whole keyboard input layer needs some serious 
> review. Just the complete lack of any locking is frightening, I'd really 
> like to know how the input layer could become the standard.

I'm very sorry about the locking, but the thing grew up in times of
kernel 2.0, which didn't require any locking. There are a few possible
races with device registration/unregistration, and it's on my list to
fix that, however under normal operation there shouldn't be any need for
locks, as there are no complex structures built that'd become
inconsistent. 

If you find scenarios which will lead to trouble in the event delivery
system, please tell me, and I'll try to fix that as soon as possible.

> I tried to find a few times to find any discussion about the input
> layer design, but I couldn't find anything.

You have to search in very old archives. There was quite a lot of it,
and it was going off on a lot of tangents. In the end, I just wrote it.

> Some of my favourites in the input layer:
> - the keyboard sound/led handling: the keyboard driver basically fakes 
> events for the device and input_event() is "clever" enough to also tell 
> the device about it. This is quite an abuse of event system for general 
> device/driver communication.

The intention here is that we have two types of events, input and
output. Most events are input (REL, ABS, ...), while some travel the
opposite direction. For simplicity, the interface is the same -
input_event(), which then, based on the event type decides where to
forward it - whether up or down the stream (or both, where other users
of the device may be interested in the change).

> - a single input device structure for all types: this structure is quite 
> big, where most of its contents is irrelevant for most devices.

I actually think this is a big plus. 

Real word devices cannot be confined into predefined structures, as
hardware develops, mice get more buttons, wheels, force feedback, 

The structure, if the size is a problem, can be made smaller by having
the larger bitmaps allocated separately.

> - fine grained matching/filtering: I have no idea why the input layer has 
> to do this down to the single event instead of just the event type.

I wonder what do you mean by this, the layer itself doesn't have any
codepaths dependent directly on event code, just on the types.

If you wonder why the input_event() function checks whether the event
generated by a device really is possible for that device and ignores it
if not, that's to make the drivers life easier by, in the example of a
PS/2 mouse always reporting the state of the middle button, even when
the PS/2 mouse is a 2-button mouse. The driver only needs to say that
the middle button doesn't exist in the bitmap setup and the packet
processing routine doesn't need to care about it.

And if you wonder whether struct input_dev needs to even know what event
codes for each type are generated by the device - that's there to tell
the event handlers (whether kernel or userspace), so they will know what
to expect and can make decisions based on it.

> Vojtech, could you please explain a bit the reason for the above and what 
> are your plans to e.g. fix the locking?

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: Possible bug in keyboard.c (2.6.10)

[Al Viro]

On Fri, Jan 28, 2005 at 11:59:37AM +0100, Vojtech Pavlik wrote:
> I'm very sorry about the locking, but the thing grew up in times of
> kernel 2.0, which didn't require any locking. There are a few possible

Incorrect.  You have blocking allocations in critical areas and they
required locking all way back.

> races with device registration/unregistration, and it's on my list to
> fix that, however under normal operation there shouldn't be any need for
> locks, as there are no complex structures built that'd become
> inconsistent. 

Um-hm...  Vojtech, meet USB mouse; USB mouse, meet Vojtech.  Now watch
a disconnect and reconnect happening when luser suddenly gets overexcited
and jerks the wrong hand a bit too hard while browsing the most profitable
sort of website...

> If you find scenarios which will lead to trouble in the event delivery
> system, please tell me, and I'll try to fix that as soon as possible.

See above.  Devices appearing and disappearing *are* normal.  

Re: Possible bug in keyboard.c (2.6.10)

[Vojtech Pavlik]

On Sat, Jan 29, 2005 at 04:50:55AM +0000, Al Viro wrote:

> > I'm very sorry about the locking, but the thing grew up in times of
> > kernel 2.0, which didn't require any locking. There are a few possible
> 
> Incorrect.  You have blocking allocations in critical areas and they
> required locking all way back.

Ok. I see a problem where input_register_device() calls input handler
connect methods, which do kmalloc(). This would be bad even on 2.0.

Anything else? I believe the ->open()/->release() methods are still
protected.

> > races with device registration/unregistration, and it's on my list to
> > fix that, however under normal operation there shouldn't be any need for
> > locks, as there are no complex structures built that'd become
> > inconsistent. 
> 
> Um-hm...  Vojtech, meet USB mouse; USB mouse, meet Vojtech.  Now watch
> a disconnect and reconnect happening when luser suddenly gets overexcited
> and jerks the wrong hand a bit too hard while browsing the most profitable
> sort of website...

I know. As I said, this is a problem I know about, and will be fixed. I
was mainly interested whether anyone sees further problems in scenarios
which don't include device addition/removal.

We already fixed this in serio, and input and gameport are next in the
list.

> > If you find scenarios which will lead to trouble in the event delivery
> > system, please tell me, and I'll try to fix that as soon as possible.
> 
> See above.  Devices appearing and disappearing *are* normal.  

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: Possible bug in keyboard.c (2.6.10)

[Dmitry Torokhov]

On Saturday 29 January 2005 06:25, Vojtech Pavlik wrote:
> On Sat, Jan 29, 2005 at 04:50:55AM +0000, Al Viro wrote:
> 
> > > I'm very sorry about the locking, but the thing grew up in times of
> > > kernel 2.0, which didn't require any locking. There are a few possible
> > 
> > Incorrect.  You have blocking allocations in critical areas and they
> > required locking all way back.
> 
> Ok. I see a problem where input_register_device() calls input handler
> connect methods, which do kmalloc(). This would be bad even on 2.0.
> 
> Anything else? I believe the ->open()/->release() methods are still
> protected.
> 

evdev, tsdev, mousedev, joydev need to protect their client lists because
interrupt could try to deliver event to already deleted device (client)
.
> > > races with device registration/unregistration, and it's on my list to
> > > fix that, however under normal operation there shouldn't be any need for
> > > locks, as there are no complex structures built that'd become
> > > inconsistent. 
> > 
> > Um-hm...  Vojtech, meet USB mouse; USB mouse, meet Vojtech.  Now watch
> > a disconnect and reconnect happening when luser suddenly gets overexcited
> > and jerks the wrong hand a bit too hard while browsing the most profitable
> > sort of website...
> 
> I know. As I said, this is a problem I know about, and will be fixed. I
> was mainly interested whether anyone sees further problems in scenarios
> which don't include device addition/removal.
> 
> We already fixed this in serio, and input and gameport are next in the
> list.
>

For the record I am still working on gameport conversion, just did not have
enough time lately...
 
> > > If you find scenarios which will lead to trouble in the event delivery
> > > system, please tell me, and I'll try to fix that as soon as possible.
> > 
> > See above.  Devices appearing and disappearing *are* normal.  
> 

-- 
Dmitry

Re: Possible bug in keyboard.c (2.6.10)

[Vojtech Pavlik]

On Sat, Jan 29, 2005 at 06:35:59PM -0500, Dmitry Torokhov wrote:
> On Saturday 29 January 2005 06:25, Vojtech Pavlik wrote:
> > On Sat, Jan 29, 2005 at 04:50:55AM +0000, Al Viro wrote:
> > 
> > > > I'm very sorry about the locking, but the thing grew up in times of
> > > > kernel 2.0, which didn't require any locking. There are a few possible
> > > 
> > > Incorrect.  You have blocking allocations in critical areas and they
> > > required locking all way back.
> > 
> > Ok. I see a problem where input_register_device() calls input handler
> > connect methods, which do kmalloc(). This would be bad even on 2.0.
> > 
> > Anything else? I believe the ->open()/->release() methods are still
> > protected.
> > 
> 
> evdev, tsdev, mousedev, joydev need to protect their client lists because
> interrupt could try to deliver event to already deleted device (client)

Oh, of course. The protection doesn't apply to the event routine.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: Possible bug in keyboard.c (2.6.10)

[Al Viro]

On Sat, Jan 29, 2005 at 12:25:10PM +0100, Vojtech Pavlik wrote:
> I know. As I said, this is a problem I know about, and will be fixed. I
> was mainly interested whether anyone sees further problems in scenarios
> which don't include device addition/removal.
> 
> We already fixed this in serio, and input and gameport are next in the
> list.

OK, I'll bite.  What's to guarantee that no events will happen in
the middle of serio_unregister_port(), right after we'd done
serio_remove_pending_events()?

Re: Possible bug in keyboard.c (2.6.10)

[Dmitry Torokhov]

On Sunday 30 January 2005 03:41, Al Viro wrote:
> On Sat, Jan 29, 2005 at 12:25:10PM +0100, Vojtech Pavlik wrote:
> > I know. As I said, this is a problem I know about, and will be fixed. I
> > was mainly interested whether anyone sees further problems in scenarios
> > which don't include device addition/removal.
> > 
> > We already fixed this in serio, and input and gameport are next in the
> > list.
> 
> OK, I'll bite.  What's to guarantee that no events will happen in
> the middle of serio_unregister_port(), right after we'd done
> serio_remove_pending_events()?

At this point serio is disconnected from driver and serio_interrupt
will only queue rescans only if serio->registered. I guess I will need
to protect change to serio->registered and take serio->lock to be
completely in clear.

Thanks for pointing this out.

-- 
Dmitry

Re: Possible bug in keyboard.c (2.6.10)

[Dmitry Torokhov]

On Sunday 30 January 2005 18:21, Dmitry Torokhov wrote:
> On Sunday 30 January 2005 03:41, Al Viro wrote:
> > On Sat, Jan 29, 2005 at 12:25:10PM +0100, Vojtech Pavlik wrote:
> > > I know. As I said, this is a problem I know about, and will be fixed. I
> > > was mainly interested whether anyone sees further problems in scenarios
> > > which don't include device addition/removal.
> > > 
> > > We already fixed this in serio, and input and gameport are next in the
> > > list.
> > 
> > OK, I'll bite.  What's to guarantee that no events will happen in
> > the middle of serio_unregister_port(), right after we'd done
> > serio_remove_pending_events()?
> 
> At this point serio is disconnected from driver and serio_interrupt
> will only queue rescans only if serio->registered. I guess I will need
> to protect change to serio->registered and take serio->lock to be
> completely in clear.
> 
> Thanks for pointing this out.
>

Oh, I just realized that this piece is not in main tree yet. You can
check the version that I am pushing to Vojtech here:

	bk pull bk://dtor.bkbits.net/input

We still not agreed on need for start/stop methods though...

-- 
Dmitry

Re: Possible bug in keyboard.c (2.6.10)

[Dmitry Torokhov]

On Sunday 30 January 2005 18:21, Dmitry Torokhov wrote:
> On Sunday 30 January 2005 03:41, Al Viro wrote:
> > On Sat, Jan 29, 2005 at 12:25:10PM +0100, Vojtech Pavlik wrote:
> > > I know. As I said, this is a problem I know about, and will be fixed. I
> > > was mainly interested whether anyone sees further problems in scenarios
> > > which don't include device addition/removal.
> > > 
> > > We already fixed this in serio, and input and gameport are next in the
> > > list.
> > 
> > OK, I'll bite.  What's to guarantee that no events will happen in
> > the middle of serio_unregister_port(), right after we'd done
> > serio_remove_pending_events()?
> 
> At this point serio is disconnected from driver and serio_interrupt
> will only queue rescans only if serio->registered. I guess I will need
> to protect change to serio->registered and take serio->lock to be
> completely in clear.
> 

Thinking about it some more - serio driver should take care of shutting
off interrupt delivery either in ->close() or ->stop() methods so we
don't really need to worry about having new events delivered here.

-- 
Dmitry

Re: Possible bug in keyboard.c (2.6.10)

[Roman Zippel]

Hi,

On Fri, 28 Jan 2005, Vojtech Pavlik wrote:

> I'm very sorry about the locking, but the thing grew up in times of
> kernel 2.0, which didn't require any locking. There are a few possible
> races with device registration/unregistration, and it's on my list to
> fix that, however under normal operation there shouldn't be any need for
> locks, as there are no complex structures built that'd become
> inconsistent. 

I wouldn't say that there is no need for that, but that these events are 
rather rare, but it can happen. Unfortunately the lack of locking is all 
over the keyboard/vc/tty layer. It would have been nice if input had 
introduced some improvement here.
This seriously becomes a problem as soon we want to allow the user to 
dynamically attach/detach devices.

> > I tried to find a few times to find any discussion about the input
> > layer design, but I couldn't find anything.
> 
> You have to search in very old archives. There was quite a lot of it,
> and it was going off on a lot of tangents. In the end, I just wrote it.

How old and which archives?

> > - a single input device structure for all types: this structure is quite 
> > big, where most of its contents is irrelevant for most devices.
> 
> I actually think this is a big plus. 
> 
> Real word devices cannot be confined into predefined structures, as
> hardware develops, mice get more buttons, wheels, force feedback, 

That doesn't necessarilly mean we have to pack everything into a single 
structure. E.g. we present pci boards with multiple functions as multiple 
devices, the same can be done for input devices. More important is that 
kernel drivers only expect a certain class of devices, e.g. the keyboard 
driver only needs the keyboard related parts and would also allow us to 
add more keyboard specific callbacks instead of sending events.

> > Some of my favourites in the input layer:
> > - the keyboard sound/led handling: the keyboard driver basically fakes 
> > events for the device and input_event() is "clever" enough to also tell 
> > the device about it. This is quite an abuse of event system for general 
> > device/driver communication.
> 
> The intention here is that we have two types of events, input and
> output. Most events are input (REL, ABS, ...), while some travel the
> opposite direction. For simplicity, the interface is the same -
> input_event(), which then, based on the event type decides where to
> forward it - whether up or down the stream (or both, where other users
> of the device may be interested in the change).

This is rather a hint that the abstraction is wrong, e.g. why not send 
sound events to the pckbd _handler_? A device should just send input 
events and handlers handle them and with sysfs we should actually rename 
the latter to input_drivers (it's less confusing this way).

Some other problems I have with the current design:
- unified keyboard/mouse device: one rather annoying problem, which is 
neither solved by the input system or the linuxconsole patches, is that 
one can't use keyboards with different mappings (e.g. german and english 
is what I have here). I have a patch which makes the keymap data more 
dynamic and assigns it to keyboard device, which has the positive side 
effect that all keyboards don't have to send the same keycodes anymore 
(and we don't have to break all the keyboards anymore which don't use AT 
style keycodes).
I'm not convinced we need this unifying layer in the kernel. We need a 
keyboard driver in the kernel for the tty layer, but we have no kernel 
user for mouse or joystick events, so why not do some simple preprocessing 
and leave the rest to userspace?
- kbd raw mode: the emulation code should really go. I'm playing with the 
idea of merging input and serio infrastructure (or basically turning serio 
devices into (raw) input devices), the differences are not that big (at 
least the management part, the event path might still differ). This way 
the keyboard driver can ask the kbd device for the raw device and read 
the events directly.

> > - fine grained matching/filtering: I have no idea why the input layer has 
> > to do this down to the single event instead of just the event type.
> 
> I wonder what do you mean by this, the layer itself doesn't have any
> codepaths dependent directly on event code, just on the types.

E.g. also input_match_device.

> If you wonder why the input_event() function checks whether the event
> generated by a device really is possible for that device and ignores it
> if not, that's to make the drivers life easier by, in the example of a
> PS/2 mouse always reporting the state of the middle button, even when
> the PS/2 mouse is a 2-button mouse. The driver only needs to say that
> the middle button doesn't exist in the bitmap setup and the packet
> processing routine doesn't need to care about it.

I don't really see what's the point of this.
Such functionality is only needed by the minority of drivers, but 
increases setup complexity, bloats structures and adds unnecessary runtime 
test for everyone. Why not leave it to the few places where it actually 
might be needed? If it basically has only informal value, there are other 
ways to export this.

bye, Roman

Re: Possible bug in keyboard.c (2.6.10)

[Vojtech Pavlik]

On Sat, Jan 29, 2005 at 01:11:08PM +0100, Roman Zippel wrote:

> > I'm very sorry about the locking, but the thing grew up in times of
> > kernel 2.0, which didn't require any locking. There are a few possible
> > races with device registration/unregistration, and it's on my list to
> > fix that, however under normal operation there shouldn't be any need for
> > locks, as there are no complex structures built that'd become
> > inconsistent. 
> 
> I wouldn't say that there is no need for that, but that these events are 
> rather rare, but it can happen.

Sorry, I meant to say that of course locking is needed for protecting
the device and handler lists, but that the event delivery path itself
is in my opinion safe, and that there is no need to eg. lock
input_event() against concurrent use by different callers.

> Unfortunately the lack of locking is all 
> over the keyboard/vc/tty layer. It would have been nice if input had 
> introduced some improvement here.
> This seriously becomes a problem as soon we want to allow the user to 
> dynamically attach/detach devices.

I agree, and I plan to fix the locking at least in input and keyboard.
How far it'll go into vc/tty I can't promise.`

> > > I tried to find a few times to find any discussion about the input
> > > layer design, but I couldn't find anything.
> > 
> > You have to search in very old archives. There was quite a lot of it,
> > and it was going off on a lot of tangents. In the end, I just wrote it.
> 
> How old and which archives?

1999 I think, some discussion was on L-K, the rest on
linux-input@atrey.karlin.mff.cuni.cz, which, unfortunately, doesn't have
a web-based archive.

> > > - a single input device structure for all types: this structure is quite 
> > > big, where most of its contents is irrelevant for most devices.
> > 
> > I actually think this is a big plus. 
> > 
> > Real word devices cannot be confined into predefined structures, as
> > hardware develops, mice get more buttons, wheels, force feedback, 
> 
> That doesn't necessarilly mean we have to pack everything into a single 
> structure. E.g. we present pci boards with multiple functions as multiple 
> devices, the same can be done for input devices. More important is that 
> kernel drivers only expect a certain class of devices, e.g. the keyboard 
> driver only needs the keyboard related parts and would also allow us to 
> add more keyboard specific callbacks instead of sending events.

The USB HID devices often are crossing the device type boundaries. A
keyboard with a few mouse buttons, for example. Yes, we could split it,
but I really doubt the gain.

The GGI people went the way of predefined device types ...

> > The intention here is that we have two types of events, input and
> > output. Most events are input (REL, ABS, ...), while some travel the
> > opposite direction. For simplicity, the interface is the same -
> > input_event(), which then, based on the event type decides where to
> > forward it - whether up or down the stream (or both, where other users
> > of the device may be interested in the change).
> 
> This is rather a hint that the abstraction is wrong, e.g. why not send 
> sound events to the pckbd _handler_? A device should just send input 
> events and handlers handle them and with sysfs we should actually rename 
> the latter to input_drivers (it's less confusing this way).

You imply that the atkbd module would register itself both as an input
device, and as an input handler? That's quite an interesting idea I
haven't thought about before.

But then all the handlers also have to register themselves as devices,
for generating the LED and Sound events. Probably we then could get rid
of the handlers altogether and have devices only, which would both send
and receive events.

It would work, but to me this looks even more confusing. 

> Some other problems I have with the current design:
> - unified keyboard/mouse device: one rather annoying problem, which is 
> neither solved by the input system or the linuxconsole patches, is that 
> one can't use keyboards with different mappings (e.g. german and english 
> is what I have here).

The unified mouse device was planned for backward compatibility only,
unfortunately it stuck, since for userspace that was the easiest way to
deal with hotplug.

The single keyboard similarly was the easiest way how to plug the input
into keyboard.c. James Simmons was working on the vc part of the code,
but it never made it there. It sort of lives in the linuxconsole
patches, but was never really finished either.

> I have a patch which makes the keymap data more 
> dynamic and assigns it to keyboard device,

Cool! How do you load the keymaps for the specific devices? How do you
assign the devices to vcs? Can a user have per-vc keymaps?

> which has the positive side effect that all keyboards don't have to
> send the same keycodes anymore (and we don't have to break all the
> keyboards anymore which don't use AT style keycodes).

Well, I think the fact that the input layer uses an unified set of
numbers for the keys really helps in a lot of places. One is that you
don't need (architectures * languages) of keymaps and only can define
language keymaps.

It's a sort of pnmtools approach. The X people tried to solve this with
xkb, and it got very complex. I believe the two stage thing is much
easier to work with.

> I'm not convinced we need this unifying layer in the kernel. We need a 
> keyboard driver in the kernel for the tty layer, but we have no kernel 
> user for mouse or joystick events, so why not do some simple preprocessing 
> and leave the rest to userspace?

We could do that. We could even pass raw HID reports to userspace like
BSDs do, and have the userspace decode the data in there.

We would need a lot of different interfaces for that, for each kind of a
device. I'm not sure we'd even save kernel code doing that.

Btw, aside from parsing the data layout and assigning proper codes to
the events, the kernel tries to do as little processing on the actual
data as possible.

> - kbd raw mode: the emulation code should really go. I'm playing with the 
> idea of merging input and serio infrastructure (or basically turning serio 
> devices into (raw) input devices), the differences are not that big (at 
> least the management part, the event path might still differ). This way 
> the keyboard driver can ask the kbd device for the raw device and read 
> the events directly.

Not all input devices are serio based. Only few are. In current 2.6, the
atkbd.c driver sends both MSC_RAW and MSC_SCAN events, representing the
raw data and scancodes for the keys. Keyboard.c can use that to do raw
mode instead of generating a fake one.

Similarly, hid-input.c can send MSC_SCAN events containing HID usage
codes - the scancodes for HID.

Other drivers can implement the same.

However, I'm sure that X won't be happy to receive anything but the
single rawmode it expects.

A question for you: What is raw mode good for? (I'd like the emulation
to go, and not be replaced by anything at all.)

> > > - fine grained matching/filtering: I have no idea why the input layer has 
> > > to do this down to the single event instead of just the event type.
> > 
> > I wonder what do you mean by this, the layer itself doesn't have any
> > codepaths dependent directly on event code, just on the types.
> 
> E.g. also input_match_device.

That's there for the handlers to be able to decide whether a device is
interesting for them. Since we don't have device type information (and
USB HID devices will necessarily not provide us with it, although often
they do), we can't simply assign handlers based on the device type.

> I don't really see what's the point of this.  Such functionality is
> only needed by the minority of drivers, but increases setup
> complexity, bloats structures and adds unnecessary runtime test for
> everyone. Why not leave it to the few places where it actually might
> be needed? If it basically has only informal value, there are other
> ways to export this.

The setup of the bitfields is there _primarily_ for userspace. In the
end, I'd like the only two handlers that will stay to be keyboard.c for
console and evdev.c for everything else.

Then the userspace will need to know what the device is, which features
it has, and how to handle it. We have mice, touchpads, touchpoints,
touchscreens, keyboards, tablets, joysticks, ... and each need special
care. Some touchpads have palm detection, other don't. Some mice don't
have three buttons, the middle button emulation is desired in that case.

All this information is stored in the bitmaps.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: Possible bug in keyboard.c (2.6.10)

[Roman Zippel]

Hi,

On Sat, 29 Jan 2005, Vojtech Pavlik wrote:

> > That doesn't necessarilly mean we have to pack everything into a single 
> > structure. E.g. we present pci boards with multiple functions as multiple 
> > devices, the same can be done for input devices. More important is that 
> > kernel drivers only expect a certain class of devices, e.g. the keyboard 
> > driver only needs the keyboard related parts and would also allow us to 
> > add more keyboard specific callbacks instead of sending events.
> 
> The USB HID devices often are crossing the device type boundaries. A
> keyboard with a few mouse buttons, for example. Yes, we could split it,
> but I really doubt the gain.

Why?

> The GGI people went the way of predefined device types ...

And what is this supposed to tell me?

> > This is rather a hint that the abstraction is wrong, e.g. why not send 
> > sound events to the pckbd _handler_? A device should just send input 
> > events and handlers handle them and with sysfs we should actually rename 
> > the latter to input_drivers (it's less confusing this way).
> 
> You imply that the atkbd module would register itself both as an input
> device, and as an input handler? That's quite an interesting idea I
> haven't thought about before.

That's one idea I'm playing with, but the keyboard.c needs a serious 
cleanup first.

> But then all the handlers also have to register themselves as devices,
> for generating the LED and Sound events. Probably we then could get rid
> of the handlers altogether and have devices only, which would both send
> and receive events.
> 
> It would work, but to me this looks even more confusing. 

No, you have to clearly define the path of events, if you want to a two 
way commucation via input events, you would need two devcies and two 
drivers, but I doubt we really want that.
E.g. led handling should be no events at all, they are actually properties 
of the keyboard device and if the keyboard driver wants to set them, it 
should just do dev->setleds(...). I know of at least two keyboards (an 
Amiga and a Mac keyboard), where the caps status (and therefore the caps 
led) cannot be changed via software.

> > I have a patch which makes the keymap data more 
> > dynamic and assigns it to keyboard device,
> 
> Cool! How do you load the keymaps for the specific devices? How do you
> assign the devices to vcs?

The loading part is still missing. Right now I only concentrate on killing 
the global data in keyboard.c

> Can a user have per-vc keymaps?

Why would you want this?

> > which has the positive side effect that all keyboards don't have to
> > send the same keycodes anymore (and we don't have to break all the
> > keyboards anymore which don't use AT style keycodes).
> 
> Well, I think the fact that the input layer uses an unified set of
> numbers for the keys really helps in a lot of places. One is that you
> don't need (architectures * languages) of keymaps and only can define
> language keymaps.

For new devices that maybe okay, but that doesn't mean we have to break 
old devices, which were working just fine.
A small tool that converts from mapping to another is probably easier and 
it doesn't completely removes the need for arch specific keymaps, e.g. my 
Amiga has no NumLock key, so I use a different shift key two switch to the 
alternative keypad mapping. Assuming all the world is a PC never really 
worked, if you want to write portable code.

> However, I'm sure that X won't be happy to receive anything but the
> single rawmode it expects.
> 
> A question for you: What is raw mode good for? (I'd like the emulation
> to go, and not be replaced by anything at all.)

I don't mind fixing X, but why not make raw events available if they 
exists (if only for debugging), if they have to be emulated, you can still 
create an emulation device, which can be feed them back to the keyboard 
driver.

> > > > - fine grained matching/filtering: I have no idea why the input layer has 
> > > > to do this down to the single event instead of just the event type.
> > > 
> > > I wonder what do you mean by this, the layer itself doesn't have any
> > > codepaths dependent directly on event code, just on the types.
> > 
> > E.g. also input_match_device.
> 
> That's there for the handlers to be able to decide whether a device is
> interesting for them. Since we don't have device type information (and
> USB HID devices will necessarily not provide us with it, although often
> they do), we can't simply assign handlers based on the device type.

Why can't you do this in the connect method? Most drivers should just care 
about the device type and take all events of a certain type and pass it on 
to userspace somehow.

> The setup of the bitfields is there _primarily_ for userspace.

So why not export it via sysfs?

> Then the userspace will need to know what the device is, which features
> it has, and how to handle it. We have mice, touchpads, touchpoints,
> touchscreens, keyboards, tablets, joysticks, ... and each need special
> care. Some touchpads have palm detection, other don't. Some mice don't
> have three buttons, the middle button emulation is desired in that case.

I don't mind device properties, but do we have to do this in such great 
detail? A lot of simple devices don't have this information, why force 
them to make this information up? Not all mouse drivers actually know 
whether the mouse has a third mouse button. If a device has extra 
information, then simply export it via sysfs, but it's not needed at 
runtime.

Kernel drivers only care about whether a device generates 
key/mouse/joystick/... events, and this doesn't require detailed 
bitfields, e.g. in the case of a keyboard the keybit array is just 
duplicated information from keycode array (which means extra code just to 
keep them in sync).

bye, Roman

Re: Possible bug in keyboard.c (2.6.10)

[Vojtech Pavlik]

On Fri, Jan 28, 2005 at 01:39:08AM +0100, Roman Zippel wrote:

> On Thu, 27 Jan 2005, Andries Brouwer wrote:
> 
> > In short - raw mode in 2.6 is badly broken.
 
And, btw, raw mode in 2.6 is not badly broken. It works as it is
intended to. If you want the 2.4 behavior on x86, you just need to
specify "atkbd.softraw=0" on the kernel command line.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: Possible bug in keyboard.c (2.6.10)

[Andries Brouwer]

On Fri, Jan 28, 2005 at 12:10:05PM +0100, Vojtech Pavlik wrote:

> And, btw, raw mode in 2.6 is not badly broken. It works as it is
> intended to. If you want the 2.4 behavior on x86, you just need to
> specify "atkbd.softraw=0" on the kernel command line.

Thanks for pointing that out - I should have read patch-2.6.9 more
carefully. I'll add that to the setkeycodes.8 man page.

Nevertheless I disagree a bit. "raw mode" is by definition the mode
where scan codes are passed unmodified to user space.
So before 2.6.9 this was just broken, and since 2.6.9 it is broken
by default but there is a boot option to make it work.

What is the reason that you do not make this the default?
The current default is really messy and confusing, especially
when people have to map keys using setkeycodes.

Andries


BTW, now that I read the corresponding code:

        if (atkbd_softrepeat)
                atkbd_softraw = 1;

        if (!atkbd_softrepeat) {
                atkbd->dev.rep[REP_DELAY] = 250;
                atkbd->dev.rep[REP_PERIOD] = 33;
        } else atkbd_softraw = 1;

The "else" part is superfluous.

Re: Possible bug in keyboard.c (2.6.10)

[Vojtech Pavlik]

On Fri, Jan 28, 2005 at 10:59:39PM +0100, Andries Brouwer wrote:
> On Fri, Jan 28, 2005 at 12:10:05PM +0100, Vojtech Pavlik wrote:
> 
> > And, btw, raw mode in 2.6 is not badly broken. It works as it is
> > intended to. If you want the 2.4 behavior on x86, you just need to
> > specify "atkbd.softraw=0" on the kernel command line.
> 
> Thanks for pointing that out - I should have read patch-2.6.9 more
> carefully. I'll add that to the setkeycodes.8 man page.
> 
> Nevertheless I disagree a bit. "raw mode" is by definition the mode
> where scan codes are passed unmodified to user space.
> So before 2.6.9 this was just broken, and since 2.6.9 it is broken
> by default but there is a boot option to make it work.

The problem is that users wouldn't be happy if I passed USB usage codes
when they enable raw mode with an USB keyboard.

The expectation is that 'it will work'.

Because of that, the 'softraw=0' option _only_ works for AT and PS/2
keyboards, with any other keyboard type it has no effect.

> What is the reason that you do not make this the default?

To have all keyboard types behave the same, instead of having a single
exception, although I admit that the exception would be for the most
common case.

> The current default is really messy and confusing, especially
> when people have to map keys using setkeycodes.

The emulated rawmode is there mainly for X. If it weren't for X, I
wouldn't bother generating rawmode for keyboards other than PS/2, and
for PS/2 I'd have kept the true raw data there.

However, on 2.6, where you can have more than one keyboard, it'd be
better to use the EVIOCSKEYCODE ioctl on the event device instead of the
KDSKEYCODE ioctl on the console, as the later only changes the first
found keyboard.

Also, if setkeycodes used the event device, it'd get the raw scancodes
easily, as they're embedded in the event stream together with the
keycodes.

I'd hoped someone of the lineak/.../... multimedia key people will make
such an utility, but now I see that what one doesn't do himself, doesn't
happen. I will write it, possibly as a patch to setkeycodes.

> BTW, now that I read the corresponding code:
> 
>         if (atkbd_softrepeat)
>                 atkbd_softraw = 1;
> 
>         if (!atkbd_softrepeat) {
>                 atkbd->dev.rep[REP_DELAY] = 250;
>                 atkbd->dev.rep[REP_PERIOD] = 33;
>         } else atkbd_softraw = 1;
> 
> The "else" part is superfluous.

It indeed is. It's been removed, too.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: Possible bug in keyboard.c (2.6.10)

[Dmitry Torokhov]

On Saturday 29 January 2005 06:12, Vojtech Pavlik wrote:
> However, on 2.6, where you can have more than one keyboard, it'd be
> better to use the EVIOCSKEYCODE ioctl on the event device instead of the
> KDSKEYCODE ioctl on the console, as the later only changes the first
> found keyboard.
> 

FWIW I changed atkbd so every keyboard has separate keymap (so one can set
one keyboard to set 2 and other to set 3). I think it should be possible to
adjust keymaps on individual keyboards to accurately map keys when keyboards
are different.

-- 
Dmitry

Re: Possible bug in keyboard.c (2.6.10)

[Pavel Machek]

Hi!

> > In short - raw mode in 2.6 is badly broken.
> 
> I think not just that. The whole keyboard input layer needs some serious 
> review. Just the complete lack of any locking is frightening, I'd really 
> like to know how the input layer could become the standard. I tried to 
> find a few times to find any discussion about the input layer design, but 
> I couldn't find anything.

Actually, at one point we went over inputs with vojtech and tried to
fix the locking. We tried adding spinlocks, but it got really ugly
really quickly. Then we figured out that set_bit() and friends should
be enough to do spinlock-equivalent without getting that ugly.

We did not have enough energy to solve plug/unplug races at that
point; but other races should have been fixed. It is possible that we
overlooked something and doing set_bit() instead of spinlocks is
broken. If it is so, please tell us. 

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!