diff mbox

The return value of hid_input_report and raw_event

Message ID 7774789.jK4HDCWEI4@al (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Peter Wu Dec. 15, 2014, 10:31 p.m. UTC
Hi Jiri and list,

The HID core has a hid_input_report function which returns an integer,
but all its callers are not really changing their behavior based on the
return value. The few^Wonly exception that does not completely ignore
the return value is the hid-logitech-dj driver which prints a debugging
message.

Should this method be changed to return void? The kerneldoc does not
document its return value anyway.

Confusion 2: the raw_event callback of the hid_driver structure returns
an int, but over time non-negative values are ignored by
hid_input_report. This happened in the following change:
--------------------------------------------------
commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina <jkosina@suse.cz>
Date:   Mon Jun 3 11:27:48 2013 +0200

    HID: core: fix reporting of raw events
    
    hdrw->raw event can return three different return value types:
    
    - ret < 0   indicates that the hdrv driver found an error while parsing
    - ret == 0  indicates no error has been encountered, and the driver has
                processed the report
    - ret > 0   indicates that there was no parsing error, and the driver hasn't
                processed the event.
    
    Calling hid_report_raw_event() has to be called appropriately so that it
    reflects what has been done by ->raw_event() callback, otherwise we might
    updates of the in-kernel structure are lost upon arrival of the report, which
    is wrong.
    
    Reported-and-tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
    Reported-and-tested-by: Daniel Leung <daniel.leung@linux.intel.com>
    Signed-off-by: Jiri Kosina <jkosina@suse.cz>

--------------------------------------------------

So now it does not matter whether the raw_event method returns 0 or 1.
The documentation says:

    raw_event and event should return 0 on no action performed, 1 when
    no further processing should be done and negative on error

What does "no further processing" mean? I guess that the intention was
to signal from a HID driver to ignore the packet, but that the above
change was done to update the state and allow hidraw to pick up the
report.

Comments

Jiri Kosina Dec. 16, 2014, 9:27 a.m. UTC | #1
On Mon, 15 Dec 2014, Peter Wu wrote:

> Hi Jiri and list,
> 
> The HID core has a hid_input_report function which returns an integer,
> but all its callers are not really changing their behavior based on the
> return value. The few^Wonly exception that does not completely ignore
> the return value is the hid-logitech-dj driver which prints a debugging
> message.
> 
> Should this method be changed to return void? The kerneldoc does not
> document its return value anyway.

Hi Peter,

yes, that definitely would be an acceptable cleanup.

> Confusion 2: the raw_event callback of the hid_driver structure returns
> an int, but over time non-negative values are ignored by
> hid_input_report. This happened in the following change:
> --------------------------------------------------
> commit b1a1442a23776756b254b69786848a94d92445ba
> Author: Jiri Kosina <jkosina@suse.cz>
> Date:   Mon Jun 3 11:27:48 2013 +0200
> 
>     HID: core: fix reporting of raw events
>     
>     hdrw->raw event can return three different return value types:
>     
>     - ret < 0   indicates that the hdrv driver found an error while parsing
>     - ret == 0  indicates no error has been encountered, and the driver has
>                 processed the report
>     - ret > 0   indicates that there was no parsing error, and the driver hasn't
>                 processed the event.
>     
>     Calling hid_report_raw_event() has to be called appropriately so that it
>     reflects what has been done by ->raw_event() callback, otherwise we might
>     updates of the in-kernel structure are lost upon arrival of the report, which
>     is wrong.
>     
>     Reported-and-tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>     Reported-and-tested-by: Daniel Leung <daniel.leung@linux.intel.com>
>     Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index c272078..8f616bd 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1293,7 +1293,7 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
>  
>         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:
> 
>     raw_event and event should return 0 on no action performed, 1 when
>     no further processing should be done and negative on error
> 
> What does "no further processing" mean? I guess that the intention was
> to signal from a HID driver to ignore the packet, but that the above
> change was done to update the state and allow hidraw to pick up the
> report.

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

See this thread

	https://lkml.org/lkml/2013/4/8/540

for an example.
diff mbox

Patch

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index c272078..8f616bd 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1293,7 +1293,7 @@  int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
 
        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;
                }