Suppressing softrepeat

Suppressing softrepeat

[Pete Zaitcev]

Add the "nosoftrepeat" parameter. This is useful if a "dumb" keyboard
has (unswitcheable) hardware repeat, like in Dell DRAC3.

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

---

Hi, Dmitry,

Dell people passed on a request to add a new parameter, "nosoftrepeat",
to i8042 (in atkbd.c). So, if I understand right, things should work so:

 - None set in grub.conf: Softrepeat is set by the driver as detected
   or derived. This is the default.
 - "softrepeat" set: Softrepeat is on, regardless
 - "nosoftrepeat" set: Softrepeat is off, regardless
 - Both "softrepeat" and "nosoftrepeat" are set: Do not do that.

The code looked confusing, but there is an good explanation in this bug:
 https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=181457

In short words, DRAC3 "plugs into" the keyboard connector, but does not
emulate output, so we detect this as "dumb" keyboard and enable softrepeat.
But softrepeat causes double keypresses.

I suppose we could revamp the old softrepeat parameter to be more than
a boolean, but I see no point.

If you agree, please include the attached.

-- Pete

--- linux-2.6.16-rc2/drivers/input/keyboard/atkbd.c	2006-02-11 00:31:41.000000000 -0800
+++ linux-2.6.16-rc2-lem/drivers/input/keyboard/atkbd.c	2006-02-21 11:53:53.000000000 -0800
@@ -50,6 +50,10 @@ static int atkbd_softrepeat;
 module_param_named(softrepeat, atkbd_softrepeat, bool, 0);
 MODULE_PARM_DESC(softrepeat, "Use software keyboard repeat");
 
+static int atkbd_nosoftrepeat;
+module_param_named(nosoftrepeat, atkbd_nosoftrepeat, bool, 0);
+MODULE_PARM_DESC(nosoftrepeat, "Do not use software keyboard repeat");
+
 static int atkbd_softraw = 1;
 module_param_named(softraw, atkbd_softraw, bool, 0);
 MODULE_PARM_DESC(softraw, "Use software generated rawmode");
@@ -862,7 +866,7 @@ static int atkbd_connect(struct serio *s
 	atkbd->softrepeat = atkbd_softrepeat;
 	atkbd->scroll = atkbd_scroll;
 
-	if (!atkbd->write)
+	if (!atkbd->write && !atkbd_nosoftrepeat)
 		atkbd->softrepeat = 1;
 
 	if (atkbd->softrepeat)

Re: Suppressing softrepeat

[Vojtech Pavlik]

On Tue, Feb 21, 2006 at 12:43:08PM -0800, Pete Zaitcev wrote:

> Add the "nosoftrepeat" parameter. This is useful if a "dumb" keyboard
> has (unswitcheable) hardware repeat, like in Dell DRAC3.

The softrepeat code should properly ignore all autorepeated keys from a
'dumb' keyboard. It's rather common that a keyboard we can't communicate
with is in autorepeat mode, because that's the mode AT keyboards wake up
in after power on.

A much simpler workaround for the DRAC3 is to set the softrepeat delay
to at least 750ms, using kbdrate(8), which will call the proper console
ioctl, resulting in updating the softrepeat parameters.

I prefer workarounds for problematic hardware done outside the kernel,
if possible.

> Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
> 
> ---
> 
> Hi, Dmitry,
> 
> Dell people passed on a request to add a new parameter, "nosoftrepeat",
> to i8042 (in atkbd.c). So, if I understand right, things should work so:
> 
>  - None set in grub.conf: Softrepeat is set by the driver as detected
>    or derived. This is the default.
>  - "softrepeat" set: Softrepeat is on, regardless
>  - "nosoftrepeat" set: Softrepeat is off, regardless
>  - Both "softrepeat" and "nosoftrepeat" are set: Do not do that.
> 
> The code looked confusing, but there is an good explanation in this bug:
>  https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=181457
> 
> In short words, DRAC3 "plugs into" the keyboard connector, but does not
> emulate output, so we detect this as "dumb" keyboard and enable softrepeat.
> But softrepeat causes double keypresses.
> 
> I suppose we could revamp the old softrepeat parameter to be more than
> a boolean, but I see no point.
> 
> If you agree, please include the attached.
> 
> -- Pete
> 
> --- linux-2.6.16-rc2/drivers/input/keyboard/atkbd.c	2006-02-11 00:31:41.000000000 -0800
> +++ linux-2.6.16-rc2-lem/drivers/input/keyboard/atkbd.c	2006-02-21 11:53:53.000000000 -0800
> @@ -50,6 +50,10 @@ static int atkbd_softrepeat;
>  module_param_named(softrepeat, atkbd_softrepeat, bool, 0);
>  MODULE_PARM_DESC(softrepeat, "Use software keyboard repeat");
>  
> +static int atkbd_nosoftrepeat;
> +module_param_named(nosoftrepeat, atkbd_nosoftrepeat, bool, 0);
> +MODULE_PARM_DESC(nosoftrepeat, "Do not use software keyboard repeat");
> +
>  static int atkbd_softraw = 1;
>  module_param_named(softraw, atkbd_softraw, bool, 0);
>  MODULE_PARM_DESC(softraw, "Use software generated rawmode");
> @@ -862,7 +866,7 @@ static int atkbd_connect(struct serio *s
>  	atkbd->softrepeat = atkbd_softrepeat;
>  	atkbd->scroll = atkbd_scroll;
>  
> -	if (!atkbd->write)
> +	if (!atkbd->write && !atkbd_nosoftrepeat)
>  		atkbd->softrepeat = 1;
>  
>  	if (atkbd->softrepeat)
> 
> 

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Suppressing softrepeat

[Dmitry Torokhov]

On 2/21/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> On Tue, Feb 21, 2006 at 12:43:08PM -0800, Pete Zaitcev wrote:
>
> > Add the "nosoftrepeat" parameter. This is useful if a "dumb" keyboard
> > has (unswitcheable) hardware repeat, like in Dell DRAC3.
>
> The softrepeat code should properly ignore all autorepeated keys from a
> 'dumb' keyboard. It's rather common that a keyboard we can't communicate
> with is in autorepeat mode, because that's the mode AT keyboards wake up
> in after power on.

Hmm, atkbd only detects "repeated" keystrokes if it is working in
hard-repeat mode:

        value = atkbd->release ? 0 :
                                (1 + (!atkbd->softrepeat &&
test_bit(atkbd->keycode[code], atkbd->dev->key)));

Should we always recognize "repeats"? Then we woudl not need any
workarounds, be it kbdrate or sysfs.

--
Dmitry

Re: Suppressing softrepeat

[Vojtech Pavlik]

On Tue, Feb 21, 2006 at 04:32:21PM -0500, Dmitry Torokhov wrote:
> On 2/21/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> > On Tue, Feb 21, 2006 at 12:43:08PM -0800, Pete Zaitcev wrote:
> >
> > > Add the "nosoftrepeat" parameter. This is useful if a "dumb" keyboard
> > > has (unswitcheable) hardware repeat, like in Dell DRAC3.
> >
> > The softrepeat code should properly ignore all autorepeated keys from a
> > 'dumb' keyboard. It's rather common that a keyboard we can't communicate
> > with is in autorepeat mode, because that's the mode AT keyboards wake up
> > in after power on.
> 
> Hmm, atkbd only detects "repeated" keystrokes if it is working in
> hard-repeat mode:
> 
>         value = atkbd->release ? 0 :
>                                 (1 + (!atkbd->softrepeat &&
> test_bit(atkbd->keycode[code], atkbd->dev->key)));

The above code is correct - in softrepeat mode, a value of '1' is
reported for each keystroke, even repeated ones, and that is then
filtered out by the input.c duplicate event filtering logic.

For hardrepeat mode, the value is changed to '2', and thus passes
through the sieve in input.c.

> Should we always recognize "repeats"? Then we woudl not need any
> workarounds, be it kbdrate or sysfs.

I'm not sure where this would help. The problem is that the DRAC3
'holds' the key pressed for full 500 ms before sending the release
scancde, even if the user pressed it just momentarily. It doesn't
generate repeat scancodes in this time. This however triggers the
softrepeat, which is by default set to 250 ms, causing repeated
characters all the time. 

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Suppressing softrepeat

[Dmitry Torokhov]

On 2/21/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
>
> I'm not sure where this would help. The problem is that the DRAC3
> 'holds' the key pressed for full 500 ms before sending the release
> scancde, even if the user pressed it just momentarily. It doesn't
> generate repeat scancodes in this time. This however triggers the
> softrepeat, which is by default set to 250 ms, causing repeated
> characters all the time.
>

Ah, I see. Well, then we really can't help it without fiddling with repeat rate.

--
Dmitry

Re: Suppressing softrepeat

[Pete Zaitcev]

On Tue, 21 Feb 2006 22:08:00 +0100, Vojtech Pavlik <vojtech@suse.cz> wrote:

> A much simpler workaround for the DRAC3 is to set the softrepeat delay
> to at least 750ms, using kbdrate(8), which will call the proper console
> ioctl, resulting in updating the softrepeat parameters.
> 
> I prefer workarounds for problematic hardware done outside the kernel,
> if possible.

I agree with the sentiment when posed in the abstract way, but let me
tell you why this case is different.

Firstly, there's nothing "problematic" about this. It's just how it is.
The only problematic thing here is our code. Currently, the situation is
assymetric. It is possible to force softrepeat on, but not possible to
force softrepeat off. Isn't it broken?

Secondly, 750ms may be not enough. Stuart is being shy here and posting
explanations to Bugzilla for some reason.

Lastly, it's such a PITA to add these things into the userland, that
it's completely impractical. Console is needed the most when things go
wrong. In such case, that echo(1) may not be reached before the single
user shell. And stuffing it into the initrd is for Linux weenies only,
unless automated by mkinitrd.

I think you're being unreasonable here. I am not asking for NFS root
or IP autoconfiguration and sort of complicated process which ought to
be done in userland indeed.

-- Pete

Re: Suppressing softrepeat

[Vojtech Pavlik]

On Wed, Feb 22, 2006 at 12:00:47PM -0800, Pete Zaitcev wrote:
> On Tue, 21 Feb 2006 22:08:00 +0100, Vojtech Pavlik <vojtech@suse.cz> wrote:
> 
> > A much simpler workaround for the DRAC3 is to set the softrepeat delay
> > to at least 750ms, using kbdrate(8), which will call the proper console
> > ioctl, resulting in updating the softrepeat parameters.
> > 
> > I prefer workarounds for problematic hardware done outside the kernel,
> > if possible.
> 
> I agree with the sentiment when posed in the abstract way, but let me
> tell you why this case is different.
> 
> Firstly, there's nothing "problematic" about this. It's just how it is.
> The only problematic thing here is our code. Currently, the situation is
> assymetric. It is possible to force softrepeat on, but not possible to
> force softrepeat off. Isn't it broken?
> 
> Secondly, 750ms may be not enough. Stuart is being shy here and posting
> explanations to Bugzilla for some reason.
> 
> Lastly, it's such a PITA to add these things into the userland, that
> it's completely impractical. Console is needed the most when things go
> wrong. In such case, that echo(1) may not be reached before the single
> user shell. And stuffing it into the initrd is for Linux weenies only,
> unless automated by mkinitrd.
> 
> I think you're being unreasonable here. I am not asking for NFS root
> or IP autoconfiguration and sort of complicated process which ought to
> be done in userland indeed.
 
I'm definitely not intending to be unreasonable, and I understand your
need to have the keyboard working all the way from the grub/lilo prompt.

I just don't like adding more module options to one that already has so
many it's hard to understand what they're used for.

How about simply this patch instead?

Setting autorepeat will not be possible on 'dumb' keyboards anymore by
default, but since these usually are special forms of hardware anyway,
like the DRAC3, this shouldn't be an issue for most users. Using
'softrepeat' on these keyboards will restore the behavior for users that
need it.

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -863,9 +863,6 @@ static int atkbd_connect(struct serio *s
 	atkbd->softrepeat = atkbd_softrepeat;
 	atkbd->scroll = atkbd_scroll;
 
-	if (!atkbd->write)
-		atkbd->softrepeat = 1;
-
 	if (atkbd->softrepeat)
 		atkbd->softraw = 1;

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Suppressing softrepeat

[Dmitry Torokhov]

On 2/22/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> On Wed, Feb 22, 2006 at 12:00:47PM -0800, Pete Zaitcev wrote:
> > On Tue, 21 Feb 2006 22:08:00 +0100, Vojtech Pavlik <vojtech@suse.cz> wrote:
> >
> > > A much simpler workaround for the DRAC3 is to set the softrepeat delay
> > > to at least 750ms, using kbdrate(8), which will call the proper console
> > > ioctl, resulting in updating the softrepeat parameters.
> > >
> > > I prefer workarounds for problematic hardware done outside the kernel,
> > > if possible.
> >
> > I agree with the sentiment when posed in the abstract way, but let me
> > tell you why this case is different.
> >
> > Firstly, there's nothing "problematic" about this. It's just how it is.
> > The only problematic thing here is our code. Currently, the situation is
> > assymetric. It is possible to force softrepeat on, but not possible to
> > force softrepeat off. Isn't it broken?
> >
> > Secondly, 750ms may be not enough. Stuart is being shy here and posting
> > explanations to Bugzilla for some reason.
> >
> > Lastly, it's such a PITA to add these things into the userland, that
> > it's completely impractical. Console is needed the most when things go
> > wrong. In such case, that echo(1) may not be reached before the single
> > user shell. And stuffing it into the initrd is for Linux weenies only,
> > unless automated by mkinitrd.
> >
> > I think you're being unreasonable here. I am not asking for NFS root
> > or IP autoconfiguration and sort of complicated process which ought to
> > be done in userland indeed.
>
> I'm definitely not intending to be unreasonable, and I understand your
> need to have the keyboard working all the way from the grub/lilo prompt.
>
> I just don't like adding more module options to one that already has so
> many it's hard to understand what they're used for.
>
> How about simply this patch instead?
>
> Setting autorepeat will not be possible on 'dumb' keyboards anymore by
> default, but since these usually are special forms of hardware anyway,
> like the DRAC3, this shouldn't be an issue for most users. Using
> 'softrepeat' on these keyboards will restore the behavior for users that
> need it.
>

I am not keen on changing the default behaviour... How many dumb
keybaords are out there? I'd rather turn atkbd.softrepeat into a
3-state switch...

--
Dmitry

Re: Suppressing softrepeat

[Vojtech Pavlik]

On Wed, Feb 22, 2006 at 04:09:33PM -0500, Dmitry Torokhov wrote:
> > How about simply this patch instead?
> >
> > Setting autorepeat will not be possible on 'dumb' keyboards anymore by
> > default, but since these usually are special forms of hardware anyway,
> > like the DRAC3, this shouldn't be an issue for most users. Using
> > 'softrepeat' on these keyboards will restore the behavior for users that
> > need it.
> 
> I am not keen on changing the default behaviour... How many dumb
> keyboards are out there?

Apart from the DRAC3, some home-made Sun-to-PS2 converter, and a single
non-x86 embedded box, I don't recall anything. Answer: very few.

There may be users, though, that use this option to force the detection
of the keyboard when not really plugged in, eg. for flaky KVMs. I've
Googled for that usage, but found none.

> I'd rather turn atkbd.softrepeat into a 3-state switch...

We could, but the more I think about it, the stronger I'm convinced that
the dumbkbd => softrepeat => softraw option implication chain is wrong.
The second implication is necessary, but with dumbkbd it's quite likely
you won't want softraw.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: Suppressing softrepeat

[Dmitry Torokhov]

On 2/22/06, Vojtech Pavlik <vojtech@suse.cz> wrote:
> On Wed, Feb 22, 2006 at 04:09:33PM -0500, Dmitry Torokhov wrote:
> > > How about simply this patch instead?
> > >
> > > Setting autorepeat will not be possible on 'dumb' keyboards anymore by
> > > default, but since these usually are special forms of hardware anyway,
> > > like the DRAC3, this shouldn't be an issue for most users. Using
> > > 'softrepeat' on these keyboards will restore the behavior for users that
> > > need it.
> >
> > I am not keen on changing the default behaviour... How many dumb
> > keyboards are out there?
>
> Apart from the DRAC3, some home-made Sun-to-PS2 converter, and a single
> non-x86 embedded box, I don't recall anything. Answer: very few.
>
> There may be users, though, that use this option to force the detection
> of the keyboard when not really plugged in, eg. for flaky KVMs. I've
> Googled for that usage, but found none.
>
> > I'd rather turn atkbd.softrepeat into a 3-state switch...
>
> We could, but the more I think about it, the stronger I'm convinced that
> the dumbkbd => softrepeat => softraw option implication chain is wrong.
> The second implication is necessary, but with dumbkbd it's quite likely
> you won't want softraw.
>

OK, then I'll schedule this change for 2.6.17 and we'll see if anyone screams.

--
Dmitry

Re: Suppressing softrepeat

[Pete Zaitcev]

On Wed, 22 Feb 2006 21:40:24 +0100, Vojtech Pavlik <vojtech@suse.cz> wrote:

> Setting autorepeat will not be possible on 'dumb' keyboards anymore by
> default, but since these usually are special forms of hardware anyway,
> like the DRAC3, this shouldn't be an issue for most users. Using
> 'softrepeat' on these keyboards will restore the behavior for users that
> need it.

> +++ b/drivers/input/keyboard/atkbd.c
> @@ -863,9 +863,6 @@ static int atkbd_connect(struct serio *s
>  	atkbd->softrepeat = atkbd_softrepeat;
>  	atkbd->scroll = atkbd_scroll;
>  
> -	if (!atkbd->write)
> -		atkbd->softrepeat = 1;
> -

This looks good to me, because it makes the operation of the code more
straightforward and easier to understand. With our docs always lagging,
it's important.

Like you said, it does "punish" people who had "dumb" keyboards, but if
they really are a special class of folks, maybe it's better that way.

And I think it's much easier to type a command to enable softrepeat
than to type a command to disable softrepeat when all your letters come
in twos and threes :-)

-- Pete

Re: Suppressing softrepeat

[Dmitry Torokhov]

On 2/21/06, Pete Zaitcev <zaitcev@redhat.com> wrote:
> Hi, Dmitry,
>

Hi Pete,

> Dell people passed on a request to add a new parameter, "nosoftrepeat",
> to i8042 (in atkbd.c). So, if I understand right, things should work so:
>
>  - None set in grub.conf: Softrepeat is set by the driver as detected
>   or derived. This is the default.
>  - "softrepeat" set: Softrepeat is on, regardless
>  - "nosoftrepeat" set: Softrepeat is off, regardless
>  - Both "softrepeat" and "nosoftrepeat" are set: Do not do that.
>
> The code looked confusing, but there is an good explanation in this bug:
>  https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=181457
>
> In short words, DRAC3 "plugs into" the keyboard connector, but does not
> emulate output, so we detect this as "dumb" keyboard and enable softrepeat.
> But softrepeat causes double keypresses.

I see the problem but I don't think we want another module parameter
for it. I think if you put the following somewhere into init scripts
it shoudl work without any additional changes:

        echo -n "0" > /sys/bus/serio/devices/serioX/softrepeat

Of couurse one would jhave to locate proper serioX but that should be easy.

I also see the following in bugzilla: "... This causs a problem on
systems that have no real keyboard plugged in, since atkbd probes for
the keyboard, and won't take control of the port if it doesn't see
one." Usually it is OK for keyboard to be missing as long as BIOS
itself does not disable keyboard port - whenever there is new data
starts coming from the port serio core will try to find proper driver
for it. I wonder why this is not working on boxes in question.

--
Dmitry

Re: Suppressing softrepeat

[Vojtech Pavlik]

On Tue, Feb 21, 2006 at 04:15:57PM -0500, Dmitry Torokhov wrote:
 
> I see the problem but I don't think we want another module parameter
> for it. I think if you put the following somewhere into init scripts
> it shoudl work without any additional changes:
> 
>         echo -n "0" > /sys/bus/serio/devices/serioX/softrepeat

This should work, indeed, as an alternative to 'kbdrate -d 750'.

> Of couurse one would jhave to locate proper serioX but that should be easy.
> 
> I also see the following in bugzilla: "... This causs a problem on
> systems that have no real keyboard plugged in, since atkbd probes for
> the keyboard, and won't take control of the port if it doesn't see
> one." Usually it is OK for keyboard to be missing as long as BIOS
> itself does not disable keyboard port - whenever there is new data
> starts coming from the port serio core will try to find proper driver
> for it. I wonder why this is not working on boxes in question.
 
The DRAC3 doesn't respond to ANY commands, thus the atkbd probe fails.

-- 
Vojtech Pavlik
Director SuSE Labs

RE: Suppressing softrepeat

[Stuart_Hayes]

Vojtech Pavlik wrote:
> On Wed, Feb 22, 2006 at 12:00:47PM -0800, Pete Zaitcev wrote:
>> On Tue, 21 Feb 2006 22:08:00 +0100, Vojtech Pavlik <vojtech@suse.cz>
>> wrote: 
>> 
>>> A much simpler workaround for the DRAC3 is to set the softrepeat
>>> delay to at least 750ms, using kbdrate(8), which will call the
>>> proper console ioctl, resulting in updating the softrepeat
>>> parameters. 
>>> 
>>> I prefer workarounds for problematic hardware done outside the
>>> kernel, if possible.
>> 
>> I agree with the sentiment when posed in the abstract way, but let me
>> tell you why this case is different.
>> 
>> Firstly, there's nothing "problematic" about this. It's just how it
>> is. The only problematic thing here is our code. Currently, the
>> situation 
>> is assymetric. It is possible to force softrepeat on, but not
>> possible 
>> to force softrepeat off. Isn't it broken?
>> 
>> Secondly, 750ms may be not enough. Stuart is being shy here and
>> posting explanations to Bugzilla for some reason.
>> 
>> Lastly, it's such a PITA to add these things into the userland, that
>> it's completely impractical. Console is needed the most when things
>> go wrong. In such case, that echo(1) may not be reached before the
>> single user shell. And stuffing it into the initrd is for Linux
>> weenies only, unless automated by mkinitrd. 
>> 
>> I think you're being unreasonable here. I am not asking for NFS root
>> or IP autoconfiguration and sort of complicated process which ought
>> to 
>> be done in userland indeed.
> 
> I'm definitely not intending to be unreasonable, and I understand
> your need to have the keyboard working all the way from the grub/lilo
> prompt.  
> 
> I just don't like adding more module options to one that already has
> so many it's hard to understand what they're used for. 
> 
> How about simply this patch instead?
> 
> Setting autorepeat will not be possible on 'dumb' keyboards anymore
> by default, but since these usually are special forms of hardware
> anyway, like the DRAC3, this shouldn't be an issue for most users.
> Using 'softrepeat' on these keyboards will restore the behavior for
> users that need it.    
> 
> diff --git a/drivers/input/keyboard/atkbd.c
> b/drivers/input/keyboard/atkbd.c --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -863,9 +863,6 @@ static int atkbd_connect(struct serio *s
>  	atkbd->softrepeat = atkbd_softrepeat;
>  	atkbd->scroll = atkbd_scroll;
> 
> -	if (!atkbd->write)
> -		atkbd->softrepeat = 1;
> -
>  	if (atkbd->softrepeat)
>  		atkbd->softraw = 1;

That seems reasonable to me, and would fix the issue with the DRAC3.

Shyly,
Stuart