* [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter @ 2017-02-14 23:14 Tomasz Kramkowski 2017-02-14 23:14 ` [PATCH v2 1/2] HID: reject input outside logical range only if null state is set Tomasz Kramkowski ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Tomasz Kramkowski @ 2017-02-14 23:14 UTC (permalink / raw) To: Jiri Kosina Cc: Benjamin Tissoires, linux-input, linux-kernel, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] HID: reject input outside logical range only if null state is set 2017-02-14 23:14 [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski @ 2017-02-14 23:14 ` Tomasz Kramkowski 2017-03-06 13:04 ` Jiri Kosina 2017-02-14 23:14 ` [PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski 2017-03-03 16:15 ` [PATCH v2 0/2] patches for Innomedia " Benjamin Tissoires 2 siblings, 1 reply; 10+ messages in thread From: Tomasz Kramkowski @ 2017-02-14 23:14 UTC (permalink / raw) To: Jiri Kosina Cc: Benjamin Tissoires, linux-input, linux-kernel, Valtteri Heikkilä, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] HID: reject input outside logical range only if null state is set 2017-02-14 23:14 ` [PATCH v2 1/2] HID: reject input outside logical range only if null state is set Tomasz Kramkowski @ 2017-03-06 13:04 ` Jiri Kosina 0 siblings, 0 replies; 10+ messages in thread From: Jiri Kosina @ 2017-03-06 13:04 UTC (permalink / raw) To: Tomasz Kramkowski Cc: Benjamin Tissoires, linux-input, linux-kernel, Valtteri Heikkilä 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter 2017-02-14 23:14 [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski 2017-02-14 23:14 ` [PATCH v2 1/2] HID: reject input outside logical range only if null state is set Tomasz Kramkowski @ 2017-02-14 23:14 ` Tomasz Kramkowski 2017-03-06 13:04 ` Jiri Kosina 2017-03-03 16:15 ` [PATCH v2 0/2] patches for Innomedia " Benjamin Tissoires 2 siblings, 1 reply; 10+ messages in thread From: Tomasz Kramkowski @ 2017-02-14 23:14 UTC (permalink / raw) To: Jiri Kosina Cc: Benjamin Tissoires, linux-input, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter 2017-02-14 23:14 ` [PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski @ 2017-03-06 13:04 ` Jiri Kosina 0 siblings, 0 replies; 10+ messages in thread From: Jiri Kosina @ 2017-03-06 13:04 UTC (permalink / raw) To: Tomasz Kramkowski; +Cc: Benjamin Tissoires, linux-input, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter 2017-02-14 23:14 [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski 2017-02-14 23:14 ` [PATCH v2 1/2] HID: reject input outside logical range only if null state is set Tomasz Kramkowski 2017-02-14 23:14 ` [PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski @ 2017-03-03 16:15 ` Benjamin Tissoires 2017-03-04 10:19 ` Tomasz Kramkowski 2 siblings, 1 reply; 10+ messages in thread From: Benjamin Tissoires @ 2017-03-03 16:15 UTC (permalink / raw) To: Tomasz Kramkowski; +Cc: Jiri Kosina, linux-input, linux-kernel 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter 2017-03-03 16:15 ` [PATCH v2 0/2] patches for Innomedia " Benjamin Tissoires @ 2017-03-04 10:19 ` Tomasz Kramkowski 2017-03-06 9:04 ` Benjamin Tissoires 0 siblings, 1 reply; 10+ messages in thread From: Tomasz Kramkowski @ 2017-03-04 10:19 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, linux-kernel 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter 2017-03-04 10:19 ` Tomasz Kramkowski @ 2017-03-06 9:04 ` Benjamin Tissoires 2017-03-07 13:10 ` Tomasz Kramkowski 0 siblings, 1 reply; 10+ messages in thread From: Benjamin Tissoires @ 2017-03-06 9:04 UTC (permalink / raw) To: Tomasz Kramkowski Cc: Jiri Kosina, linux-input, linux-kernel, Denilson Figueiredo de Sá 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter 2017-03-06 9:04 ` Benjamin Tissoires @ 2017-03-07 13:10 ` Tomasz Kramkowski 2017-03-08 14:05 ` Jiri Kosina 0 siblings, 1 reply; 10+ messages in thread From: Tomasz Kramkowski @ 2017-03-07 13:10 UTC (permalink / raw) To: Benjamin Tissoires Cc: Jiri Kosina, linux-input, linux-kernel, Denilson Figueiredo de Sá 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter 2017-03-07 13:10 ` Tomasz Kramkowski @ 2017-03-08 14:05 ` Jiri Kosina 0 siblings, 0 replies; 10+ messages in thread From: Jiri Kosina @ 2017-03-08 14:05 UTC (permalink / raw) To: Tomasz Kramkowski Cc: Benjamin Tissoires, linux-input, linux-kernel, Denilson Figueiredo de Sá 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-08 14:53 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-14 23:14 [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski 2017-02-14 23:14 ` [PATCH v2 1/2] HID: reject input outside logical range only if null state is set Tomasz Kramkowski 2017-03-06 13:04 ` Jiri Kosina 2017-02-14 23:14 ` [PATCH v2 2/2] HID: usbhid: add quirk for innomedia INNEX GENESIS/ATARI adapter Tomasz Kramkowski 2017-03-06 13:04 ` Jiri Kosina 2017-03-03 16:15 ` [PATCH v2 0/2] patches for Innomedia " Benjamin Tissoires 2017-03-04 10:19 ` Tomasz Kramkowski 2017-03-06 9:04 ` Benjamin Tissoires 2017-03-07 13:10 ` Tomasz Kramkowski 2017-03-08 14:05 ` Jiri Kosina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).