diff mbox series

HID: fix up .raw_event() documentation

Message ID 20181104103247.32691-1-linus.walleij@linaro.org
State Accepted
Commit aa9b760cec2385ad408bb2e346c7f6dc1be69a79
Headers show
Series HID: fix up .raw_event() documentation | expand

Commit Message

Linus Walleij Nov. 4, 2018, 10:32 a.m. UTC
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

Comments

Jiri Kosina Nov. 6, 2018, 12:59 p.m. UTC | #1
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
Linus Walleij Nov. 12, 2018, 11:18 a.m. UTC | #2
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
Benjamin Tissoires Nov. 12, 2018, 11:51 a.m. UTC | #3
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 mbox series

Patch

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