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

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

[Tomasz Kramkowski]

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.

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.

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

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.

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

Which suggests that the control would report a null value despite the
"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."

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.

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

[PATCH v2 1/2] HID: reject input outside logical range only if null state is set

[Tomasz Kramkowski]

From: Valtteri Heikkilä <rnd@nic.fi>

This patch fixes an issue in drivers/hid/hid-input.c where USB HID
control null state flag is not checked upon rejecting inputs outside
logical minimum-maximum range. The check should be made according to USB
HID specification 1.11, section 6.2.2.5, p.31. The fix will resolve
issues with some game controllers, such as:
https://bugzilla.kernel.org/show_bug.cgi?id=68621

[tk@the-tk.com: shortened and fixed spelling in commit message]
Signed-off-by: Valtteri Heikkilä <rnd@nic.fi>
Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
 drivers/hid/hid-input.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index d05f903c7614..cf8256aac2bd 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1157,6 +1157,7 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 	 * don't specify logical min and max.
 	 */
 	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
+	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
 	    (field->logical_minimum < field->logical_maximum) &&
 	    (value < field->logical_minimum ||
 	     value > field->logical_maximum)) {
-- 
2.11.0

[PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter

[Tomasz Kramkowski]

The (1292:4745) Innomedia INNEX GENESIS/ATARI adapter needs
HID_QUIRK_MULTI_INPUT to split the device up into two controllers
instead of inputs from both being merged into one.

Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>
---
 drivers/hid/hid-ids.h           | 3 +++
 drivers/hid/usbhid/hid-quirks.c | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index f46f2c5117fa..e6b38fa49709 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -541,6 +541,9 @@
 #define USB_VENDOR_ID_IRTOUCHSYSTEMS	0x6615
 #define USB_DEVICE_ID_IRTOUCH_INFRARED_USB	0x0070
 
+#define USB_VENDOR_ID_INNOMEDIA			0x1292
+#define USB_DEVICE_ID_INNEX_GENESIS_ATARI	0x4745
+
 #define USB_VENDOR_ID_ITE               0x048d
 #define USB_DEVICE_ID_ITE_LENOVO_YOGA   0x8386
 #define USB_DEVICE_ID_ITE_LENOVO_YOGA2  0x8350
diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index e9d6cc7cdfc5..b3fe5308b353 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -167,6 +167,7 @@ static const struct hid_blacklist {
 	{ USB_VENDOR_ID_MULTIPLE_1781, USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_DRACAL_RAPHNET, USB_DEVICE_ID_RAPHNET_2NES2SNES, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_DRACAL_RAPHNET, USB_DEVICE_ID_RAPHNET_4NES4SNES, HID_QUIRK_MULTI_INPUT },
+	{ USB_VENDOR_ID_INNOMEDIA, USB_DEVICE_ID_INNEX_GENESIS_ATARI, HID_QUIRK_MULTI_INPUT },
 
 	{ 0, 0 }
 };
-- 
2.11.0

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@redhat.com>

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
> 

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/

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

[Benjamin Tissoires]

On Mar 04 2017 or thereabouts, Tomasz Kramkowski wrote:
> 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.

Well, the HID spec is not very clear, that the least we can agree on :)

But Microsoft's interpretation is rather clear in the
multitouch/touchpad/pen specification:
https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).aspx
paragraph "Required HID usages for pen digitizers":

"It should be noted that the host will recognize the values outside the
logical range as signifying the implementation of this protocol only if
the report descriptor specifically includes the bit signifying the fact
that X and Y support NULL states. Otherwise, values outside the logical
range are simply moved to the nearest boundary value."

And the 2 report descriptors written after are correct concerning the
NULL state bit.

For Microsoft (in the pointer delivery protocol):
- NULL state -> ignore out of range values
- No NULL State -> clamp at the nearest boundary.

Following this would solve both issues If I understand correctly. Your
controller would be clamped to [-1..1] (No Null State), and the ones
that need to be ignored (like the ones from Denilson will be thanks to
the NULL state bit set.

Maybe we can follow this to say we are mimicking Microsoft's driver and
hope for the best?

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

Cheers,
Benjamin

Re: [PATCH v2 1/2] HID: reject input outside logical range only if null state is set

[Jiri Kosina]

On Tue, 14 Feb 2017, Tomasz Kramkowski wrote:

> From: Valtteri Heikkilä <rnd@nic.fi>
> 
> This patch fixes an issue in drivers/hid/hid-input.c where USB HID
> control null state flag is not checked upon rejecting inputs outside
> logical minimum-maximum range. The check should be made according to USB
> HID specification 1.11, section 6.2.2.5, p.31. The fix will resolve
> issues with some game controllers, such as:
> https://bugzilla.kernel.org/show_bug.cgi?id=68621
> 
> [tk@the-tk.com: shortened and fixed spelling in commit message]
> Signed-off-by: Valtteri Heikkilä <rnd@nic.fi>
> Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>

Applied to for-4.12/hid-core-null-state-handling. Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter

[Jiri Kosina]

On Tue, 14 Feb 2017, Tomasz Kramkowski wrote:

> The (1292:4745) Innomedia INNEX GENESIS/ATARI adapter needs
> HID_QUIRK_MULTI_INPUT to split the device up into two controllers
> instead of inputs from both being merged into one.
> 
> Signed-off-by: Tomasz Kramkowski <tk@the-tk.com>

Applied to for-4.12/innomedia. Thanks Tomasz,

-- 
Jiri Kosina
SUSE Labs

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

[Tomasz Kramkowski]

On Mon, Mar 06, 2017 at 10:04:50AM +0100, Benjamin Tissoires wrote:
> Well, the HID spec is not very clear, that the least we can agree on :)
> 
> But Microsoft's interpretation is rather clear in the
> multitouch/touchpad/pen specification:
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).aspx
> paragraph "Required HID usages for pen digitizers":
> 
> "It should be noted that the host will recognize the values outside the
> logical range as signifying the implementation of this protocol only if
> the report descriptor specifically includes the bit signifying the fact
> that X and Y support NULL states. Otherwise, values outside the logical
> range are simply moved to the nearest boundary value."
> 
> And the 2 report descriptors written after are correct concerning the
> NULL state bit.
> 
> For Microsoft (in the pointer delivery protocol):
> - NULL state -> ignore out of range values
> - No NULL State -> clamp at the nearest boundary.
> 
> Following this would solve both issues If I understand correctly. Your
> controller would be clamped to [-1..1] (No Null State), and the ones
> that need to be ignored (like the ones from Denilson will be thanks to
> the NULL state bit set.

The clamping behaviour is the best of both worlds, it still matches how
I interpret the spec and provides a concrete definition of what should
happen when an out of bounds value is reported with the "No Null
Position | Null State" bit unset.

However, currently we just let the value pass through unchanged. So I
propose another patch on top of this one (at the bottom of the email,
done against for-4.12/hid-core-null-state-handling). The original change
which ignores the out of range value does a dbg_hid, I'm not sure if
that's necessary for the clamping scenario. I'll leave a few days for
any comments and if testing goes well (I don't see why not) I'll post it
on here as a patch. I'm not sure if that would be a v3 or a new patch.

> Maybe we can follow this to say we are mimicking Microsoft's driver and
> hope for the best?

Their approach to the ambiguity takes the safest bet and compatibility
with them might not be a bad idea anyway so I agree with following their
interpretation. I will note a link to that website in my commit message.

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

---
 drivers/hid/hid-input.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index cf8256aac2bd..cf38ff79cfe9 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1157,12 +1157,15 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 	 * don't specify logical min and max.
 	 */
 	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
-	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
 	    (field->logical_minimum < field->logical_maximum) &&
 	    (value < field->logical_minimum ||
 	     value > field->logical_maximum)) {
-		dbg_hid("Ignoring out-of-range value %x\n", value);
-		return;
+		if (field->flags & HID_MAIN_ITEM_NULL_STATE) {
+			dbg_hid("Ignoring out-of-range value %x\n", value);
+			return;
+		}
+		value = value < field->logical_minimum ?
+			field->logical_minimum : field->logical_maximum;
 	}
 
 	/*
-- 
2.12.0

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

[Jiri Kosina]

On Tue, 7 Mar 2017, Tomasz Kramkowski wrote:

> I'll post it on here as a patch. I'm not sure if that would be a v3 or a 
> new patch.

As the baseline patch is already merged and I am not rebasing hid.git, 
please send anything else as a followup fix.

Thanks,

-- 
Jiri Kosina
SUSE Labs