Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[Henrik Rydberg]

Hi David,

Perhaps one should make hid-core warn instead of bailing out instead?
Something may need to be done to make sure everything works smoothly,
but really, why would hid-core mind keeping a device on the bus with a
non-standard io driver?

Thanks,
Henrik

Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[David Herrmann]

Hi Henrik

On Mon, Jul 16, 2012 at 8:45 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi David,
>
> Perhaps one should make hid-core warn instead of bailing out instead?
> Something may need to be done to make sure everything works smoothly,
> but really, why would hid-core mind keeping a device on the bus with a
> non-standard io driver?

No, I think bailing out is the correct thing to do. If we emit a
warning but leave the device on the bus, the device will have no
driver loaded and idling in background. Hence, dropping the device is
the best solution for 99% of the devices.

However, devices that explicitly do not require the generic drivers
(like picolcd and wiimote) should be able to stay on the bus. We could
of course put them out of the HID subsystem as they have very little
in common with the other HID drivers, but that wouldn't make the
situation any better. It would mean writing transport-level drivers
for picolcd and wiimote which are exactly the same as USBHID and HIDP.
Hence, I think allowing non-standard drivers on the BUS is ok as long
as they explicitly request it.

Thanks
David

Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[Henrik Rydberg]

On Mon, Jul 16, 2012 at 09:08:47PM +0200, David Herrmann wrote:
> Hi Henrik
> 
> On Mon, Jul 16, 2012 at 8:45 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> > Hi David,
> >
> > Perhaps one should make hid-core warn instead of bailing out instead?
> > Something may need to be done to make sure everything works smoothly,
> > but really, why would hid-core mind keeping a device on the bus with a
> > non-standard io driver?
> 
> No, I think bailing out is the correct thing to do. If we emit a
> warning but leave the device on the bus, the device will have no
> driver loaded and idling in background. Hence, dropping the device is
> the best solution for 99% of the devices.

If the driver that picks up the device (wiimote for instance) decides
it does not want to use any of the io drivers (HID_CONNECT*) by
calling hid_hw_start(hdev, 0), in what way is that different from
calling it with, say, hid_hw_start(hdev, HID_CONNECT_TO_OTHER)?  (The
latter would be an equivalent (but cleaner) way of setting your
HID_CLAIMED_OTHER flag).

To catch possible mistakes, one could check for the presence of
raw_event() instead, for instance.

What I am getting at is that we really do not need to create any more
backdoors into the hid core - on the contrary, we can most likely
easily remove some of them instead.

> However, devices that explicitly do not require the generic drivers
> (like picolcd and wiimote) should be able to stay on the bus. We could
> of course put them out of the HID subsystem as they have very little
> in common with the other HID drivers, but that wouldn't make the
> situation any better. It would mean writing transport-level drivers
> for picolcd and wiimote which are exactly the same as USBHID and HIDP.
> Hence, I think allowing non-standard drivers on the BUS is ok as long
> as they explicitly request it.

Yes, the question is only what that request looks like. The simpler
the better, don't you agree?

Thanks,
Henrik

Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[David Herrmann]

Hi Henrik

On Mon, Jul 16, 2012 at 9:35 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Mon, Jul 16, 2012 at 09:08:47PM +0200, David Herrmann wrote:
>> Hi Henrik
>>
>> On Mon, Jul 16, 2012 at 8:45 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > Hi David,
>> >
>> > Perhaps one should make hid-core warn instead of bailing out instead?
>> > Something may need to be done to make sure everything works smoothly,
>> > but really, why would hid-core mind keeping a device on the bus with a
>> > non-standard io driver?
>>
>> No, I think bailing out is the correct thing to do. If we emit a
>> warning but leave the device on the bus, the device will have no
>> driver loaded and idling in background. Hence, dropping the device is
>> the best solution for 99% of the devices.
>
> If the driver that picks up the device (wiimote for instance) decides
> it does not want to use any of the io drivers (HID_CONNECT*) by
> calling hid_hw_start(hdev, 0), in what way is that different from
> calling it with, say, hid_hw_start(hdev, HID_CONNECT_TO_OTHER)?  (The
> latter would be an equivalent (but cleaner) way of setting your
> HID_CLAIMED_OTHER flag).

I am sorry, I misunderstood you. Yes, in fact, this is what wiimote
currently does. Well, it uses HID_CONNECT_HIDRAW but this has no
effect if CONFIG_HIDRAW is not set so this is equivalent to 0. My
first attempt was to make this work, however, this means refactoring
hid_connect() a lot as we need to differentiate between
hidraw_connect() failing and HID_CONNECT_HIDRAW being not set. Same
for hidinput_connect() and hiddev_connect(). That is, if
hid_hw_start() is called with HID_CONNECT_HIDDEV set, but
hiddev_connect() fails? Should the core bail out or let the device
through? In most cases bailing out is the best option. However, what
to do for the wiimote case? It requests hidraw but if hidraw_connect()
fails, then the wiimote driver can still work without it so in this
case we must not bail out.

Taking this into account I really have no idea how to implement this
in a cleaner and safer way than using HID_CONNECT_OTHER. Btw., what
should the wiimote driver pass to hid_hw_start() in your case? It
wants HID_CONNECT_HIDRAW but also wants to get through if
CONFIG_HIDRAW is not set. It cannot pass 0 as this would always
disable HIDRAW. But in the case that HIDRAW is not available, it
cannot tell the core that it wants to stay on the bus. Hence, I think
using HID_CONNECT_OTHER is the only way, isn't it?

> To catch possible mistakes, one could check for the presence of
> raw_event() instead, for instance.
>
> What I am getting at is that we really do not need to create any more
> backdoors into the hid core - on the contrary, we can most likely
> easily remove some of them instead.

I fully agree, but I have to admit that I didn't find an easier way
that actually works.

Thanks a lot for having a look at this. If you have any other ideas I
will gladly implement and test them, but I am currently out of ideas.
Regards
David

Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[Henrik Rydberg]

> I am sorry, I misunderstood you. Yes, in fact, this is what wiimote
> currently does. Well, it uses HID_CONNECT_HIDRAW but this has no
> effect if CONFIG_HIDRAW is not set so this is equivalent to 0. My
> first attempt was to make this work, however, this means refactoring
> hid_connect() a lot as we need to differentiate between
> hidraw_connect() failing and HID_CONNECT_HIDRAW being not set. Same
> for hidinput_connect() and hiddev_connect(). That is, if
> hid_hw_start() is called with HID_CONNECT_HIDDEV set, but
> hiddev_connect() fails? Should the core bail out or let the device
> through? In most cases bailing out is the best option. However, what
> to do for the wiimote case? It requests hidraw but if hidraw_connect()
> fails, then the wiimote driver can still work without it so in this
> case we must not bail out.
> 
> Taking this into account I really have no idea how to implement this
> in a cleaner and safer way than using HID_CONNECT_OTHER. Btw., what
> should the wiimote driver pass to hid_hw_start() in your case? It
> wants HID_CONNECT_HIDRAW but also wants to get through if
> CONFIG_HIDRAW is not set. It cannot pass 0 as this would always
> disable HIDRAW. But in the case that HIDRAW is not available, it
> cannot tell the core that it wants to stay on the bus. Hence, I think
> using HID_CONNECT_OTHER is the only way, isn't it?
> 
> > To catch possible mistakes, one could check for the presence of
> > raw_event() instead, for instance.
> >
> > What I am getting at is that we really do not need to create any more
> > backdoors into the hid core - on the contrary, we can most likely
> > easily remove some of them instead.
> 
> I fully agree, but I have to admit that I didn't find an easier way
> that actually works.
> 
> Thanks a lot for having a look at this. If you have any other ideas I
> will gladly implement and test them, but I am currently out of ideas.

Would something like this work for you?

Henrik

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 4c87276..a43e14c 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1373,8 +1373,8 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 	if ((connect_mask & HID_CONNECT_HIDRAW) && !hidraw_connect(hdev))
 		hdev->claimed |= HID_CLAIMED_HIDRAW;
 
-	if (!hdev->claimed) {
-		hid_err(hdev, "claimed by neither input, hiddev nor hidraw\n");
+	if (!hdev->claimed && !hdev->driver->raw_event) {
+		hid_err(hdev, "device has no listeners, quitting\n");
 		return -ENODEV;
 	}
 
diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 45c3433..74c388d 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -2613,11 +2613,7 @@ static int picolcd_probe(struct hid_device *hdev,
 		goto err_cleanup_data;
 	}
 
-	/* We don't use hidinput but hid_hw_start() fails if nothing is
-	 * claimed. So spoof claimed input. */
-	hdev->claimed = HID_CLAIMED_INPUT;
 	error = hid_hw_start(hdev, 0);
-	hdev->claimed = 0;
 	if (error) {
 		hid_err(hdev, "hardware start failed\n");
 		goto err_cleanup_data;

Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[David Herrmann]

Hi Henrik

On Mon, Jul 16, 2012 at 10:59 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> I am sorry, I misunderstood you. Yes, in fact, this is what wiimote
>> currently does. Well, it uses HID_CONNECT_HIDRAW but this has no
>> effect if CONFIG_HIDRAW is not set so this is equivalent to 0. My
>> first attempt was to make this work, however, this means refactoring
>> hid_connect() a lot as we need to differentiate between
>> hidraw_connect() failing and HID_CONNECT_HIDRAW being not set. Same
>> for hidinput_connect() and hiddev_connect(). That is, if
>> hid_hw_start() is called with HID_CONNECT_HIDDEV set, but
>> hiddev_connect() fails? Should the core bail out or let the device
>> through? In most cases bailing out is the best option. However, what
>> to do for the wiimote case? It requests hidraw but if hidraw_connect()
>> fails, then the wiimote driver can still work without it so in this
>> case we must not bail out.
>>
>> Taking this into account I really have no idea how to implement this
>> in a cleaner and safer way than using HID_CONNECT_OTHER. Btw., what
>> should the wiimote driver pass to hid_hw_start() in your case? It
>> wants HID_CONNECT_HIDRAW but also wants to get through if
>> CONFIG_HIDRAW is not set. It cannot pass 0 as this would always
>> disable HIDRAW. But in the case that HIDRAW is not available, it
>> cannot tell the core that it wants to stay on the bus. Hence, I think
>> using HID_CONNECT_OTHER is the only way, isn't it?
>>
>> > To catch possible mistakes, one could check for the presence of
>> > raw_event() instead, for instance.
>> >
>> > What I am getting at is that we really do not need to create any more
>> > backdoors into the hid core - on the contrary, we can most likely
>> > easily remove some of them instead.
>>
>> I fully agree, but I have to admit that I didn't find an easier way
>> that actually works.
>>
>> Thanks a lot for having a look at this. If you have any other ideas I
>> will gladly implement and test them, but I am currently out of ideas.
>
> Would something like this work for you?

Yes, that works fine. I just want some nice comment there so people
know we are using some heuristics. If you don't mind, I will wrap this
up tomorrow and send a new version of the patch(set). Otherwise, feel
free to send it yourself.

Thanks for your reviews!
David

> Henrik
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 4c87276..a43e14c 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1373,8 +1373,8 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
>         if ((connect_mask & HID_CONNECT_HIDRAW) && !hidraw_connect(hdev))
>                 hdev->claimed |= HID_CLAIMED_HIDRAW;
>
> -       if (!hdev->claimed) {
> -               hid_err(hdev, "claimed by neither input, hiddev nor hidraw\n");
> +       if (!hdev->claimed && !hdev->driver->raw_event) {
> +               hid_err(hdev, "device has no listeners, quitting\n");
>                 return -ENODEV;
>         }
>
> diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> index 45c3433..74c388d 100644
> --- a/drivers/hid/hid-picolcd.c
> +++ b/drivers/hid/hid-picolcd.c
> @@ -2613,11 +2613,7 @@ static int picolcd_probe(struct hid_device *hdev,
>                 goto err_cleanup_data;
>         }
>
> -       /* We don't use hidinput but hid_hw_start() fails if nothing is
> -        * claimed. So spoof claimed input. */
> -       hdev->claimed = HID_CLAIMED_INPUT;
>         error = hid_hw_start(hdev, 0);
> -       hdev->claimed = 0;
>         if (error) {
>                 hid_err(hdev, "hardware start failed\n");
>                 goto err_cleanup_data;

Re: [PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[Henrik Rydberg]

> Yes, that works fine. I just want some nice comment there so people
> know we are using some heuristics. If you don't mind, I will wrap this
> up tomorrow and send a new version of the patch(set). Otherwise, feel
> free to send it yourself.

I don't mind at all, tomorrow it is.

Cheers,
Henrik

[PATCH 1/3] HID: Add HID_CLAIMED_OTHER for non-generic drivers

[David Herrmann]

All normal devices that correctly conform to the HID specs are handled by
the hid-input driver. However, for non-standard drivers we often don't
want hid-input to handle them but rather use specific hid drivers. But
hid_connect() requires at least one generic driver to claim the devices.
As hid_hw_start() calls hid_connect() we currently use an ugly workaround
by adding HID_CLAIMED_INPUT to hid->claimed and clear it after
hid_hw_start() again (see hid-picolcd.c for examples).

This adds the HID_CLAIMED_OTHER flag which can be set by device drivers to
tell the hid-core that they handle the device without the need of any
generic driver like hidinput, hiddev or hidraw.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 drivers/hid/hid-core.c | 2 ++
 include/linux/hid.h    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9cdc74e..2039f32 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1391,6 +1391,8 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 	if (hdev->claimed & HID_CLAIMED_HIDRAW)
 		len += sprintf(buf + len, "%shidraw%d", len ? "," : "",
 				((struct hidraw *)hdev->hidraw)->minor);
+	if (hdev->claimed & HID_CLAIMED_OTHER)
+		len += sprintf(buf + len, "%sother", len ? "," : "");
 
 	type = "Device";
 	for (i = 0; i < hdev->maxcollection; i++) {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 449fa38..26b336a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -452,6 +452,7 @@ struct hid_output_fifo {
 #define HID_CLAIMED_INPUT	1
 #define HID_CLAIMED_HIDDEV	2
 #define HID_CLAIMED_HIDRAW	4
+#define HID_CLAIMED_OTHER	8
 
 #define HID_STAT_ADDED		1
 #define HID_STAT_PARSED		2
-- 
1.7.11.2