On 05/18/2017 12:20 AM, Benjamin Tissoires wrote:
> On May 17 2017 or thereabouts, Jason Gerecke wrote:
>> 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).
>>
>> Signed-off-by: Jason Gerecke <jason.gerecke@xxxxxxxxx>
>> ---
>
> 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).
>
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....
> Besides that, small nitpicks:
>
>> drivers/hid/wacom_sys.c | 90 ++++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 60 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> index 838c1ebfffa9..c68458e44139 100644
>> --- a/drivers/hid/wacom_sys.c
>> +++ b/drivers/hid/wacom_sys.c
>> @@ -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)) {
>
> wacom_is_real_usb() already checks for BUS_USB, so the first test is
> redundant.
>
>> + 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
>
>> + product_name = wacom->hdev->name;
>> }
>> + }
>> + else if (wacom->hdev->bus == BUS_BLUETOOTH) {
>
> likewise
>
>> + 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;
> }
>
>>
>> - /* 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
>
>> + 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));
>> }
>> --
>> 2.12.2
>>
>
> Cheers,
> Benjamin
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
