Re: [PATCH 1/1] HID: add have_special_driver hid module parameter

Hi Jiri,

From: Jiri Kosina <redacted>
Subject: [PATCH] HID: make hid_parse() reentrant

It is possible to rebind the drivers on HID bus manually from userspace
(through sysfs bind/unbind facility). This way, we can easily allow drivers to
claim device IDs even if this doesn't happen automatically through explicit
hid_have_special_driver[] entry.

If driver is rebound, hid_parse() sees the HID_STATE_PARSED flag set, and
doesn't proceed parsing the report descriptor again.

This might however be unwanted in cases when the newly rebound driver does
a report descriptor fixup, as it doesn't get parsed again with the replaced
values.

Instead of bailing out directly when HID_STAT_PARSED flag is set, throw
away the old report descriptor stored in hid_device->rdesc, and let the
whole rdesc parsing be restarted (with ->report_fixup callback of the newly
rebound driver being called prior to the actual parsing).

Signed-off-by: Jiri Kosina <redacted>
---
 include/linux/hid.h |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

It seems an equivalent patch would be to remove HID_STAT_PARSED
altogether, replacing it with something like this:

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 1e68543..456fdbf 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -454,7 +454,6 @@ struct hid_output_fifo {
 #define HID_CLAIMED_HIDRAW     4
 
 #define HID_STAT_ADDED         1
-#define HID_STAT_PARSED                2
 
 struct hid_input {
        struct list_head list;

@@ -811,16 +810,10 @@ static inline void hid_map_usage_clear(struct hid_input *hidinput,
  */
 static inline int __must_check hid_parse(struct hid_device *hdev)
 {
-       int ret;
+       kfree(hdev->rdesc);
+       hdev->rdesc = NULL;
 
-       if (hdev->status & HID_STAT_PARSED)
-               return 0;
-
-       ret = hdev->ll_driver->parse(hdev);
-       if (!ret)
-               hdev->status |= HID_STAT_PARSED;
-
-       return ret;
+       return hdev->ll_driver->parse(hdev);
 }

which makes me wonder if something will break or be called
unnecessarily often as a result?

I think the main logic problem stems from viewing hid devices as being
on the same level as usb/bt devices.  Perhaps report fixups should be
part of the hid_ll_driver layer instead.

Thanks,
Henrik