bad handling of HID input reports for a Delcom foot pedal

bad handling of HID input reports for a Delcom foot pedal

[Benjamin Tissoires]

Hi Jiri,

Please have a look at: https://bugzilla.kernel.org/show_bug.cgi?id=76461

To me, this device sends garbage and should not send the modifiers
both in a Var input and in an Array input.

I can fix it either by amending the report descriptor of the device
(one byte to change) or by changing hid-input.c for it to send the
events in the array processing no matter what was the previous state
(see below).

I don't know which one to pick because this seems to be a corner case,
so fixing hid-input might not be the place, but I am afraid of
removing part of the reports coming from the device (what if the
modifiers are actually used for something in the report).

Any thoughts on it?

For the record, fixing hid-input would just remove the test in hid_input_field:

                if (value[n] >= min && value[n] <= max
-                       && field->usage[value[n] - min].hid
-                       && search(field->value, value[n], count))
+                       && field->usage[value[n] - min].hid)
                                hid_process_event(hid, field,
&field->usage[value[n] - min], 1, interrupt);
        }

(Sorry, gmail, whitespace damaged and everything)

Cheers,
Benjamin

Re: bad handling of HID input reports for a Delcom foot pedal

[Jiri Kosina]

On Mon, 19 May 2014, Benjamin Tissoires wrote:

> Hi Jiri,
> 
> Please have a look at: https://bugzilla.kernel.org/show_bug.cgi?id=76461
> 
> To me, this device sends garbage and should not send the modifiers
> both in a Var input and in an Array input.
> 
> I can fix it either by amending the report descriptor of the device
> (one byte to change) or by changing hid-input.c for it to send the
> events in the array processing no matter what was the previous state
> (see below).
> 
> I don't know which one to pick because this seems to be a corner case,
> so fixing hid-input might not be the place, but I am afraid of
> removing part of the reports coming from the device (what if the
> modifiers are actually used for something in the report).
> 
> Any thoughts on it?
> 
> For the record, fixing hid-input would just remove the test in hid_input_field:
> 
>                 if (value[n] >= min && value[n] <= max
> -                       && field->usage[value[n] - min].hid
> -                       && search(field->value, value[n], count))
> +                       && field->usage[value[n] - min].hid)
>                                 hid_process_event(hid, field,
> &field->usage[value[n] - min], 1, interrupt);
>         }
> 
> (Sorry, gmail, whitespace damaged and everything)

So if modifying the report descriptor is not the option (which is what I 
understood from Bryan's mail), I have to say I don't really like this 
change in hid_input_field(), as it's really rather "non obvious".

How about just creating a device specific input event quirk for this?

-- 
Jiri Kosina
SUSE Labs

Re: bad handling of HID input reports for a Delcom foot pedal

[Bryan Mills]

On Wed, Jun 18, 2014 at 8:13 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 19 May 2014, Benjamin Tissoires wrote:
> > Hi Jiri,
> >
> > Please have a look at: https://bugzilla.kernel.org/show_bug.cgi?id=76461
> >
> > To me, this device sends garbage and should not send the modifiers
> > both in a Var input and in an Array input.
> >
> > I can fix it either by amending the report descriptor of the device
> > (one byte to change) or by changing hid-input.c for it to send the
> > events in the array processing no matter what was the previous state
> > (see below).
> >
> > I don't know which one to pick because this seems to be a corner case,
> > so fixing hid-input might not be the place, but I am afraid of
> > removing part of the reports coming from the device (what if the
> > modifiers are actually used for something in the report).
> >
> > Any thoughts on it?
> >
> > For the record, fixing hid-input would just remove the test in hid_input_field:
> >
> >                 if (value[n] >= min && value[n] <= max
> > -                       && field->usage[value[n] - min].hid
> > -                       && search(field->value, value[n], count))
> > +                       && field->usage[value[n] - min].hid)
> >                                 hid_process_event(hid, field,
> > &field->usage[value[n] - min], 1, interrupt);
> >         }
> >
> > (Sorry, gmail, whitespace damaged and everything)
>
> So if modifying the report descriptor is not the option (which is what I
> understood from Bryan's mail), I have to say I don't really like this
> change in hid_input_field(), as it's really rather "non obvious".
>
> How about just creating a device specific input event quirk for this?
>
> --
> Jiri Kosina
> SUSE Labs

A device-specific quirk would be fine.

But what's wrong with simply removing the "search" test?  It's a no-op
for non-quirky devices (which don't send redundant keycodes) and a bug
for quirky ones (which should take the union of redundant keycodes
instead of dropping them).

Removing the test produces simpler code and handles the quirky devices
correctly without the need to check for them explicitly.