LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [patch] hid: quirk to fixup fullspeed interval on highspeed devices
@ 2008-03-01 14:53 Pekka Sarnila
  2008-03-03 12:50 ` Jiri Kosina
  0 siblings, 1 reply; 4+ messages in thread
From: Pekka Sarnila @ 2008-03-01 14:53 UTC (permalink / raw)
  To: jkosina; +Cc: linux-kernel

From: Pekka Sarnila <sarnila@adit.fi>

Many vendors highspeed devices give erroneously fullspeed interval value in endpoint descriptor for interrupt endpoints. This quirk fixes up that by recalculating the right value for highspeed device.

---

This patch is against jikos/hid.git.

At the time of hid configuration this quirk calculates which highspeed interval value gives same interval delay as, or next smaller then, what it would be if the original value would be interpreted as fullspeed value. In subsequent urbs that new value is used instead.

Forming the 'hid->name' in usb_hid_config() was moved up to accommodate more descriptive printk reporting the fixup.

In this patch the quirk is set for one such device: Afatech DVB-T 2 infrared HID-keyboard. It reports value 16 which means 4,069s in highspeed while obviously 16ms was intended. In this case quirk calculates new value to be 8 which gives when interpreted as highspeed value 16ms as wanted. The behavior of the device was verified to be what expected both before and after the patch.

Actually better place for this fixup would be in the usb/core/config.c:usb_parse_endpoint(), but usb subsystem provides no quirk system, and there is no way for the parser to generally know that the value is wrong (except if the value is out of range; then fix is done there).

Signed-off-by: Pekka Sarnila <sarnila@adit.fi>


diff -uprN -X hid.orig/Documentation/dontdiff hid.orig/drivers/hid/usbhid/hid-core.c hid/drivers/hid/usbhid/hid-core.c
--- hid.orig/drivers/hid/usbhid/hid-core.c	2008-02-29 19:35:24.000000000 +0200
+++ hid/drivers/hid/usbhid/hid-core.c	2008-03-01 14:18:13.000000000 +0200
@@ -800,6 +800,22 @@ static struct hid_device *usb_hid_config
 		goto fail;
 	}
 
+	hid->name[0] = 0;
+
+	if (dev->manufacturer)
+		strlcpy(hid->name, dev->manufacturer, sizeof(hid->name));
+
+	if (dev->product) {
+		if (dev->manufacturer)
+			strlcat(hid->name, " ", sizeof(hid->name));
+		strlcat(hid->name, dev->product, sizeof(hid->name));
+	}
+
+	if (!strlen(hid->name))
+		snprintf(hid->name, sizeof(hid->name), "HID %04x:%04x",
+			 le16_to_cpu(dev->descriptor.idVendor),
+			 le16_to_cpu(dev->descriptor.idProduct));
+
 	for (n = 0; n < interface->desc.bNumEndpoints; n++) {
 
 		struct usb_endpoint_descriptor *endpoint;
@@ -812,6 +828,14 @@ static struct hid_device *usb_hid_config
 
 		interval = endpoint->bInterval;
 
+		/* Some vendors give fullspeed interval on highspeed devides */
+		if (quirks & HID_QUIRK_FULLSPEED_INTERVAL  &&
+		    dev->speed == USB_SPEED_HIGH) {
+			interval = fls(endpoint->bInterval*8);
+			printk(KERN_INFO "%s: Fixing fullspeed to highspeed interval: %d -> %d\n",
+			       hid->name, endpoint->bInterval, interval);
+		}
+
 		/* Change the polling interval of mice. */
 		if (hid->collection->usage == HID_GD_MOUSE && hid_mousepoll_interval > 0)
 			interval = hid_mousepoll_interval;
@@ -859,22 +883,6 @@ static struct hid_device *usb_hid_config
 	usbhid->intf = intf;
 	usbhid->ifnum = interface->desc.bInterfaceNumber;
 
-	hid->name[0] = 0;
-
-	if (dev->manufacturer)
-		strlcpy(hid->name, dev->manufacturer, sizeof(hid->name));
-
-	if (dev->product) {
-		if (dev->manufacturer)
-			strlcat(hid->name, " ", sizeof(hid->name));
-		strlcat(hid->name, dev->product, sizeof(hid->name));
-	}
-
-	if (!strlen(hid->name))
-		snprintf(hid->name, sizeof(hid->name), "HID %04x:%04x",
-			 le16_to_cpu(dev->descriptor.idVendor),
-			 le16_to_cpu(dev->descriptor.idProduct));
-
 	hid->bus = BUS_USB;
 	hid->vendor = le16_to_cpu(dev->descriptor.idVendor);
 	hid->product = le16_to_cpu(dev->descriptor.idProduct);
diff -uprN -X hid.orig/Documentation/dontdiff hid.orig/drivers/hid/usbhid/hid-quirks.c hid/drivers/hid/usbhid/hid-quirks.c
--- hid.orig/drivers/hid/usbhid/hid-quirks.c	2008-02-29 19:35:24.000000000 +0200
+++ hid/drivers/hid/usbhid/hid-quirks.c	2008-02-29 19:49:28.000000000 +0200
@@ -32,6 +32,9 @@
 #define USB_VENDOR_ID_ADS_TECH 		0x06e1
 #define USB_DEVICE_ID_ADS_TECH_RADIO_SI470X	0xa155
 
+#define USB_VENDOR_ID_AFATECH		0x15a4
+#define USB_DEVICE_ID_AFATECH_AF9016	0x9016
+
 #define USB_VENDOR_ID_AIPTEK		0x08ca
 #define USB_DEVICE_ID_AIPTEK_01		0x0001
 #define USB_DEVICE_ID_AIPTEK_10		0x0010
@@ -414,6 +417,8 @@ static const struct hid_blacklist {
 	
 	{ USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_EDGE, HID_QUIRK_DUPLICATE_USAGES },
 
+	{ USB_VENDOR_ID_AFATECH, USB_DEVICE_ID_AFATECH_AF9016, HID_QUIRK_FULLSPEED_INTERVAL },
+
 	{ USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM, HID_QUIRK_HIDDEV },
 	{ USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4, HID_QUIRK_HIDDEV | HID_QUIRK_IGNORE_HIDINPUT },
 	{ USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV, HID_QUIRK_HIDINPUT },
diff -uprN -X hid.orig/Documentation/dontdiff hid.orig/include/linux/hid.h hid/include/linux/hid.h
--- hid.orig/include/linux/hid.h	2008-02-29 19:35:29.000000000 +0200
+++ hid/include/linux/hid.h	2008-02-29 20:07:15.000000000 +0200
@@ -284,6 +284,7 @@ struct hid_item {
 #define HID_QUIRK_2WHEEL_MOUSE_HACK_B8		0x02000000
 #define HID_QUIRK_HWHEEL_WHEEL_INVERT		0x04000000
 #define HID_QUIRK_MICROSOFT_KEYS		0x08000000
+#define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 
 /*
  * Separate quirks for runtime report descriptor fixup


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [patch] hid: quirk to fixup fullspeed interval on highspeed devices
  2008-03-01 14:53 [patch] hid: quirk to fixup fullspeed interval on highspeed devices Pekka Sarnila
@ 2008-03-03 12:50 ` Jiri Kosina
  2008-03-03 14:38   ` Pekka Sarnila
  0 siblings, 1 reply; 4+ messages in thread
From: Jiri Kosina @ 2008-03-03 12:50 UTC (permalink / raw)
  To: Pekka Sarnila; +Cc: linux-kernel

On Sat, 1 Mar 2008, Pekka Sarnila wrote:

> Many vendors highspeed devices give erroneously fullspeed interval value 
> in endpoint descriptor for interrupt endpoints. This quirk fixes up that 
> by recalculating the right value for highspeed device.

Hi Pekka,

thanks for the patch. I however think that HID code is really a wrong 
place to to this, we really want to do this in USB core instead, as that 
it where it belongs.

Why do you think that adding a new quirk for this device to 
usb/core/quirks.c and then checking for it in usb_parse_endpoint() is not 
feasible?

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [patch] hid: quirk to fixup fullspeed interval on highspeed devices
  2008-03-03 12:50 ` Jiri Kosina
@ 2008-03-03 14:38   ` Pekka Sarnila
  2008-03-06 12:08     ` Jiri Kosina
  0 siblings, 1 reply; 4+ messages in thread
From: Pekka Sarnila @ 2008-03-03 14:38 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: linux-kernel

Hi,

thanks for the answer.

Jiri Kosina wrote:
> On Sat, 1 Mar 2008, Pekka Sarnila wrote:
> 
>> Many vendors highspeed devices give erroneously fullspeed interval value 
>> in endpoint descriptor for interrupt endpoints. This quirk fixes up that 
>> by recalculating the right value for highspeed device.
> 
> Hi Pekka,
> 
> thanks for the patch. I however think that HID code is really a wrong 
> place to to this, we really want to do this in USB core instead, as that 
> it where it belongs.

Yes, I kind of agree, but ...

> Why do you think that adding a new quirk for this device to 
> usb/core/quirks.c and then checking for it in usb_parse_endpoint() is not 
> feasible?

The quirks for usb/core (only two so far) are listed in
include/linux/usb/quirks.h. As it is states there:

/*
 * This file holds the definitions of quirks found in USB devices.
 * Only quirks that affect the whole device, not an interface,
 * belong here.
 */

There are two interfaces on this particular device. Only one interface is
HID, and the problem pertains only to that interface. Thus a class specific
quirk (HID) to eliminate the kernel tinkering with the endpoints of the
other interface (not having this problem).

This rises the question of the generality of this quirk. In this particular
case there are no interrupt endpoints on the other device. So based on this
one case it is impossible say anything on whether on devices having several
interrupt endpoints, maybe on several interfaces, the problem is device,
interface or endpoint specific. I suspect all combinations exist.
One might speculate that manufacturers are combining in to one usb.2 device
old usb.1 and new usb.2 components forgetting to update the interval value
in the firmware for the old component (I suspects this has happened in this
case: new being dvb, old ifr kbd).

If so it implies the need, not only for device or class specific, but also
interface and endpoint specific quirks. No such quirk-mechanism (to my
knowledge) exist.

Anyway, in the case of this particular device it was justified to limit
the quirk to HID-class. What to do in general is the question: My guess is
(without interface or endpoint specific quirks) class specific quirks hit
the target more precisely. But it is really only a guess.

Pekka

 
> Thanks,
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [patch] hid: quirk to fixup fullspeed interval on highspeed devices
  2008-03-03 14:38   ` Pekka Sarnila
@ 2008-03-06 12:08     ` Jiri Kosina
  0 siblings, 0 replies; 4+ messages in thread
From: Jiri Kosina @ 2008-03-06 12:08 UTC (permalink / raw)
  To: Pekka Sarnila; +Cc: linux-kernel

On Mon, 3 Mar 2008, Pekka Sarnila wrote:

> Anyway, in the case of this particular device it was justified to limit 
> the quirk to HID-class. What to do in general is the question: My guess 
> is (without interface or endpoint specific quirks) class specific quirks 
> hit the target more precisely. But it is really only a guess.

OK, I agree, thanks. I will apply your patch to my tree.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2008-03-06 12:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-01 14:53 [patch] hid: quirk to fixup fullspeed interval on highspeed devices Pekka Sarnila
2008-03-03 12:50 ` Jiri Kosina
2008-03-03 14:38   ` Pekka Sarnila
2008-03-06 12:08     ` Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).