HID: wacom: Improve generic name generation
Subsystems (2)
PATCH v2 1/1 HID: wacom: Improve generic name generation
The 'wacom_update_name' function is responsible for producing names for the input device nodes based on the hardware device name. Commit f2209d4 added the ability to strip off prefixes like "Wacom Co.,Ltd." where the prefix was immediately (and redundantly) followed by "Wacom". The 2nd-generation Intuos Pro 2 has such a prefix, but with a small error (the period and comma are swapped) that prevents the existing code from matching it. We're loath to extend the number of cases out endlessly and so instead try to be smarter about name generation.
We observe that the cause of the redundant prefixes is HID combining the manufacturer and product strings of USB devices together. By using the original product name (with "Wacom" prefixed, if it does not already exist in the string) we can bypass the gyrations to find and remove redundant prefixes. Other devices either don't have a manufacturer string that needs to be removed (Bluetooth, uhid) or should have their name generated from scratch (I2C).
--- drivers/hid/wacom_sys.c | 90 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 30 deletions(-)
drivers/hid/wacom_sys.c +60 -30
@@ -14,6 +14,7 @@#include "wacom_wac.h" #include "wacom.h" #include <linux/input/mt.h> +#include <linux/usb.h> #define WAC_MSG_RETRIES 5#define WAC_CMD_RETRIES 10
@@ -2020,6 +2021,29 @@ static size_t wacom_compute_pktlen(struct hid_device *hdev)return size; } + +static int __wacom_is_usb_parent (struct usb_device *usbdev,void *ptr) +{ +struct wacom *wacom = ptr; +struct device *parent = wacom->hdev->dev.parent; +struct usb_host_config *config = usbdev->actconfig; +int i; + +for (i =0 ; i < config->desc.bNumInterfaces; i++) { +if (&config->interface[i]->dev == parent) +return 1 ; + } +return 0 ; +} + +/* +* Distinguish between real USB devices and uhid-created doppelgangers +*/ +static bool wacom_is_real_usb (struct wacom *wacom) { +return wacom->hdev->bus ==BUS_USB && + usb_for_each_dev(wacom, __wacom_is_usb_parent); +} +static void wacom_update_name (struct wacom *wacom,const char *suffix) {struct wacom_wac *wacom_wac = &wacom->wacom_wac;
@@ -2028,41 +2052,47 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)/* Generic devices name unspecified */ if ((features->type ==HID_GENERIC ) && !strcmp ("Wacom HID" , features->name)) { -if (strstr (wacom->hdev->name,"Wacom" ) || -strstr (wacom->hdev->name,"wacom" ) || -strstr (wacom->hdev->name,"WACOM" )) { -/* name is in HID descriptor, use it */ -strlcpy (name, wacom->hdev->name,sizeof (name)); - -/* strip out excess whitespaces */ -while (1 ) { -char *gap =strstr (name," " ); -if (gap ==NULL ) -break ; -/* shift everything including the terminator */ -memmove (gap, gap+1 ,strlen (gap)); - } +char *product_name =NULL ; -/* strip off excessive prefixing */ -if (strstr (name,"Wacom Co.,Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co.,Ltd. " ); -memmove (name, name+x, n-x+1 ); +if (wacom->hdev->bus ==BUS_USB ) { +if (wacom_is_real_usb (wacom)) { +struct usb_interface *intf =to_usb_interface (wacom->hdev->dev.parent); +struct usb_device *dev =interface_to_usbdev (intf); + product_name = dev->product; } -if (strstr (name,"Wacom Co., Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co., Ltd. " ); -memmove (name, name+x, n-x+1 ); +else { + product_name = wacom->hdev->name; } + } +else if (wacom->hdev->bus ==BUS_BLUETOOTH ) { + product_name = wacom->hdev->name; + } -/* get rid of trailing whitespace */ -if (name[strlen (name)-1 ] ==' ' ) - name[strlen (name)-1 ] ='\0' ; - }else { -/* no meaningful name retrieved. use product ID */ -snprintf (name,sizeof (name), -"%s %X" , features->name, wacom->hdev->product); +if (!product_name) { +snprintf (name,sizeof (name),"%s %X" , + features->name, wacom->hdev->product); + } +else if (strstr (product_name,"Wacom" ) || +strstr (product_name,"wacom" ) || +strstr (product_name,"WACOM" )) { +strlcpy (name, product_name,sizeof (name)); } +else { +snprintf (name,sizeof (name),"Wacom %s" , product_name); + } + +/* strip out excess whitespaces */ +while (1 ) { +char *gap =strstr (name," " ); +if (gap ==NULL ) +break ; +/* shift everything including the terminator */ +memmove (gap, gap+1 ,strlen (gap)); + } + +/* get rid of trailing whitespace */ +if (name[strlen (name)-1 ] ==' ' ) + name[strlen (name)-1 ] ='\0' ; }else {strlcpy (name, features->name,sizeof (name)); }
Signature · 1 line
2.12.2
Quoted 17 lines · Jason Gerecke
The '
wacom_update_name' function is responsible for producing names for the input device nodes based on the hardware device name. Commit f2209d4 added the ability to strip off prefixes like "Wacom Co.,Ltd." where the prefix was immediately (and redundantly) followed by "Wacom". The 2nd-generation Intuos Pro 2 has such a prefix, but with a small error (the period and comma are swapped) that prevents the existing code from matching it. We're loath to extend the number of cases out endlessly and so instead try to be smarter about name generation.We observe that the cause of the redundant prefixes is HID combining the manufacturer and product strings of USB devices together. By using the original product name (with "Wacom" prefixed, if it does not already exist in the string) we can bypass the gyrations to find and remove redundant prefixes. Other devices either don't have a manufacturer string that needs to be removed (Bluetooth, uhid) or should have their name generated from scratch (I2C).
Quoted 2 lines
Signed-off-by Jason Gerecke <[email protected]>---
Thanks Jason.
I am not a big fan of the "real USB" check (I would believe enhancing struct hid_device would make something more generic), but it seems to be doing the job OK.
Just giving out 2 ideas to achieve the same, so you could decide keeping this or not:
- adding .product_name to struct hid_device directly
- exporting usb_hid_driver in usbhid/usbhid.h and comparing hdev->ll_driver with it (this can be achieved in an inlined function in usbhid.h).
I would believe having the second option above would provide several benefits:
- the wacom module will not have to check against all usb devices
- other drivers would be able to quickly tell if they are using a low level USB device, and not a uhid one. This way, we can remove some errors when we emulate a HID device on a HID driver that expects USB to be the actual transport layer
Your function works too, but I would believe this will be faster and better (I also get the concern of having something self consistent for easier backporting).
Besides that, small nitpicks:
Quoted 62 lines
drivers/hid/wacom_sys.c | 90 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 30 deletions(-)drivers/hid/wacom_sys.c +24
-13@@ -14,6 +14,7 @@#include "wacom_wac.h" #include "wacom.h" #include <linux/input/mt.h> +#include <linux/usb.h> #define WAC_MSG_RETRIES 5#define WAC_CMD_RETRIES 10@@ -2020,6 +2021,29 @@ static size_t wacom_compute_pktlen(struct hid_device *hdev)return size; } + +static int __wacom_is_usb_parent (struct usb_device *usbdev,void *ptr) +{ +struct wacom *wacom = ptr; +struct device *parent = wacom->hdev->dev.parent; +struct usb_host_config *config = usbdev->actconfig; +int i; + +for (i =0 ; i < config->desc.bNumInterfaces; i++) { +if (&config->interface[i]->dev == parent) +return 1 ; + } +return 0 ; +} + +/* +* Distinguish between real USB devices and uhid-created doppelgangers +*/ +static bool wacom_is_real_usb (struct wacom *wacom) { +return wacom->hdev->bus ==BUS_USB && + usb_for_each_dev(wacom, __wacom_is_usb_parent); +} +static void wacom_update_name (struct wacom *wacom,const char *suffix) {struct wacom_wac *wacom_wac = &wacom->wacom_wac;@@ -2028,41 +2052,47 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)/* Generic devices name unspecified */ if ((features->type ==HID_GENERIC ) && !strcmp ("Wacom HID" , features->name)) { -if (strstr (wacom->hdev->name,"Wacom" ) || -strstr (wacom->hdev->name,"wacom" ) || -strstr (wacom->hdev->name,"WACOM" )) { -/* name is in HID descriptor, use it */ -strlcpy (name, wacom->hdev->name,sizeof (name)); - -/* strip out excess whitespaces */ -while (1 ) { -char *gap =strstr (name," " ); -if (gap ==NULL ) -break ; -/* shift everything including the terminator */ -memmove (gap, gap+1 ,strlen (gap));
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2058
Quoted 10 lines
drivers/hid/wacom_sys.c +3
-6- } +char *product_name =NULL ; -/* strip off excessive prefixing */ -if (strstr (name,"Wacom Co.,Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co.,Ltd. " ); -memmove (name, name+x, n-x+1 ); +if (wacom->hdev->bus ==BUS_USB ) { +if (wacom_is_real_usb (wacom)) {
wacom_is_real_usb() already checks for BUS_USB, so the first test is redundant.
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2063
Quoted 9 lines
drivers/hid/wacom_sys.c +4
-4+struct usb_interface *intf =to_usb_interface (wacom->hdev->dev.parent); +struct usb_device *dev =interface_to_usbdev (intf); + product_name = dev->product; } -if (strstr (name,"Wacom Co., Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co., Ltd. " ); -memmove (name, name+x, n-x+1 ); +else {
this else shoule be on the previous line
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2067
Quoted 4 lines
drivers/hid/wacom_sys.c +3
-0+ product_name = wacom->hdev->name; } + } +else if (wacom->hdev->bus ==BUS_BLUETOOTH ) {
likewise
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2069
Quoted 2 lines
drivers/hid/wacom_sys.c +2
-0+ product_name = wacom->hdev->name; + }
How about?:
if (wacom_is_real_usb(wacom)) {
struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent);
struct usb_device *dev = interface_to_usbdev(intf);
product_name = dev->product;
} else if (wacom->hdev->bus == BUS_BLUETOOTH ||
wacom->hdev->bus == BUS_USB) {
product_name = wacom->hdev->name;
}drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2075
Quoted 13 lines
drivers/hid/wacom_sys.c +5
-7-/* get rid of trailing whitespace */ -if (name[strlen (name)-1 ] ==' ' ) - name[strlen (name)-1 ] ='\0' ; - }else { -/* no meaningful name retrieved. use product ID */ -snprintf (name,sizeof (name), -"%s %X" , features->name, wacom->hdev->product); +if (!product_name) { +snprintf (name,sizeof (name),"%s %X" , + features->name, wacom->hdev->product); + } + else if (strstr(product_name, "Wacom") ||
else should be on the previous line
Quoted 26 lines
drivers/hid/wacom_sys.c +19
-0+ strstr(product_name, "wacom") || + strstr(product_name, "WACOM")) { + strlcpy(name, product_name, sizeof(name)); } + else { +snprintf (name,sizeof (name),"Wacom %s" , product_name); + } + +/* strip out excess whitespaces */ +while (1 ) { +char *gap =strstr (name," " ); +if (gap ==NULL ) +break ; +/* shift everything including the terminator */ +memmove (gap, gap+1 ,strlen (gap)); + } + +/* get rid of trailing whitespace */ +if (name[strlen (name)-1 ] ==' ' ) + name[strlen (name)-1 ] ='\0' ; }else {strlcpy (name, features->name,sizeof (name)); }Signature · 1 line
2.12.2
Cheers,
Benjamin
Quoted 42 lines · Benjamin Tissoires
Quoted 17 lines · Jason Gerecke
The '
wacom_update_name' function is responsible for producing names for the input device nodes based on the hardware device name. Commit f2209d4 added the ability to strip off prefixes like "Wacom Co.,Ltd." where the prefix was immediately (and redundantly) followed by "Wacom". The 2nd-generation Intuos Pro 2 has such a prefix, but with a small error (the period and comma are swapped) that prevents the existing code from matching it. We're loath to extend the number of cases out endlessly and so instead try to be smarter about name generation.We observe that the cause of the redundant prefixes is HID combining the manufacturer and product strings of USB devices together. By using the original product name (with "Wacom" prefixed, if it does not already exist in the string) we can bypass the gyrations to find and remove redundant prefixes. Other devices either don't have a manufacturer string that needs to be removed (Bluetooth, uhid) or should have their name generated from scratch (I2C).
Quoted 2 lines
Signed-off-by Jason Gerecke <[email protected]>---
Thanks Jason.
I am not a big fan of the "real USB" check (I would believe enhancing struct
hid_devicewould make something more generic), but it seems to be doing the job OK.Just giving out 2 ideas to achieve the same, so you could decide keeping this or not:
- adding .product_name to struct
hid_devicedirectly
- exportingusb_hid_driverinusbhid/usbhid.hand comparinghdev->ll_driverwith it (this can be achieved in an inlined function inusbhid.h).I would believe having the second option above would provide several benefits:
- the wacom module will not have to check against all usb devices
- other drivers would be able to quickly tell if they are using a low level USB device, and not a uhid one. This way, we can remove some errors when we emulate a HID device on a HID driver that expects USB to be the actual transport layer
Quoted 4 lines
Your function works too, but I would believe this will be faster and better (I also get the concern of having something self consistent for easier backporting).
Ah, I think I misunderstood your earlier reply. I avoided a generic approach since it seemed this naming issue was unique to the USB bus and our driver. Backporting concerns also entered the picture, though we can always conditionally compile this "real USB" function as fallback code like we do in other cases where we have to work with older kernels.
I would be tempted to take the second option as well (I was quite disappointed when I realized such a check wasn't already possible), but using definitions from usbhid.h or other non-system headers isn't ideal. We'd have to require users to have a full source tree instead of just a "kernel-devel" package in order to build an input-wacom backport (or have the "real USB" function used even when compiled with upstream kernels :().
A modification of the second option that might be a bit better is to have hid.h expose a function that does the comparison on behalf of the caller. I presume that someone else could someday want to know if their e.g. I2C device is really from uhid, so I'd probably also change the logic to be "return hid->ll_driver == uhid_hid_driver". That way we don't have to export and compare against every transport driver...
A different option I'd considered and am rather fond of would be to define something like "HID_QUIRK_UHID" (or "HID_QUIRK_VIRTUAL"?) which the uhid driver could set on devices that it creates. It'd be trivial to implement and check and seems like it'd fit right in with the other device quirks. Thoughts?
Nitpicks noted for an eventual V3 :)
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
Quoted 156 lines
Besides that, small nitpicks:
Quoted 62 lines
drivers/hid/wacom_sys.c | 90 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 30 deletions(-)drivers/hid/wacom_sys.c +24
-13@@ -14,6 +14,7 @@#include "wacom_wac.h" #include "wacom.h" #include <linux/input/mt.h> +#include <linux/usb.h> #define WAC_MSG_RETRIES 5#define WAC_CMD_RETRIES 10@@ -2020,6 +2021,29 @@ static size_t wacom_compute_pktlen(struct hid_device *hdev)return size; } + +static int __wacom_is_usb_parent (struct usb_device *usbdev,void *ptr) +{ +struct wacom *wacom = ptr; +struct device *parent = wacom->hdev->dev.parent; +struct usb_host_config *config = usbdev->actconfig; +int i; + +for (i =0 ; i < config->desc.bNumInterfaces; i++) { +if (&config->interface[i]->dev == parent) +return 1 ; + } +return 0 ; +} + +/* +* Distinguish between real USB devices and uhid-created doppelgangers +*/ +static bool wacom_is_real_usb (struct wacom *wacom) { +return wacom->hdev->bus ==BUS_USB && + usb_for_each_dev(wacom, __wacom_is_usb_parent); +} +static void wacom_update_name (struct wacom *wacom,const char *suffix) {struct wacom_wac *wacom_wac = &wacom->wacom_wac;@@ -2028,41 +2052,47 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)/* Generic devices name unspecified */ if ((features->type ==HID_GENERIC ) && !strcmp ("Wacom HID" , features->name)) { -if (strstr (wacom->hdev->name,"Wacom" ) || -strstr (wacom->hdev->name,"wacom" ) || -strstr (wacom->hdev->name,"WACOM" )) { -/* name is in HID descriptor, use it */ -strlcpy (name, wacom->hdev->name,sizeof (name)); - -/* strip out excess whitespaces */ -while (1 ) { -char *gap =strstr (name," " ); -if (gap ==NULL ) -break ; -/* shift everything including the terminator */ -memmove (gap, gap+1 ,strlen (gap));drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2058
Quoted 10 lines
drivers/hid/wacom_sys.c +3
-6- } +char *product_name =NULL ; -/* strip off excessive prefixing */ -if (strstr (name,"Wacom Co.,Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co.,Ltd. " ); -memmove (name, name+x, n-x+1 ); +if (wacom->hdev->bus ==BUS_USB ) { +if (wacom_is_real_usb (wacom)) {
wacom_is_real_usb()already checks forBUS_USB, so the first test is redundant.drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2063
Quoted 9 lines
drivers/hid/wacom_sys.c +4
-4+struct usb_interface *intf =to_usb_interface (wacom->hdev->dev.parent); +struct usb_device *dev =interface_to_usbdev (intf); + product_name = dev->product; } -if (strstr (name,"Wacom Co., Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co., Ltd. " ); -memmove (name, name+x, n-x+1 ); +else {this else shoule be on the previous line
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2067
Quoted 4 lines
drivers/hid/wacom_sys.c +3
-0+ product_name = wacom->hdev->name; } + } +else if (wacom->hdev->bus ==BUS_BLUETOOTH ) {likewise
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2069
Quoted 2 lines
drivers/hid/wacom_sys.c +2
-0+ product_name = wacom->hdev->name; + }How about?:
if (wacom_is_real_usb(wacom)) { struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); struct usb_device *dev = interface_to_usbdev(intf); product_name = dev->product; } else if (wacom->hdev->bus == BUS_BLUETOOTH || wacom->hdev->bus == BUS_USB) { product_name = wacom->hdev->name; }drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2075
Quoted 13 lines
drivers/hid/wacom_sys.c +5
-7-/* get rid of trailing whitespace */ -if (name[strlen (name)-1 ] ==' ' ) - name[strlen (name)-1 ] ='\0' ; - }else { -/* no meaningful name retrieved. use product ID */ -snprintf (name,sizeof (name), -"%s %X" , features->name, wacom->hdev->product); +if (!product_name) { +snprintf (name,sizeof (name),"%s %X" , + features->name, wacom->hdev->product); + } + else if (strstr(product_name, "Wacom") ||else should be on the previous line
Quoted 26 lines
drivers/hid/wacom_sys.c +19
-0+ strstr(product_name, "wacom") || + strstr(product_name, "WACOM")) { + strlcpy(name, product_name, sizeof(name)); } + else { +snprintf (name,sizeof (name),"Wacom %s" , product_name); + } + +/* strip out excess whitespaces */ +while (1 ) { +char *gap =strstr (name," " ); +if (gap ==NULL ) +break ; +/* shift everything including the terminator */ +memmove (gap, gap+1 ,strlen (gap)); + } + +/* get rid of trailing whitespace */ +if (name[strlen (name)-1 ] ==' ' ) + name[strlen (name)-1 ] ='\0' ; }else {strlcpy (name, features->name,sizeof (name)); }Signature · 1 line
2.12.2Cheers,
Benjamin
Quoted 54 lines · Jason Gerecke
Quoted 42 lines · Benjamin Tissoires
Quoted 17 lines · Jason Gerecke
The '
wacom_update_name' function is responsible for producing names for the input device nodes based on the hardware device name. Commit f2209d4 added the ability to strip off prefixes like "Wacom Co.,Ltd." where the prefix was immediately (and redundantly) followed by "Wacom". The 2nd-generation Intuos Pro 2 has such a prefix, but with a small error (the period and comma are swapped) that prevents the existing code from matching it. We're loath to extend the number of cases out endlessly and so instead try to be smarter about name generation.We observe that the cause of the redundant prefixes is HID combining the manufacturer and product strings of USB devices together. By using the original product name (with "Wacom" prefixed, if it does not already exist in the string) we can bypass the gyrations to find and remove redundant prefixes. Other devices either don't have a manufacturer string that needs to be removed (Bluetooth, uhid) or should have their name generated from scratch (I2C).
Quoted 2 lines
Signed-off-by Jason Gerecke <[email protected]>---
Thanks Jason.
I am not a big fan of the "real USB" check (I would believe enhancing struct
hid_devicewould make something more generic), but it seems to be doing the job OK.Just giving out 2 ideas to achieve the same, so you could decide keeping this or not:
- adding .product_name to struct
hid_devicedirectly
- exportingusb_hid_driverinusbhid/usbhid.hand comparinghdev->ll_driverwith it (this can be achieved in an inlined function inusbhid.h).I would believe having the second option above would provide several benefits:
- the wacom module will not have to check against all usb devices
- other drivers would be able to quickly tell if they are using a low level USB device, and not a uhid one. This way, we can remove some errors when we emulate a HID device on a HID driver that expects USB to be the actual transport layerQuoted 4 lines
Your function works too, but I would believe this will be faster and better (I also get the concern of having something self consistent for easier backporting).
Ah, I think I misunderstood your earlier reply. I avoided a generic approach since it seemed this naming issue was unique to the USB bus and our driver. Backporting concerns also entered the picture, though we can always conditionally compile this "real USB" function as fallback code like we do in other cases where we have to work with older kernels.
Quoted 7 lines
I would be tempted to take the second option as well (I was quite disappointed when I realized such a check wasn't already possible), but using definitions from
usbhid.hor other non-system headers isn't ideal. We'd have to require users to have a full source tree instead of just a "kernel-devel" package in order to build an input-wacom backport (or have the "real USB" function used even when compiled with upstream kernels :().
You can also define locally the function with this implementation if you do not want to change usbhid.h
Quoted 7 lines
A modification of the second option that might be a bit better is to have
hid.hexpose a function that does the comparison on behalf of the caller. I presume that someone else could someday want to know if their e.g. I2C device is really from uhid, so I'd probably also change the logic to be "returnhid->ll_driver==uhid_hid_driver". That way we don't have to export and compare against every transport driver...
I'd rather ask: "are you of such type?" than "are you not on such type?".
The problem with the negation is that as soon as you want to target only Bluetooth for instance, then you'll need to ask for "bluetooth and not uhid".
OK, that is not clear. An other way of saying it is that you are interesting in casting a pointer into a pointer type from the low level transport driver. So if you are not absolutely sure about it, then you should not cast it.
Quoted 6 lines
A different option I'd considered and am rather fond of would be to define something like "
HID_QUIRK_UHID" (or "HID_QUIRK_VIRTUAL"?) which the uhid driver could set on devices that it creates. It'd be trivial to implement and check and seems like it'd fit right in with the other device quirks. Thoughts?
I locally have a patch that achieves that by changing the hdev->type. But I am not so sure it is good because we would change HID_TYPE_OTHER, HID_TYPE_USBMOUSE, or HID_TYPE_USBNONE into HID_TYPE_UHID... Also I do not like much the fact that the transport layer has to change something in hdev if only one does such a change.
So I would believe it's better to ask usbhid if it handles the device currently than asking uhid if it is not handling it.
Cheers,
Benjamin
Quoted 167 lines
Nitpicks noted for an eventual V3 :)
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....Quoted 156 lines
Besides that, small nitpicks:
Quoted 62 lines
drivers/hid/wacom_sys.c | 90 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 30 deletions(-)drivers/hid/wacom_sys.c +24
-13@@ -14,6 +14,7 @@#include "wacom_wac.h" #include "wacom.h" #include <linux/input/mt.h> +#include <linux/usb.h> #define WAC_MSG_RETRIES 5#define WAC_CMD_RETRIES 10@@ -2020,6 +2021,29 @@ static size_t wacom_compute_pktlen(struct hid_device *hdev)return size; } + +static int __wacom_is_usb_parent (struct usb_device *usbdev,void *ptr) +{ +struct wacom *wacom = ptr; +struct device *parent = wacom->hdev->dev.parent; +struct usb_host_config *config = usbdev->actconfig; +int i; + +for (i =0 ; i < config->desc.bNumInterfaces; i++) { +if (&config->interface[i]->dev == parent) +return 1 ; + } +return 0 ; +} + +/* +* Distinguish between real USB devices and uhid-created doppelgangers +*/ +static bool wacom_is_real_usb (struct wacom *wacom) { +return wacom->hdev->bus ==BUS_USB && + usb_for_each_dev(wacom, __wacom_is_usb_parent); +} +static void wacom_update_name (struct wacom *wacom,const char *suffix) {struct wacom_wac *wacom_wac = &wacom->wacom_wac;@@ -2028,41 +2052,47 @@ static void wacom_update_name(struct wacom *wacom, const char *suffix)/* Generic devices name unspecified */ if ((features->type ==HID_GENERIC ) && !strcmp ("Wacom HID" , features->name)) { -if (strstr (wacom->hdev->name,"Wacom" ) || -strstr (wacom->hdev->name,"wacom" ) || -strstr (wacom->hdev->name,"WACOM" )) { -/* name is in HID descriptor, use it */ -strlcpy (name, wacom->hdev->name,sizeof (name)); - -/* strip out excess whitespaces */ -while (1 ) { -char *gap =strstr (name," " ); -if (gap ==NULL ) -break ; -/* shift everything including the terminator */ -memmove (gap, gap+1 ,strlen (gap));drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2058
Quoted 10 lines
drivers/hid/wacom_sys.c +3
-6- } +char *product_name =NULL ; -/* strip off excessive prefixing */ -if (strstr (name,"Wacom Co.,Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co.,Ltd. " ); -memmove (name, name+x, n-x+1 ); +if (wacom->hdev->bus ==BUS_USB ) { +if (wacom_is_real_usb (wacom)) {
wacom_is_real_usb()already checks forBUS_USB, so the first test is redundant.drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2063
Quoted 9 lines
drivers/hid/wacom_sys.c +4
-4+struct usb_interface *intf =to_usb_interface (wacom->hdev->dev.parent); +struct usb_device *dev =interface_to_usbdev (intf); + product_name = dev->product; } -if (strstr (name,"Wacom Co., Ltd. Wacom " ) == name) { -int n =strlen (name); -int x =strlen ("Wacom Co., Ltd. " ); -memmove (name, name+x, n-x+1 ); +else {this else shoule be on the previous line
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2067
Quoted 4 lines
drivers/hid/wacom_sys.c +3
-0+ product_name = wacom->hdev->name; } + } +else if (wacom->hdev->bus ==BUS_BLUETOOTH ) {likewise
drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2069
Quoted 2 lines
drivers/hid/wacom_sys.c +2
-0+ product_name = wacom->hdev->name; + }How about?:
if (wacom_is_real_usb(wacom)) { struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent); struct usb_device *dev = interface_to_usbdev(intf); product_name = dev->product; } else if (wacom->hdev->bus == BUS_BLUETOOTH || wacom->hdev->bus == BUS_USB) { product_name = wacom->hdev->name; }drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix) · line 2075
Quoted 13 lines
drivers/hid/wacom_sys.c +5
-7-/* get rid of trailing whitespace */ -if (name[strlen (name)-1 ] ==' ' ) - name[strlen (name)-1 ] ='\0' ; - }else { -/* no meaningful name retrieved. use product ID */ -snprintf (name,sizeof (name), -"%s %X" , features->name, wacom->hdev->product); +if (!product_name) { +snprintf (name,sizeof (name),"%s %X" , + features->name, wacom->hdev->product); + } + else if (strstr(product_name, "Wacom") ||else should be on the previous line
Quoted 26 lines
drivers/hid/wacom_sys.c +19
-0+ strstr(product_name, "wacom") || + strstr(product_name, "WACOM")) { + strlcpy(name, product_name, sizeof(name)); } + else { +snprintf (name,sizeof (name),"Wacom %s" , product_name); + } + +/* strip out excess whitespaces */ +while (1 ) { +char *gap =strstr (name," " ); +if (gap ==NULL ) +break ; +/* shift everything including the terminator */ +memmove (gap, gap+1 ,strlen (gap)); + } + +/* get rid of trailing whitespace */ +if (name[strlen (name)-1 ] ==' ' ) + name[strlen (name)-1 ] ='\0' ; }else {strlcpy (name, features->name,sizeof (name)); }Signature · 1 line
2.12.2Cheers,
Benjamin
Quoted 99 lines · Benjamin Tissoires
Quoted 54 lines · Jason Gerecke
Quoted 42 lines · Benjamin Tissoires
Quoted 17 lines · Jason Gerecke
The '
wacom_update_name' function is responsible for producing names for the input device nodes based on the hardware device name. Commit f2209d4 added the ability to strip off prefixes like "Wacom Co.,Ltd." where the prefix was immediately (and redundantly) followed by "Wacom". The 2nd-generation Intuos Pro 2 has such a prefix, but with a small error (the period and comma are swapped) that prevents the existing code from matching it. We're loath to extend the number of cases out endlessly and so instead try to be smarter about name generation.We observe that the cause of the redundant prefixes is HID combining the manufacturer and product strings of USB devices together. By using the original product name (with "Wacom" prefixed, if it does not already exist in the string) we can bypass the gyrations to find and remove redundant prefixes. Other devices either don't have a manufacturer string that needs to be removed (Bluetooth, uhid) or should have their name generated from scratch (I2C).
Quoted 2 lines
Signed-off-by Jason Gerecke <[email protected]>---
Thanks Jason.
I am not a big fan of the "real USB" check (I would believe enhancing struct
hid_devicewould make something more generic), but it seems to be doing the job OK.Just giving out 2 ideas to achieve the same, so you could decide keeping this or not:
- adding .product_name to struct
hid_devicedirectly
- exportingusb_hid_driverinusbhid/usbhid.hand comparinghdev->ll_driverwith it (this can be achieved in an inlined function inusbhid.h).I would believe having the second option above would provide several benefits:
- the wacom module will not have to check against all usb devices
- other drivers would be able to quickly tell if they are using a low level USB device, and not a uhid one. This way, we can remove some errors when we emulate a HID device on a HID driver that expects USB to be the actual transport layerQuoted 4 lines
Your function works too, but I would believe this will be faster and better (I also get the concern of having something self consistent for easier backporting).
Ah, I think I misunderstood your earlier reply. I avoided a generic approach since it seemed this naming issue was unique to the USB bus and our driver. Backporting concerns also entered the picture, though we can always conditionally compile this "real USB" function as fallback code like we do in other cases where we have to work with older kernels.
Quoted 7 lines
I would be tempted to take the second option as well (I was quite disappointed when I realized such a check wasn't already possible), but using definitions from
usbhid.hor other non-system headers isn't ideal. We'd have to require users to have a full source tree instead of just a "kernel-devel" package in order to build an input-wacom backport (or have the "real USB" function used even when compiled with upstream kernels :().You can also define locally the function with this implementation if you do not want to change
usbhid.hQuoted 7 lines
A modification of the second option that might be a bit better is to have
hid.hexpose a function that does the comparison on behalf of the caller. I presume that someone else could someday want to know if their e.g. I2C device is really from uhid, so I'd probably also change the logic to be "returnhid->ll_driver==uhid_hid_driver". That way we don't have to export and compare against every transport driver...I'd rather ask: "are you of such type?" than "are you not on such type?".
The problem with the negation is that as soon as you want to target only Bluetooth for instance, then you'll need to ask for "bluetooth and not uhid".
OK, that is not clear. An other way of saying it is that you are interesting in casting a pointer into a pointer type from the low level transport driver. So if you are not absolutely sure about it, then you should not cast it.
Quoted 6 lines
A different option I'd considered and am rather fond of would be to define something like "
HID_QUIRK_UHID" (or "HID_QUIRK_VIRTUAL"?) which the uhid driver could set on devices that it creates. It'd be trivial to implement and check and seems like it'd fit right in with the other device quirks. Thoughts?I locally have a patch that achieves that by changing the
hdev->type. But I am not so sure it is good because we would changeHID_TYPE_OTHER,HID_TYPE_USBMOUSE, orHID_TYPE_USBNONEintoHID_TYPE_UHID... Also I do not like much the fact that the transport layer has to change something in hdev if only one does such a change.
Quoted 6 lines
So I would believe it's better to ask usbhid if it handles the device currently than asking uhid if it is not handling it.
Cheers,
Benjamin
Been busy with other tasks and finally getting back to this...
I hadn't considered using export/extern -- I suppose that does work. I think exposing the usb_hid_driver structure (and friends) is a little weird, but its ultimately Jiri's call. *nudges Jiri for comment*
If I make a patch implementing this (with the check implemented in our driver due to usbhid.h concerns), I'd also like to check if only the bus-related hid_ll_driver structures should be exported, or if it makes sense to also export more driver-specific ones (specifically those within "hid-hyperv.c" and "hid-logitech-dj.c").
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....