Recent change to hid-core.c

Recent change to hid-core.c

[Mark Lord]

Jiri,

The recent update 08ec2dcc3527a20c619aca2fb36f800908256bac
 "Merge branches 'for-3.11/multitouch', 'for-3.11/sony' and 'for-3.11/upstream' into for-linus"
included an unexpected change to the return code handing for ->raw_event() calls.

A HID driver's raw_event() method previously could return these values:
   0 --> keep processing.
   1 --> no further processing required.
   <0 --> error.

Now, "1" and "0" are both treated as "keep processing",
so a lower level HID driver has to return a negative error code
to achieve the "no further processing required" state.

Was this intentional?  Doesn't that have side-effects for some drivers?

Thanks
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

Re: Recent change to hid-core.c

[Jiri Kosina]

On Sun, 15 Dec 2013, Mark Lord wrote:

> The recent update 08ec2dcc3527a20c619aca2fb36f800908256bac
>  "Merge branches 'for-3.11/multitouch', 'for-3.11/sony' and 'for-3.11/upstream' into for-linus"
> included an unexpected change to the return code handing for ->raw_event() calls.
> 
> A HID driver's raw_event() method previously could return these values:
>    0 --> keep processing.
>    1 --> no further processing required.
>    <0 --> error.
> 
> Now, "1" and "0" are both treated as "keep processing",
> so a lower level HID driver has to return a negative error code
> to achieve the "no further processing required" state.
> 
> Was this intentional?  Doesn't that have side-effects for some drivers?

Hi Mark,

this was intentional -- please see commit b1a1442a23 and discussion here: 
https://lkml.org/lkml/2013/3/21/591

If you have any other concerns, please let me know.

-- 
Jiri Kosina
SUSE Labs

Re: Recent change to hid-core.c

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On 13-12-16 08:24 AM, Jiri Kosina wrote:
> On Sun, 15 Dec 2013, Mark Lord wrote:
> 
>> The recent update 08ec2dcc3527a20c619aca2fb36f800908256bac
>>  "Merge branches 'for-3.11/multitouch', 'for-3.11/sony' and 'for-3.11/upstream' into for-linus"
>> included an unexpected change to the return code handing for ->raw_event() calls.
,,
>> Was this intentional?  Doesn't that have side-effects for some drivers?
..
> this was intentional -- please see commit b1a1442a23 and discussion here: 
> https://lkml.org/lkml/2013/3/21/591
..

Thanks Jiri.
In that case you'll be wanting this patch (unmangled copy attached) as well
to clean up a tiny bit of leftover logic in there.

* * * * * * SNIP * * * * * *

Recent updates to raw_event handling resulted in some leftover "dead" logic
in hid-core.c::hid_input_report().  Nuke it.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/hid/hid-core.c	2013-12-12 01:38:07.000000000 -0500
+++ linux/drivers/hid/hid-core.c	2013-12-16 09:48:27.339162173 -0500
@@ -1418,10 +1418,8 @@

 	if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
 		ret = hdrv->raw_event(hid, report, data, size);
-		if (ret < 0) {
-			ret = ret < 0 ? ret : 0;
+		if (ret < 0)
 			goto unlock;
-		}
 	}

 	ret = hid_report_raw_event(hid, type, data, size, interrupt);

[-- Attachment #2: hid_core_remove_leftover_logic.patch --]
[-- Type: text/x-patch, Size: 599 bytes --]

Recent updates to raw_event handling resulted in some leftover "dead" logic
in hid-core.c::hid_input_report().  Nuke it.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- old/drivers/hid/hid-core.c	2013-12-12 01:38:07.000000000 -0500
+++ linux/drivers/hid/hid-core.c	2013-12-16 09:48:27.339162173 -0500
@@ -1418,10 +1418,8 @@
 
 	if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
 		ret = hdrv->raw_event(hid, report, data, size);
-		if (ret < 0) {
-			ret = ret < 0 ? ret : 0;
+		if (ret < 0)
 			goto unlock;
-		}
 	}
 
 	ret = hid_report_raw_event(hid, type, data, size, interrupt);

Re: Recent change to hid-core.c

[Jiri Kosina]

On Mon, 16 Dec 2013, Mark Lord wrote:

> >> The recent update 08ec2dcc3527a20c619aca2fb36f800908256bac
> >>  "Merge branches 'for-3.11/multitouch', 'for-3.11/sony' and 'for-3.11/upstream' into for-linus"
> >> included an unexpected change to the return code handing for ->raw_event() calls.
> ,,
> >> Was this intentional?  Doesn't that have side-effects for some drivers?
> ..
> > this was intentional -- please see commit b1a1442a23 and discussion here: 
> > https://lkml.org/lkml/2013/3/21/591
> ..
> 
> Thanks Jiri.
> In that case you'll be wanting this patch (unmangled copy attached) as well
> to clean up a tiny bit of leftover logic in there.
> 
> * * * * * * SNIP * * * * * *
> 
> Recent updates to raw_event handling resulted in some leftover "dead" logic
> in hid-core.c::hid_input_report().  Nuke it.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- old/drivers/hid/hid-core.c	2013-12-12 01:38:07.000000000 -0500
> +++ linux/drivers/hid/hid-core.c	2013-12-16 09:48:27.339162173 -0500
> @@ -1418,10 +1418,8 @@
> 
>  	if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
>  		ret = hdrv->raw_event(hid, report, data, size);
> -		if (ret < 0) {
> -			ret = ret < 0 ? ret : 0;
> +		if (ret < 0)
>  			goto unlock;
> -		}

Please see commit 556483e2a ("HID: remove self-assignment from 
hid_input_report") in Linus' tree.

-- 
Jiri Kosina
SUSE Labs