Messages in this thread | | | From | Dmitry Semyonov <> | Date | Thu, 11 Feb 2016 01:22:18 +0300 | Subject | Incorrect processing of "1" raw_event() return value inside hid_input_report() |
| |
Hello Jiri,
Please, refer to the commit b1a1442a23776756b254b69786848a94d92445ba and to the second part of http://thread.gmane.org/gmane.linux.kernel.input/39802. (The message author was right but hesitated to insist. Therefore, I have to re-open the issue):
>> if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) { >> ret = hdrv->raw_event(hid, report, data, size); >> - if (ret != 0) { >> + if (ret < 0) { >> ret = ret < 0 ? ret : 0; >> goto unlock; >> } >> -------------------------------------------------- >> >> So now it does not matter whether the raw_event method returns 0 or 1. >> The documentation says:
(The hid_driver structure documentation in linux/hid.h)
>> raw_event and event should return 0 on no action performed, 1 when >> no further processing should be done and negative on error [...] > This change was done so that we don't call hid_report_raw_event() when > ->raw_event() callback encountered error during parsing (and therefore > returned negative value).
I cannot accept this answer because the processing of negative return values was not affected by the above change.
What is worse, I cannot accept the change itself because it breaks the contract established by the hid_driver structure documentation for HID drivers.
Ubuntu guys had to revert it[1] due to changed behavior that uncovered another bug[2]. A colleague of mine re-discovered the same root cause while investigating a different issue, and had to revert the change as well because of some existing proprietary keypad driver was using the "1" value to indicate that no further processing is needed, (exactly as specified in the documentation).
[1]https://lists.ubuntu.com/archives/kernel-team/2013-October/032351.html [2]http://www.kernelhub.org/?p=2&msg=380901
A lot of HID drivers in the kernel source tree use negative, 0, and 1 return values in their *_raw_event() functions. Some of them may be using the values incorrectly but at least they were verified by the original developers, and I doubt you had a chance to properly re-test *all* the drivers before introducing the breaking API change. Therefore, I strongly recommend to revert the following two related patches, and fix the specific driver(s) instead if there are any issues left:
b1a1442a HID: core: fix reporting of raw events 556483e2 HID: remove self-assignment from hid_input_report
-- ...Bye..Dmitry.
| |