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

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

Hi Henrik

On Mon, Jul 16, 2012 at 8:45 PM, Henrik Rydberg [off-list ref] 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

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 [off-list ref] wrote:

quoted

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

Hi Henrik

On Mon, Jul 16, 2012 at 9:35 PM, Henrik Rydberg [off-list ref] wrote:

On Mon, Jul 16, 2012 at 09:08:47PM +0200, David Herrmann wrote:

quoted

Hi Henrik

On Mon, Jul 16, 2012 at 8:45 PM, Henrik Rydberg [off-list ref] wrote:

quoted

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

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?

quoted

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;

Hi Henrik

On Mon, Jul 16, 2012 at 10:59 PM, Henrik Rydberg [off-list ref] wrote:

quoted

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?

quoted

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

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