Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[Benjamin Tissoires]

Hi,

After some checks on the devices I have and the history of the code, I
think the patch in this form is correct:

Acked-By: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>

Actually, in the discussion that introduced those checks, there was even
a mention of checking the NULL state byte. Unfortunately, it didn't end
up in the final patch:
https://lkml.org/lkml/2011/10/22/142

Note that the device in question sets the Null bit, so it shouldn't
regress with the patch.

On Feb 14 2017 or thereabouts, Tomasz Kramkowski wrote:
> I apologise sincerely for sitting on these patches for so long, aside from
> other tasks I was busy with, I have found it very difficult to try to correctly
> interpret the HID specification on this topic and then to try to word my
> explanation below.

No need to apologize. We are all busy and perfectly understand that
there can be some time between the idea and the proper submission.

> 
> As agreed on the list, I have looked into the patch provided on the
> bugzilla bug #68621 and have verified that it does in fact fix the issue
> with the adapter.
> 
> This v2 patch set contains the original patch provided by Valtteri and
> an additional patch to make the device fully operational.
> 
> As mentioned in a prior email, not dropping the out of range value
> produces expected results from the resulting joydev file (when used with
> jstest). The evdev file (when tested with evtest) correctly reports a
> minimum of -1 and a maximum of 1 but also returns events with values of
> -2 and 1. I'm not too sure how this will affect certain applications but
> from my tests the device works fine despite this.
> 
> However, I'm not entirely sure that replacing the other fix with this
> fix is completely correct, I'll try to explain my reasoning:
> 
> Firstly, the value reported by the device is still unusual and does not
> correctly represent the state of the device.

That's a little bit worrying. I still think the patch could be taken,
but it would be interesting to fix the device. What behavior are you
seeing?

> 
> The next point is a bit more confusing so I'll try to go over my
> reasoning:
> 
> This new patch is based on the idea that "null values should not be
> ignored when the 'No Null Position/Null State' flag is 0."
> 
> This contradicts Â5.10 of the HID 1.11 specification which states:
> 
> "If an 8-bit field is declared and the range of valid values is 0 to
> 0x7F then any value between 0x80 and 0xFF will be considered out of
> range and ignored when received. The initialization of null values in a
> report is much easier if they are all the same."

I think the whole paragraph is conditioned to the fact that the Null bit
is set ("This is accomplished by declaring bit field in a report that is
capable of containing a range of values larger than those actually
generated by the control.")

> 
> This sentence even in context appears to make no mention of the "No Null
> Position/Null State" bit which seems to suggest that all values which
> are out of logical range should be ignored.
> 
> I think this sentence does not contradict Â6.2.2.5 (page 31) which
> states this about the "No Null Position/Null State" bit:
> 
> "Indicates whether the control has a state in which it is not sending
> meaningful data."
> 
> Which suggests that leaving the bit unset means "the control is always
> sending meaningful data" as this can simply be interpreted to mean that
> the control should never be reporting out-of-range values, not that out
> of range values should stop being ignored.

I interpret this as:
- No Null Position (0) -> all the data sent by the device is valid and
  should be treated as such. Out of range values should then be forwarded
- Null State (1) -> the device can send out-of-range values that are
  meaningless and should be ignored

> 
> However, part of the Universal Serial Bus HID Usage Tables document
> appears to contradict this interpretation to some extent.
> ÂA.4 (page 132) states:
> 
> "The No Null Position flag indicates that there is never a state in
> which it is not sending meaningful data. The returned values are Null =
> No event (outside of the Logical Minimum / Logical Maximum range) 1 =
> Stat Not Ready, 2 = Stat Ready, or 3 = Err Not a loadable character."

IMO, the report descriptor above is buggy:
  ReportSize(2), ReportCount(1), 
  Logical Maximum(2), 
  Usage(Display Status), 
  Collection(Logical), 
      Usage(Stat Not Ready), 
      Usage(Stat    Ready),    
      Usage(Err Not a loadable character), 
      Input(Data, Array, Absolute, No Null Position),  ; 3-bit status field 
  End Collection(), 

The explicit logical minimum is not set, implying that "Stat Not Ready"
should be numerical value 0x00, not 0x01.

> 
> Which suggests that the control would report a null value despite the

No. The fact that "No Null Position" is there (Null bit not set) shows
that an event of 0x03 is valid and "Err Not a loadable character" should
be reported. I am just unsure about the logical minimum and the implicit
"Null == event 0x00". The definition of the Array bit (Â6.2.2.5 p30 of
the HID spec) states that "The number of elements in the array can be
deduced by examining the difference between Logical Minimum and Logical
Maximum (number of elements = Logical Maximum - Logical Minimum + 1).

So in this case, logical min being 0, this gives 3 elements. And maybe
the "official" original driver (Windows) only started indexes at 1. (Or
this should be the continuation of the previous report descriptor that
specifies Logical Min to 1).

> "No Null Position" flag. However, this seems to conflict with an earlier
> mention of the "Stat Not Ready" usage in Â3.4.2.1 which states:
> 
> "The array field never returns an index of NULL because one usage is
> always valid. An example is Stat Not Ready on the Alphanumeric Display
> page."

Yep, but that is not much of an issue I guess :)
'Sel' should not report NULL values, but in the example it does, meaning
that this must be in a bigger report that contains other bits here and
there.

> 
> The two other annotated examples mentioning this flag (ÂA.3.1, ÂA.3.2)
> do not have the same contradictory wording suggesting that this was
> possibly a mistake.
> 
> Personally I am not entirely sure I am interpreting everything
> correctly, but it seems that the "No Null Position/Null State" bit is
> not prescriptive on how the data should be handled but rather
> descriptive of what data should be expected.

Agree. Though it seems that if the device explains if it will provide
Null values or not, we should handle this gracefully.

I was a bit hesitant to give my Acked-by at the beginning, but
re-reading the original kernel thread made up my mind. Also, the fact
that most devices that I saw are not using NULL values and that this bit
needs to be explicitly set makes me feel comfortable testing this
upstream for v4.12.

Cheers,
Benjamin

> 
> -- 
> Tomasz Kramkowski
> 
> Tomasz Kramkowski (1):
>   HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter
> 
> Valtteri Heikkilà (1):
>   HID: reject input outside logical range only if null state is set
> 
>  drivers/hid/hid-ids.h           | 3 +++
>  drivers/hid/hid-input.c         | 1 +
>  drivers/hid/usbhid/hid-quirks.c | 1 +
>  3 files changed, 5 insertions(+)
> 
> -- 
> 2.11.0
>