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

["Henrik Rydberg" <rydberg@euromail.se>]

Hi Jiri,

> From: Jiri Kosina <jkosina@xxxxxxx>
> 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 <jkosina@xxxxxxx>
> ---
>  include/linux/hid.h |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)

> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 3a95da6..f771eba 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -807,8 +807,16 @@ static inline int __must_check hid_parse(struct hid_devic> e *hdev)
>  {
>  	int ret;
>  
> -	if (hdev->status & HID_STAT_PARSED)
> -		return 0;
> +	if (hdev->status & HID_STAT_PARSED) {
> +		/*
> +		 * We want to be re-entrant to allow for dynamic driver
> +		 * rebinding and still allow rdescs to be replaced and
> +		 * and re-parsed once the driver has been dynamically
> +		 * rebound
> +		 */
> +		kfree(hdev->rdesc);
> +		hdev->status &= ~HID_STAT_PARSED;
> +	}
>  
>  	ret = hdev->ll_driver->parse(hdev);
>  	if (!ret)

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