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

[Tomasz Kramkowski]

On Fri, Mar 03, 2017 at 05:15:01PM +0100, Benjamin Tissoires wrote:
> > 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?

This device is an adapter for ATARI and SEGA GENESIS controllers.

The genesis controller I am testing with has a basic directional pad, if
I am interpreting the HID spec correctly, such a control should report
its state in two fields, one for X one for Y, and the range of values
should be between -1 and 1. The logical minimum and maximum specified by
the device's report descriptor does indeed give -1 to 1 as the range,
however the reports themselves give values of -2 and 1 (when they should
be -1 and 1). For more information, I recommend perusing the previous
iteration of this patch which directly addressed this issue:

https://lkml.org/lkml/2016/12/16/595

(And as a side note, this bug has been reported by other users of this
adapter and the GENESIS controller I am testing with is genuine so I do
not think that the controller itself is at fault here.)

> > "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.")

I'm not entirely sure about this, because at this point in the spec the
"No null position / Null state" bit has not even been mentioned.

The sentence "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." (aside from having a small
grammatical mistake which might be the source of ambiguity) seems to be
referring to a "bit field" in the way that the rest of the spec refers
to "bit fields" as fields comprised of n bits. This is further supported
by a number of mentions of 8-bit fields and 1-bit fields throughout the
specification, and also by the existence of the 8th data bit of Input,
Output and Feature items: {Bit Field (0) | Buffered Bytes (1)} This bit
distinguishes between a field of n bits and a 8-bit aligned "fixed-size
stream of bytes".

So what I think this sentence is saying is: If you have a field of 4
bits but a logical minimum of -7 and a logical maximum of 7. In this
example, the range that a 4-bit field could store would be 0-15 or -8 to
7, since the logical minimum excludes the -8, this is a field capable of
producing a null value.

(This is also mentioned in the thread you mentioned before:
https://lkml.org/lkml/2011/11/2/470 Denilson comes to the same
conclusion)

> > 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

The thing I think we can both agree on is that the "No Null Position |
Null State" bit seems rather unnecessary if it is interpreted this way.
If all values should be forwarded, it would be more obvious if the
logical range was simply extended rather than have this ambiguous bit
set. This is why I personally think that if the spec is interpreted to
understand this bit as "descriptive" of how the device should behave, it
makes more sense to see it as a way of detecting if the device is
malfunctioning rather than as a way of interpreting the report.

I think to this end it might make more sense to patch the code to
produce a warning message on the kernel log if a device with the "No
Null State" bit set does in fact produce an out of range value, but I'm
personally not sure if the kernel attempts to make information about
buggy USB devices known.

> > 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.

I think the fact this descriptor is buggy might be a good indication
that it is not representative of the usage of the "No Null Position |
Null State" bit.

> > 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.

Indeed, the question is what does "gracefully" mean, the mentioned
previous thread which introduced the initial check suggests that there
are rare cases of devices which make use of out of range values and it
seems dropping these values seemed like the correct approach. And this
conclusion was made from the interpretation of the spec with the creator
of a device on hand to explain how they personally interpreted the spec.

In the current case, we have an even rarer device and no creator to
speak to. I think in these cases it is difficult to make a decision on
how to handle things based on how one device (which by my interpretation
is buggy even if it does work on Windows) expects things to work.

The other issue is that the ambiguity of the spec itself makes it
difficult to decide how most other creators of devices might interpret
it themselves in the future.

> 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.

I am personally not as confident that the original thread clears
everything up, but I am entirely happy to take this slowly and carefully
and discuss all the issues and ambiguities before this patch is
accepted.

I think you're right that this patch is not likely to bring about any
issues, but I think it's more likely that this bit is simply generally
avoided due to the ambiguity of its specification rather than the
explicit correctness of our interpretations.

I think to some extent it might even be worth bringing Denilson
Figueiredo de Sá for his opinion on my and your interpretations of the
spec.

Thank you for your review, I greatly appreciate your time spent going
over my interpretation.

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/