| Message ID | 20181104103247.32691-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | aa9b760cec2385ad408bb2e346c7f6dc1be69a79 |
| Headers | show |
| Series | HID: fix up .raw_event() documentation | expand |
On Sun, 4 Nov 2018, Linus Walleij wrote: > The documentation for the .raw_event() callback says that if the > driver return 1, there will be no further processing of the event, > but this is not true Thanks, applied. -- Jiri Kosina SUSE Labs
On Sun, Nov 4, 2018 at 11:34 AM Linus Walleij <linus.walleij@linaro.org> wrote: > The documentation for the .raw_event() callback says that if the > driver return 1, there will be no further processing of the event, > but this is not true, the actual code in hid-core.c looks like this: > > if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) { > ret = hdrv->raw_event(hid, report, data, size); > if (ret < 0) > goto unlock; > } > > ret = hid_report_raw_event(hid, type, data, size, interrupt); > > The only return value that has any effect on the processing is > a negative error. I noticed that there is a whole slew of drivers in the kernel that actually return 1 from their .raw_event handlers. drivers/hid/hid-alps.c drivers/hid/hid-asus.c drivers/hid/hid-cp2112.c drivers/hid/hid-elan.c drivers/hid/hid-elo.c (...) I suspect what they want is "no further event processing" so it's a pretty weird legacy bug or something. Should we patch them all one by one to return something like -ENODATA or should we patch the library to actually respect the return value 1 and skip further event processing if that happens? Yours, Linus Walleij
On Mon, Nov 12, 2018 at 12:19 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sun, Nov 4, 2018 at 11:34 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > The documentation for the .raw_event() callback says that if the > > driver return 1, there will be no further processing of the event, > > but this is not true, the actual code in hid-core.c looks like this: > > > > if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) { > > ret = hdrv->raw_event(hid, report, data, size); > > if (ret < 0) > > goto unlock; > > } > > > > ret = hid_report_raw_event(hid, type, data, size, interrupt); > > > > The only return value that has any effect on the processing is > > a negative error. > > I noticed that there is a whole slew of drivers in the kernel > that actually return 1 from their .raw_event handlers. > > drivers/hid/hid-alps.c > drivers/hid/hid-asus.c > drivers/hid/hid-cp2112.c > drivers/hid/hid-elan.c > drivers/hid/hid-elo.c > (...) > > I suspect what they want is "no further event processing" > so it's a pretty weird legacy bug or something. > > Should we patch them all one by one to return something like > -ENODATA or should we patch the library to actually > respect the return value 1 and skip further event processing > if that happens? > I finally found the time to find the issue of this (I wanted to do it before your patch get applied, but -ETIME): So, before b1a1442a23776756b ("HID: core: fix reporting of raw events" - 2013-06-03), if the returned value of .raw_event() was positive, the processing of the event was interrupted and .event() was not called after. However, there was issues with that as mentioned in b1a1442a. So since then, the HID processing goes on even if .raw_event() says "please stop processing". Given that it's been 5 years, and no one complained about it, I think we should keep the 'new' behavior of hid-core.c calling .raw_event(). As for fixing the drivers in .raw_event, I would not blindly change them to -ENODATA as the current code path is equivalent to returning 0. For some of them I can definitively have a good opinion on what to return (0 or -ENODATA), but for some others, I don't have the hardware so I can't really take a position. Cheers, Benjamin
diff --git a/include/linux/hid.h b/include/linux/hid.h index 2827b87590d8..387c70df6f29 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -722,8 +722,8 @@ struct hid_usage_id { * input will not be passed to raw_event unless hid_device_io_start is * called. * - * raw_event and event should return 0 on no action performed, 1 when no - * further processing should be done and negative on error + * raw_event and event should return negative on error, any other value will + * pass the event on to .event() typically return 0 for success. * * input_mapping shall return a negative value to completely ignore this usage * (e.g. doubled or invalid usage), zero to continue with parsing of this
The documentation for the .raw_event() callback says that if the driver return 1, there will be no further processing of the event, but this is not true, the actual code in hid-core.c looks like this: if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) { ret = hdrv->raw_event(hid, report, data, size); if (ret < 0) goto unlock; } ret = hid_report_raw_event(hid, type, data, size, interrupt); The only return value that has any effect on the processing is a negative error. Correct this as it seems to confuse people: I found bogus code in the Razer out-of-tree driver attempting to return 1 here. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- include/linux/hid.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.17.2