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

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

[Nikolai Kondrashov]

Add "have_special_driver" hid module parameter - a list of additional HID
devices which have specialized driver on HID bus. Needed to support out-of-tree
HID drivers.

Signed-off-by: Nikolai Kondrashov <spbnick@gmail.com>
---

I plan to use this with a DKMS package of my tablet drivers, which haven't yet
appeared in the end-user distributions. I'll be submitting them to the kernel
once they are tested, as usual.

 drivers/hid/hid-core.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 79 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index af08ce7..829d9a1 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -50,6 +50,21 @@ module_param_named(debug, hid_debug, int, 0600);
 MODULE_PARM_DESC(debug, "toggle HID debugging messages");
 EXPORT_SYMBOL_GPL(hid_debug);
 
+#define MAX_HAVE_SPECIAL_DRIVER_EXTRA 4
+
+static char *hid_have_special_driver_param[MAX_HAVE_SPECIAL_DRIVER_EXTRA] = {
+		NULL,
+};
+module_param_array_named(have_special_driver, hid_have_special_driver_param,
+			 charp, NULL, 0444);
+MODULE_PARM_DESC(have_special_driver,
+		 "A comma-separated list of additional HID devices which have "
+		 "specialized driver; each device is described as BUS:VID:PID, "
+		 "where BUS is either \"usb\" or \"bluetooth\" and both VID "
+		 "and PID are 0x-prefixed hex numbers, e.g. "
+		 "have_special_driver=bluetooth:0x1234:0x1234,"
+		 "usb:0x4321:0x4321");
+
 /*
  * Register a new report for a device.
  */
@@ -1578,6 +1593,13 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ }
 };
 
+/*
+ * An additional, load-time supplied list of devices which have specialized
+ * driver on HID bus.
+ */
+static struct hid_device_id hid_have_special_driver_extra[
+				MAX_HAVE_SPECIAL_DRIVER_EXTRA + 1] = { { } };
+
 struct hid_dynid {
 	struct list_head list;
 	struct hid_device_id id;
@@ -1668,7 +1690,8 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
 
 	/* generic wants all that don't have specialized driver */
 	if (!strncmp(hdrv->name, "generic-", 8))
-		return !hid_match_id(hdev, hid_have_special_driver);
+		return !hid_match_id(hdev, hid_have_special_driver) &&
+		       !hid_match_id(hdev, hid_have_special_driver_extra);
 
 	return 1;
 }
@@ -2167,6 +2190,55 @@ int hid_check_keys_pressed(struct hid_device *hid)
 
 EXPORT_SYMBOL_GPL(hid_check_keys_pressed);
 
+static int __init hid_parse_device_id_list(struct hid_device_id *id_list,
+					   char **str_list,
+					   size_t max)
+{
+	static const char *usb_str = "usb";
+	static const char *bluetooth_str = "bluetooth";
+
+	size_t i;
+	const char *p;
+	const char *t;
+	int n;
+	u16 bus;
+	u32 vid, pid;
+
+	for (i = 0; i < max && str_list[i] != NULL; i++) {
+		p = str_list[i];
+
+		t = strchr(p, ':');
+		if (t == NULL)
+			goto err;
+
+		if ((t - p) == strlen(usb_str) &&
+		    memcmp(p, usb_str, t - p) == 0)
+			bus = BUS_USB;
+		else if ((t - p) == strlen(bluetooth_str) &&
+			 memcmp(p, bluetooth_str, t - p) == 0)
+			bus = BUS_BLUETOOTH;
+		else
+			goto err;
+
+		p = t + 1;
+
+		if (sscanf(p, "0x%x:0x%x%n", &vid, &pid, &n) != 2)
+			goto err;
+
+		p += n;
+		if (*p != '\0')
+			goto err;
+
+		id_list[i] = (struct hid_device_id){HID_DEVICE(bus, vid, pid)};
+	}
+
+	id_list[i].bus = 0;
+	return 0;
+err:
+	id_list[0].bus = 0;
+	return 1;
+}
+
 static int __init hid_init(void)
 {
 	int ret;
@@ -2175,6 +2247,12 @@ static int __init hid_init(void)
 		pr_warn("hid_debug is now used solely for parser and driver debugging.\n"
 			"debugfs is now used for inspecting the device (report descriptor, reports)\n");
 
+	ret = hid_parse_device_id_list(hid_have_special_driver_extra,
+				       hid_have_special_driver_param,
+				       MAX_HAVE_SPECIAL_DRIVER_EXTRA);
+	if (ret)
+		pr_warn("invalid have_special_driver module parameter\n");
+
 	ret = bus_register(&hid_bus_type);
 	if (ret) {
 		pr_err("can't register hid bus\n");
-- 
1.7.9

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

[Nikolai Kondrashov]

Hi Jiri,

On Mon, Feb 20, 2012 at 11:29 PM, Nikolai Kondrashov <spbnick@gmail.com> wrote:
> Add "have_special_driver" hid module parameter - a list of additional HID
> devices which have specialized driver on HID bus. Needed to support out-of-tree
> HID drivers.

Did you have time to take a look at the patch?
If there's anything wrong with it, please tell me and I'll fix it.

Thanks :)

Sincerely,
Nick

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

[Jiri Kosina]

On Mon, 27 Feb 2012, Nikolai Kondrashov wrote:

> > Add "have_special_driver" hid module parameter - a list of additional 
> > HID devices which have specialized driver on HID bus. Needed to 
> > support out-of-tree HID drivers.
> 
> Did you have time to take a look at the patch?
> If there's anything wrong with it, please tell me and I'll fix it.

Hi Nikolai,

well, I am not really sure what value does it provide to have such 
functionality in the kernel.

I can understand that it helps you when developing out-of-tree (yet) 
drivers ... but as you are patching the kernel anyway with those drivers, 
you can easily run a kernel that has hid_have_special_driver[] array 
adjusted accordingly. And once you are submitting the driver for 
inclusion, you are of course submitting it together with 
hid_have_special_driver[] addition.

So it doesn't seem to be a debugging facility to me, and neither does it 
provide any added value for users of vanilla kernel.

Hmm?

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Nikolai Kondrashov]

Hi Jiri,

On 02/28/2012 04:30 PM, Jiri Kosina wrote:
> well, I am not really sure what value does it provide to have such
> functionality in the kernel.
>
> I can understand that it helps you when developing out-of-tree (yet)
> drivers ...

It doesn't just help, it makes them possible. Without this parameter it is
not possible to have out-of-tree HID drivers. Currently, a driver won't be
used if it isn't in the hid_have_special_driver array, as you probably know.

> but as you are patching the kernel anyway with those drivers,
> you can easily run a kernel that has hid_have_special_driver[] array
> adjusted accordingly.

Me yes, but I can't expect tablet users to patch and build their kernels
every time their distributions update them.

> And once you are submitting the driver for inclusion, you are of course
> submitting it together with hid_have_special_driver[] addition.

Sure.

> So it doesn't seem to be a debugging facility to me, and neither does it
> provide any added value for users of vanilla kernel.

The current workflow is this:

1. The user buys a new tablet, which is not yet supported by the kernel.
2. I add and submit kernel support for it.

Now, until my patch gets into the user's distribution kernel (which takes
3 months minimum, but usually about 5), he/she should build a kernel with my
patch *every* time the distribution updates it, if the user whishes to keep
up with the security updates and have a working tablet.

This is a bit too much to expect from the regular graphics tablet users.

Ubuntu, for example, does lots of kernel releases even for LTS
distributions. Then, there are users who want to stay on an older kernel for
some reasons.

My plan is to have regular releases of a DKMS-enabled driver package, which
once installed is rebuilt and installed automatically with each kernel
update. The only additional required action would be putting "options hid
have_special_driver=usb:VID:PID" into modprobe.conf.

So, the full planned workflow is this:

1. The user buys a new tablet, which is not yet supported by the kernel.
2. I add and submit kernel support for it.
3. I release a new version of the DKMS-enabled driver package including
    support for the user's tablet.
4. The user downloads and installs the package variant for his distribution,
    say, a .deb.
5. The user adds the "options hid have_special_driver=usb:VID:PID" line into
    his modprobe.conf.
6. The tablet driver is automatically re-built and re-installed with each
    kernel update.
7. Once the distribution pushes a kernel release with his tablet driver, the
    user is free to remove the DKMS package.

It would also be useful to be able to send the user a DKMS package with an
experimental driver to have him easily install and test it. Currently I'm
forced to build whole kernel packages in these cases.

Another point in support of this change is that the usbhid module already
has "quirks" parameter, which is somewhat similar in purpose.

Thanks :)!

Sincerely,
Nick

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

[Nikolai Kondrashov]

On 02/28/2012 04:30 PM, Jiri Kosina wrote:
> well, I am not really sure what value does it provide to have such
> functionality in the kernel.
>
> I can understand that it helps you when developing out-of-tree (yet)
> drivers ... but as you are patching the kernel anyway with those drivers,
> you can easily run a kernel that has hid_have_special_driver[] array
> adjusted accordingly. And once you are submitting the driver for
> inclusion, you are of course submitting it together with
> hid_have_special_driver[] addition.
>
> So it doesn't seem to be a debugging facility to me, and neither does it
> provide any added value for users of vanilla kernel.

Maybe I did too extensive an explanation. Here is a shorter one.

This is not needed for development, but only for end users to be able to
load an out-of-tree driver before it becomes available in their
distribution.

Without this, they will need to build their own kernel with my patches every
time it is updated.

With this, they will even be able to use DKMS to have drivers built and
installed automatically with each kernel update.

My rationalization for this specific solution:
http://thread.gmane.org/gmane.linux.usb.general/58661/focus=58869

Sincerely,
Nick

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

[Nikolai Kondrashov]

Hi Jiri,

On 02/28/2012 04:30 PM, Jiri Kosina wrote:
> On Mon, 27 Feb 2012, Nikolai Kondrashov wrote:
>
>>> Add "have_special_driver" hid module parameter - a list of additional
>>> HID devices which have specialized driver on HID bus. Needed to
>>> support out-of-tree HID drivers.
>>
>> Did you have time to take a look at the patch?
>> If there's anything wrong with it, please tell me and I'll fix it.
>
> Hi Nikolai,
>
> well, I am not really sure what value does it provide to have such
> functionality in the kernel.

In addition to my other explanations, I know about 10 people who are using
my tablet drivers which are to be included into the 3.4 release. And these
are only those who contacted me. Now, they can't be helped, they will have
to build their own kernels until they can install an official one with the
drivers.

However, there are still many unsupported tablets and other devices which
could be made to work with a simple HID driver. If this or a similar
functionality is there, they will need to only build and install a few
modules, or just install a DKMS-enabled package for their distribution.
And the sooner that happens, the better.

I understand that this solution is somewhat hackish, but it matches the
generic HID driver's ignoring of devices. The only proper solution,
considering asynchronous module loading, would be to bind the generic HID
driver to all HID devices unconditionally and then rebind a specific driver
once it's loaded. Yet, I'm not sure how this would fit the device driver
model.

Thank you for your attention and sorry if I bother you unnecessarily :)

Sincerely,
Nick

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

[Jiri Kosina]

On Mon, 20 Feb 2012, Nikolai Kondrashov wrote:

> Add "have_special_driver" hid module parameter - a list of additional HID
> devices which have specialized driver on HID bus. Needed to support out-of-tree
> HID drivers.
> 
> Signed-off-by: Nikolai Kondrashov <spbnick@gmail.com>
> ---
> 
> I plan to use this with a DKMS package of my tablet drivers, which haven't yet
> appeared in the end-user distributions. I'll be submitting them to the kernel
> once they are tested, as usual.
> 
>  drivers/hid/hid-core.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 79 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index af08ce7..829d9a1 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -50,6 +50,21 @@ module_param_named(debug, hid_debug, int, 0600);
>  MODULE_PARM_DESC(debug, "toggle HID debugging messages");
>  EXPORT_SYMBOL_GPL(hid_debug);
>  
> +#define MAX_HAVE_SPECIAL_DRIVER_EXTRA 4
> +
> +static char *hid_have_special_driver_param[MAX_HAVE_SPECIAL_DRIVER_EXTRA] = {
> +		NULL,
> +};
> +module_param_array_named(have_special_driver, hid_have_special_driver_param,
> +			 charp, NULL, 0444);
> +MODULE_PARM_DESC(have_special_driver,
> +		 "A comma-separated list of additional HID devices which have "
> +		 "specialized driver; each device is described as BUS:VID:PID, "
> +		 "where BUS is either \"usb\" or \"bluetooth\" and both VID "
> +		 "and PID are 0x-prefixed hex numbers, e.g. "
> +		 "have_special_driver=bluetooth:0x1234:0x1234,"
> +		 "usb:0x4321:0x4321");
> +
>  /*
>   * Register a new report for a device.
>   */
> @@ -1578,6 +1593,13 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ }
>  };
>  
> +/*
> + * An additional, load-time supplied list of devices which have specialized
> + * driver on HID bus.
> + */
> +static struct hid_device_id hid_have_special_driver_extra[
> +				MAX_HAVE_SPECIAL_DRIVER_EXTRA + 1] = { { } };
> +
>  struct hid_dynid {
>  	struct list_head list;
>  	struct hid_device_id id;
> @@ -1668,7 +1690,8 @@ static int hid_bus_match(struct device *dev, struct device_driver *drv)
>  
>  	/* generic wants all that don't have specialized driver */
>  	if (!strncmp(hdrv->name, "generic-", 8))
> -		return !hid_match_id(hdev, hid_have_special_driver);
> +		return !hid_match_id(hdev, hid_have_special_driver) &&
> +		       !hid_match_id(hdev, hid_have_special_driver_extra);
>  
>  	return 1;
>  }
> @@ -2167,6 +2190,55 @@ int hid_check_keys_pressed(struct hid_device *hid)
>  
>  EXPORT_SYMBOL_GPL(hid_check_keys_pressed);
>  
> +static int __init hid_parse_device_id_list(struct hid_device_id *id_list,
> +					   char **str_list,
> +					   size_t max)
> +{
> +	static const char *usb_str = "usb";
> +	static const char *bluetooth_str = "bluetooth";

I find this a little bit hackish :) How about for example the userspace 
transport drivers for Bluetooth-LE that are currently in the works?

I'd very much prefer just using sysfs (bind/unbind) way of doing things if 
possible for this purpose. What do you think?

-- 
Jiri Kosina
SUSE Labs

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

[Nikolai Kondrashov]

Hi Jiri,

On Fri, Mar 30, 2012 at 4:49 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 20 Feb 2012, Nikolai Kondrashov wrote:
>> Add "have_special_driver" hid module parameter - a list of additional HID
>> devices which have specialized driver on HID bus. Needed to support out-of-tree
>> HID drivers.
>
> I find this a little bit hackish :) How about for example the userspace
> transport drivers for Bluetooth-LE that are currently in the works?

I like the idea and the implementation seems good, but I can't see how it
is related to the task of having out-of-tree HID drivers, sorry.

> I'd very much prefer just using sysfs (bind/unbind) way of doing things if
> possible for this purpose. What do you think?

To my shame I must admit I didn't know or forgot of this mechanism. Although
I did some research asking kernel developers at work about possibility to
rebind a different driver. They either forgot about it at the time too, or I
didn't ask properly.

I think, I can craft an udev rule which will detach a tablet from the
generic HID driver and attach it to my driver through sysfs.

Please forget about this patch for now, I will resume bothering you in case
it's not possible or not appropriate :)

Thanks!

Sincerely,
Nick

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

[David Herrmann]

Hi Nikolai

On Fri, Mar 30, 2012 at 5:22 PM, Nikolai Kondrashov <spbnick@gmail.com> wrote:
> Hi Jiri,
>
> On Fri, Mar 30, 2012 at 4:49 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> On Mon, 20 Feb 2012, Nikolai Kondrashov wrote:
>>> Add "have_special_driver" hid module parameter - a list of additional HID
>>> devices which have specialized driver on HID bus. Needed to support out-of-tree
>>> HID drivers.
>>
>> I find this a little bit hackish :) How about for example the userspace
>> transport drivers for Bluetooth-LE that are currently in the works?
>
> I like the idea and the implementation seems good, but I can't see how it
> is related to the task of having out-of-tree HID drivers, sorry.
>
>> I'd very much prefer just using sysfs (bind/unbind) way of doing things if
>> possible for this purpose. What do you think?
>
> To my shame I must admit I didn't know or forgot of this mechanism. Although
> I did some research asking kernel developers at work about possibility to
> rebind a different driver. They either forgot about it at the time too, or I
> didn't ask properly.
>
> I think, I can craft an udev rule which will detach a tablet from the
> generic HID driver and attach it to my driver through sysfs.
>
> Please forget about this patch for now, I will resume bothering you in case
> it's not possible or not appropriate :)

Rebinding works perfectly well for me. I use it all the time when
testing HID drivers. Furthermore, there is a dynid-implementation in
the kernel so when hacking on something like this I'd recommend using
this. It would also remove the hard limit that you currently have.

> Thanks!
>
> Sincerely,
> Nick

Regards
David

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

[Nikolai Kondrashov]

Hi Jiri,

On 03/30/2012 04:49 PM, Jiri Kosina wrote:
> On Mon, 20 Feb 2012, Nikolai Kondrashov wrote:
>> Add "have_special_driver" hid module parameter - a list of additional HID
>> devices which have specialized driver on HID bus. Needed to support out-of-tree
>> HID drivers.
>
> I'd very much prefer just using sysfs (bind/unbind) way of doing things if
> possible for this purpose. What do you think?

I've tried this and it is not suitable. At least not in the current shape.

In short, two reasons:

1. The HID report descriptor is parsed only once, on first device probe and
    the resulting data is kept within device structure. It is not parsed
    again after unbind/bind, thus report_fixup of my driver is not called and
    the reports are getting interpreted according to the old one, rendering
    the device (partially) unusable.

2. The udev rules and scripts needed to make it work in a plug'n'play manner
    suitable for users are considerably more hacky than this solution.

I'd like to ask you to accept this patch for now.  Would it still be
possible to get this patch into 3.4? I promise to look for a better
solution. First step would probably be to fix rebinding.

It seems that you don't have much time recently to review/accept my patches.
Would you direct me to someone else who could do this and thus reduce your
load?

Thanks you.

Sincerely,
Nick

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

[Nikolai Kondrashov]

Hi David,

On 03/31/2012 02:03 AM, David Herrmann wrote:
> Rebinding works perfectly well for me. I use it all the time when
> testing HID drivers.

Thank you.

I've tried it, but it doesn't work for me, because HID report descriptors
are not re-parsed after rebinding and I rely on report_fixup's a lot. Please
see my reply to Jiri for slightly more details:
http://thread.gmane.org/gmane.linux.kernel.input/23708/focus=24337

> Furthermore, there is a dynid-implementation in the kernel so when hacking
> on something like this I'd recommend using this. It would also remove the
> hard limit that you currently have.

Thanks, but this doesn't suit me currently. I don't implement report parsing
in my drivers, but rather describe every device protocol separately with a
report descriptor and rely on the generic driver for parsing. Thus, I save
in code complexity and maintenance, but loose in portability.

However, I plan to make a non-HID driver for Waltop tablets, using their
proprietary, HID-incompatible protocol and there I will probably use that.

Sincerely,
Nick

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

[Jiri Kosina]

On Sun, 1 Apr 2012, Nikolai Kondrashov wrote:

> > > Add "have_special_driver" hid module parameter - a list of additional HID
> > > devices which have specialized driver on HID bus. Needed to support
> > > out-of-tree
> > > HID drivers.
> > 
> > I'd very much prefer just using sysfs (bind/unbind) way of doing things if
> > possible for this purpose. What do you think?
> 
> I've tried this and it is not suitable. At least not in the current shape.

Hi Nikolai,

thanks for looking into this.

> In short, two reasons:
> 
> 1. The HID report descriptor is parsed only once, on first device probe and
>    the resulting data is kept within device structure. It is not parsed
>    again after unbind/bind, thus report_fixup of my driver is not called and
>    the reports are getting interpreted according to the old one, rendering
>    the device (partially) unusable.

Hmm, you are absolutely right. This is the thing we should fix first, 
instead of introducing more-or-less hackish workarounds.

I will be travelling for the whole day tomorrow, so I will look into this 
onboard the airplane and will try to come up with a fix.

> 2. The udev rules and scripts needed to make it work in a plug'n'play manner
>    suitable for users are considerably more hacky than this solution.

As this is solely for the purpose of out-of-tree drivers, I believe it's 
better to keep the hackery in userspace though.

> I'd like to ask you to accept this patch for now.  Would it still be 
> possible to get this patch into 3.4? I promise to look for a better 
> solution. First step would probably be to fix rebinding.

Let's see whether we can come up with proper rebinding fix quickly enough. 
I don't want to end up introducing module parameter that we don't need, as 
we'll have to be maintaining it for compatibility reasons forever.

> It seems that you don't have much time recently to review/accept my patches.
> Would you direct me to someone else who could do this and thus reduce your
> load?

It's just a matter of prioritizing the incoming queue. I believe I process 
your new drivers and support for new devices by in-tree drivers in a 
timely manner.

But this is a core infrastructure change, influencing how we aproach 
out-of-tree drivers, so we'd better be sure to get it right before 
merging (even more so as your solution introduces a userspace interface 
(module parameter) that we'll have to keep forever).

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Jiri Kosina]

On Sun, 1 Apr 2012, Nikolai Kondrashov wrote:

> However, I plan to make a non-HID driver for Waltop tablets, using their 
> proprietary, HID-incompatible protocol and there I will probably use 
> that.

Hi Nick,

is this for new tablets only, or are you planning to move the support from 
hid-waltop away into this new driver?

The ones currently supported by hid-waltop seem to be covered pretty 
nicely with only report descriptors fixups being necessary; is this still 
a limited support not covering the full device capabilities?

Just curious.

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[David Herrmann]

Hi Nikolai

On Sun, Apr 1, 2012 at 9:44 PM, Nikolai Kondrashov <spbnick@gmail.com> wrote:
> Hi David,
>
>
> On 03/31/2012 02:03 AM, David Herrmann wrote:
>>
>> Rebinding works perfectly well for me. I use it all the time when
>> testing HID drivers.
>
>
> Thank you.
>
> I've tried it, but it doesn't work for me, because HID report descriptors
> are not re-parsed after rebinding and I rely on report_fixup's a lot. Please
> see my reply to Jiri for slightly more details:
> http://thread.gmane.org/gmane.linux.kernel.input/23708/focus=24337
>
>
>> Furthermore, there is a dynid-implementation in the kernel so when hacking
>> on something like this I'd recommend using this. It would also remove the
>> hard limit that you currently have.
>
>
> Thanks, but this doesn't suit me currently. I don't implement report parsing
> in my drivers, but rather describe every device protocol separately with a
> report descriptor and rely on the generic driver for parsing. Thus, I save
> in code complexity and maintenance, but loose in portability.

I think you don't understand me correctly. I meant instead of using
some static global table in the module you could instead implement
something like the "new_id"/store_new_id()/dyn_list/dyn_lock mechanism
but for the special_driver-id-array. So one could use "echo vid:pid
>/sys/bus/hid/new_special_driver_id" and it would be added to a linked
list which is checked additionally to the static array.

Such a mechanism is already used by all kinds of buses, hid, usb,
pci... but only for the id-lists of drivers, not a global module list.
At least this would look less hackish than a fixed-length-array module
parameter ;)

> However, I plan to make a non-HID driver for Waltop tablets, using their
> proprietary, HID-incompatible protocol and there I will probably use that.
>
> Sincerely,
> Nick

Regards
David

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

[Nikolai Kondrashov]

Hi Jiri,

On 04/01/2012 10:49 PM, Jiri Kosina wrote:
> On Sun, 1 Apr 2012, Nikolai Kondrashov wrote:
>> However, I plan to make a non-HID driver for Waltop tablets, using their
>> proprietary, HID-incompatible protocol and there I will probably use
>> that.
>
> is this for new tablets only, or are you planning to move the support from
> hid-waltop away into this new driver?

For now, this is for higher-end tablets, which have reduced functionality in
the default mode. There are three such tablets among currently supported and
probably more among the ones I haven't seen yet.

> The ones currently supported by hid-waltop seem to be covered pretty
> nicely with only report descriptors fixups being necessary; is this still
> a limited support not covering the full device capabilities?

Yes. The higher-end tablets report in half or even less the resolution
available in proprietary mode. Then, not all the frame controls are
supported properly.

I have contacted Waltop in the beginning of the year and they agreed to help
and already sent me two samples. However, I'm trying to use their help
sparingly. Then, I can't say when I will find the time to do this driver.

Sincerely,
Nick

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

[Nikolai Kondrashov]

Hi David,

On 04/01/2012 11:10 PM, David Herrmann wrote:
> On Sun, Apr 1, 2012 at 9:44 PM, Nikolai Kondrashov<spbnick@gmail.com>  wrote:
>> Thanks, but this doesn't suit me currently. I don't implement report parsing
>> in my drivers, but rather describe every device protocol separately with a
>> report descriptor and rely on the generic driver for parsing. Thus, I save
>> in code complexity and maintenance, but loose in portability.
>
> I think you don't understand me correctly.

Yes, I didn't, sorry :)

> I meant instead of using some static global table in the module you could
> instead implement something like the
> "new_id"/store_new_id()/dyn_list/dyn_lock mechanism but for the
> special_driver-id-array. So one could use "echo vid:pid
>> /sys/bus/hid/new_special_driver_id" and it would be added to a linked
> list which is checked additionally to the static array.
>
> Such a mechanism is already used by all kinds of buses, hid, usb,
> pci... but only for the id-lists of drivers, not a global module list.
> At least this would look less hackish than a fixed-length-array module
> parameter ;)

Yes, it could be done like this. However, I think Jiri would reject it. It
basically has the same hackishness WRT the overall design, while having more
complicated implementation.

Then, at least for my purpose, it would be worse. Because it would be harder
to configure it permanently. With a module parameter you could just modify
modprobe configuration, but with a sysfs node, you would need to have some
sort of a script. Then cold-plugged devices won't work unless you care to
unbind the existing devices when adding an ID.

Sincerely,
Nick

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

[Nikolai Kondrashov]

On 04/01/2012 10:46 PM, Jiri Kosina wrote:
> On Sun, 1 Apr 2012, Nikolai Kondrashov wrote:
>> 1. The HID report descriptor is parsed only once, on first device probe and
>>     the resulting data is kept within device structure. It is not parsed
>>     again after unbind/bind, thus report_fixup of my driver is not called and
>>     the reports are getting interpreted according to the old one, rendering
>>     the device (partially) unusable.
>
> Hmm, you are absolutely right. This is the thing we should fix first,
> instead of introducing more-or-less hackish workarounds.

I agree. It's just I didn't think I could fix it quickly and didn't expect
anyone to be interested in fixing it for me. Thus, I proposed accepting this
solution for now.

> I will be travelling for the whole day tomorrow, so I will look into this
> onboard the airplane and will try to come up with a fix.

Great! Thank you :)!

>> 2. The udev rules and scripts needed to make it work in a plug'n'play manner
>>     suitable for users are considerably more hacky than this solution.
>
> As this is solely for the purpose of out-of-tree drivers, I believe it's
> better to keep the hackery in userspace though.

I tend to agree. After all, decision on which module to load lies on
userspace, so it might as well decide which one to use. Anyway, I haven't
implemented it fully yet and am still to see if it's really that bad.

>> I'd like to ask you to accept this patch for now.  Would it still be
>> possible to get this patch into 3.4? I promise to look for a better
>> solution. First step would probably be to fix rebinding.
>
> Let's see whether we can come up with proper rebinding fix quickly enough.
> I don't want to end up introducing module parameter that we don't need, as
> we'll have to be maintaining it for compatibility reasons forever.

Sure. I must admit, I didn't think about maintaining the compatibility,
sorry.

>> It seems that you don't have much time recently to review/accept my patches.
>> Would you direct me to someone else who could do this and thus reduce your
>> load?
>
> It's just a matter of prioritizing the incoming queue. I believe I process
> your new drivers and support for new devices by in-tree drivers in a
> timely manner.

Sorry, I didn't mean to complain. It's just we didn't get to discuss this
patch properly until more than a month after I submitted it and then the
Waltop Sirius patch didn't make it into 3.4, so I thought maybe I'm not
supposed to send everything to you and there is some other way.

Thank you for reviewing my patches all this time.

> But this is a core infrastructure change, influencing how we aproach
> out-of-tree drivers, so we'd better be sure to get it right before
> merging (even more so as your solution introduces a userspace interface
> (module parameter) that we'll have to keep forever).

I agree.

Thank you.

Sincerely,
Nick

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

[Jiri Kosina]

On Sun, 1 Apr 2012, Nikolai Kondrashov wrote:

> > I will be travelling for the whole day tomorrow, so I will look into this
> > onboard the airplane and will try to come up with a fix.
> 
> Great! Thank you :)!

Please let me know whether the patch below fixes the rebinding for your 
drivers that replace the report descriptor. I believe it should. Thanks in 
advance for testing.



From: Jiri Kosina <jkosina@suse.cz>
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@suse.cz>
---
 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_device *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)
-- 
Jiri Kosina
SUSE Labs

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

[Henrik Rydberg]

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

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

[Jiri Kosina]

On Tue, 3 Apr 2012, Henrik Rydberg wrote:

> >  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:

Yes, that's identical.

[ ... snip ... ]
> which makes me wonder if something will break or be called
> unnecessarily often as a result?

I don't currently see how such thing could happen. Do you have anything 
particular on your mind?

> 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.

It probably should, yes. One of the reasons supporting this is that during 
USB reset (for example), the descriptors are reread from the device, which 
has a potential to cause a mismatch if a full rebind cycle is not 
performed. (I have a patch in my queue that tries to fix this, but moving 
the fixups to ll drivers is still something I will be considering).

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Henrik Rydberg]

> > which makes me wonder if something will break or be called
> > unnecessarily often as a result?
> 
> I don't currently see how such thing could happen. Do you have anything 
> particular on your mind?

Only that there ought to have been a reason the flag was added in the
first place.

> > 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.
> 
> It probably should, yes. One of the reasons supporting this is that during 
> USB reset (for example), the descriptors are reread from the device, which 
> has a potential to cause a mismatch if a full rebind cycle is not 
> performed. (I have a patch in my queue that tries to fix this, but moving 
> the fixups to ll drivers is still something I will be considering).

Sounds good, it will also pave the way for the modalias patches.

Thanks,
Henrik

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

[Jiri Kosina]

On Tue, 3 Apr 2012, Henrik Rydberg wrote:

> > It probably should, yes. One of the reasons supporting this is that during 
> > USB reset (for example), the descriptors are reread from the device, which 
> > has a potential to cause a mismatch if a full rebind cycle is not 
> > performed. (I have a patch in my queue that tries to fix this, but moving 
> > the fixups to ll drivers is still something I will be considering).
> 
> Sounds good, it will also pave the way for the modalias patches.

A proper and clean implementation will have to be thought about though.

I specifically want to avoid situation when we'll be heading towards 
"let's put all quirk handling into one driver under giant VID/PID-based 
switch", as getting rid of that was the very original pulse to have HID 
bus introduced.

-- 
Jiri Kosina
SUSE Labs

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

[Henrik Rydberg]

On Tue, Apr 03, 2012 at 10:01:37AM -0700, Jiri Kosina wrote:
> On Tue, 3 Apr 2012, Henrik Rydberg wrote:
> 
> > > It probably should, yes. One of the reasons supporting this is that during 
> > > USB reset (for example), the descriptors are reread from the device, which 
> > > has a potential to cause a mismatch if a full rebind cycle is not 
> > > performed. (I have a patch in my queue that tries to fix this, but moving 
> > > the fixups to ll drivers is still something I will be considering).
> > 
> > Sounds good, it will also pave the way for the modalias patches.
> 
> A proper and clean implementation will have to be thought about though.
> 
> I specifically want to avoid situation when we'll be heading towards 
> "let's put all quirk handling into one driver under giant VID/PID-based 
> switch", as getting rid of that was the very original pulse to have HID 
> bus introduced.

Absolutely. Less in-kernel lookups, less complex switches and more modularity.

Henrik

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

[Nikolai Kondrashov]

Hi Jiri,

On 04/03/2012 02:41 AM, Jiri Kosina wrote:
> On Sun, 1 Apr 2012, Nikolai Kondrashov wrote:
>
>>> I will be travelling for the whole day tomorrow, so I will look into this
>>> onboard the airplane and will try to come up with a fix.
>>
>> Great! Thank you :)!
>
> Please let me know whether the patch below fixes the rebinding for your
> drivers that replace the report descriptor. I believe it should. Thanks in
> advance for testing.

It seems it doesn't work. The report_fixup is called, but the parsed report
descriptor doesn't get fixed, at least as seen from debugfs.

I didn't have much time to see why this happens, but I will try to do so
today or tomorrow.

Sincerely,
Nick

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

[Nikolai Kondrashov]

Hi Jiri,

On 04/03/2012 02:41 AM, Jiri Kosina wrote:
> On Sun, 1 Apr 2012, Nikolai Kondrashov wrote:
>
>>> I will be travelling for the whole day tomorrow, so I will look into this
>>> onboard the airplane and will try to come up with a fix.
>>
>> Great! Thank you :)!
>
> Please let me know whether the patch below fixes the rebinding for your
> drivers that replace the report descriptor. I believe it should. Thanks in
> advance for testing.

On a closer look, it seems simply freeing rdesc is far from enough. There
are lots of fields in the hid_device structure which describe the reports
and which are filled during report parsing. Namely: collection,
collection_size, maxcollection, maxapplication, report_enum, and likely
others. Parsing a new descriptor over the values of these variables,
corresponding to the previous descriptor, doesn't produce a meaningful
result.

These fields should be reset and linked data freed before a new report
descriptor could be parsed.

Sincerely,
Nick