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@xxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> Reported-and-tested-by: Daniel Leung <daniel.leung@xxxxxxxxxxxxxxx>
> Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
>
> 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.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
