Re: [RFC 3/5] hid: Parse the device before adding it

[Benjamin Tissoires <benjamin.tissoires@gmail.com>]

Hi Henrik,

On Tue, Apr 3, 2012 at 09:05, Henrik Rydberg <rydberg@euromail.se> wrote:
> The hid bus is populated by devices created by the usb and bluetooth
> subsystems. The hid device is then broadcast to userland via uevents.
> Currently, the parsing of the hid reports is done during probe of
> the hid device, after the device has been broadcast. In order to
> allow for the report descriptors to influence the device properties,
> it is desirable to parse the device _before_ it is broadcast to
> userland. In actuality, the parsing depends only accidentally on
> the driver being present, so it can be trivially achieved.
>
> Something also needs to be done for the report_fixup handler, which
> seems to be the only hard device-driver coupling in the code.
>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hid/hid-core.c        |    2 +-
>  drivers/hid/usbhid/hid-core.c |    5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 35ba9d9..8a7b59e 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -658,7 +658,7 @@ int hid_parse_report(struct hid_device *device, __u8 *start,
>                hid_parser_reserved
>        };
>
> -       if (device->driver->report_fixup)
> +       if (device->driver && device->driver->report_fixup)
>                start = device->driver->report_fixup(device, start, &size);

Well, this is not a good solution for the following drivers:
hid-zydacron
hid-prodikeys
hid-uclogic
hid-samsung
hid-sony, etc...

they all use report_fixup and as the parse is made only once, they
won't be able to work properly.


>
>        device->rdesc = kmemdup(start, size, GFP_KERNEL);
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 5bf91db..e63613b 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1266,6 +1266,11 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>        if (usb_string(dev, dev->descriptor.iSerialNumber, hid->uniq, 64) <= 0)
>                hid->uniq[0] = 0;
>
> +       ret = hid_parse(hid);
> +       if (ret)
> +               goto err;

shouldn't we do the same for hidp?

Thanks,
Benjamin

> +
> +
>        usbhid = kzalloc(sizeof(*usbhid), GFP_KERNEL);
>        if (usbhid == NULL) {
>                ret = -ENOMEM;
> --
> 1.7.9.5
>