[PATCH v3] HID: wacom: Improve generic name generation

[PATCH v3] HID: wacom: Improve generic name generation

[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).

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Changes from v2:
 * Use export/extern to directly gain access to the usb_hid_driver struct
   for comparison to ll_driver (rather than scanning through the list of
   USB devices for a matching parent).

 drivers/hid/usbhid/hid-core.c |  1 +
 drivers/hid/wacom_sys.c       | 60 +++++++++++++++++++++----------------------
 2 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 83772fa7d92a..4a7a306995a9 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1269,6 +1269,7 @@ static struct hid_ll_driver usb_hid_driver = {
 	.output_report = usbhid_output_report,
 	.idle = usbhid_idle,
 };
+EXPORT_SYMBOL_GPL(usb_hid_driver);
 
 static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 0022c0dac88a..94d493c724c8 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -22,6 +22,8 @@
 #define DEV_ATTR_WO_PERM (S_IWUSR | S_IWGRP)
 #define DEV_ATTR_RO_PERM (S_IRUSR | S_IRGRP)
 
+extern struct hid_ll_driver usb_hid_driver;
+
 static int wacom_get_report(struct hid_device *hdev, u8 type, u8 *buf,
 			    size_t size, unsigned int retries)
 {
@@ -2026,41 +2028,37 @@ 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 = wacom->hdev->name;
 
-			/* 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 (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->ll_driver == &usb_hid_driver) {
+			struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent);
+			struct usb_device *dev = interface_to_usbdev(intf);
+			product_name = dev->product;
+		}
 
-			/* get rid of trailing whitespace */
-			if (name[strlen(name)-1] == ' ')
-				name[strlen(name)-1] = '\0';
+		if (wacom->hdev->bus == BUS_I2C) {
+			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 {
-			/* no meaningful name retrieved. use product ID */
-			snprintf(name, sizeof(name),
-				 "%s %X", features->name, wacom->hdev->product);
+			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.13.1

Re: [PATCH v3] HID: wacom: Improve generic name generation

[Benjamin Tissoires]

Hi Jason,

On Jun 28 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).
> 

Sorry for nitpicking, but I have a couple of comments:

> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
> Changes from v2:
>  * Use export/extern to directly gain access to the usb_hid_driver struct
>    for comparison to ll_driver (rather than scanning through the list of
>    USB devices for a matching parent).
> 
>  drivers/hid/usbhid/hid-core.c |  1 +
>  drivers/hid/wacom_sys.c       | 60 +++++++++++++++++++++----------------------
>  2 files changed, 30 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 83772fa7d92a..4a7a306995a9 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1269,6 +1269,7 @@ static struct hid_ll_driver usb_hid_driver = {
>  	.output_report = usbhid_output_report,
>  	.idle = usbhid_idle,
>  };
> +EXPORT_SYMBOL_GPL(usb_hid_driver);

I would rather see this in a separate commit (in case we need to revert
this one, we shouldn't revert the export).

>  
>  static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  {
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 0022c0dac88a..94d493c724c8 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -22,6 +22,8 @@
>  #define DEV_ATTR_WO_PERM (S_IWUSR | S_IWGRP)
>  #define DEV_ATTR_RO_PERM (S_IRUSR | S_IRGRP)
>  
> +extern struct hid_ll_driver usb_hid_driver;

Ouch. I'd rather see that one in drivers/hid/usbhid/usbhid.h

I would like also to have the following in usbhid.h, that you can reuse
later in the patch:
static inline bool hid_is_using_usbhid(struct hid_device *hdev)
{
	return hdev->ll_driver == &usb_hid_driver;
}


> +
>  static int wacom_get_report(struct hid_device *hdev, u8 type, u8 *buf,
>  			    size_t size, unsigned int retries)
>  {
> @@ -2026,41 +2028,37 @@ 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 = wacom->hdev->name;
>  
> -			/* 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 (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->ll_driver == &usb_hid_driver) {
> +			struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent);
> +			struct usb_device *dev = interface_to_usbdev(intf);
> +			product_name = dev->product;
> +		}
>  
> -			/* get rid of trailing whitespace */
> -			if (name[strlen(name)-1] == ' ')
> -				name[strlen(name)-1] = '\0';
> +		if (wacom->hdev->bus == BUS_I2C) {
> +			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 {
> -			/* no meaningful name retrieved. use product ID */
> -			snprintf(name, sizeof(name),
> -				 "%s %X", features->name, wacom->hdev->product);
> +			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.13.1
> 

Rest looks good to me.

Cheers,
Benjamin

Re: [PATCH v3] HID: wacom: Improve generic name generation

[Jason Gerecke]

On June 29, 2017 7:10:21 AM PDT, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>Hi Jason,
>
>On Jun 28 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).
>>
>
>Sorry for nitpicking, but I have a couple of comments:
>

Might as well get the patch right :)

>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>> ---
>> Changes from v2:
>>  * Use export/extern to directly gain access to the usb_hid_driver
>struct
>>    for comparison to ll_driver (rather than scanning through the list
>of
>>    USB devices for a matching parent).
>>
>>  drivers/hid/usbhid/hid-core.c |  1 +
>>  drivers/hid/wacom_sys.c       | 60
>+++++++++++++++++++++----------------------
>>  2 files changed, 30 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c
>b/drivers/hid/usbhid/hid-core.c
>> index 83772fa7d92a..4a7a306995a9 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1269,6 +1269,7 @@ static struct hid_ll_driver usb_hid_driver = {
>>   .output_report = usbhid_output_report,
>>   .idle = usbhid_idle,
>>  };
>> +EXPORT_SYMBOL_GPL(usb_hid_driver);
>
>I would rather see this in a separate commit (in case we need to revert
>this one, we shouldn't revert the export).
>

Ack. Left it here momentarily to keep mail noise down while the review
process chugs along :)

>>
>>  static int usbhid_probe(struct usb_interface *intf, const struct
>usb_device_id *id)
>>  {
>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> index 0022c0dac88a..94d493c724c8 100644
>> --- a/drivers/hid/wacom_sys.c
>> +++ b/drivers/hid/wacom_sys.c
>> @@ -22,6 +22,8 @@
>>  #define DEV_ATTR_WO_PERM (S_IWUSR | S_IWGRP)
>>  #define DEV_ATTR_RO_PERM (S_IRUSR | S_IRGRP)
>>
>> +extern struct hid_ll_driver usb_hid_driver;
>
>Ouch. I'd rather see that one in drivers/hid/usbhid/usbhid.h
>
>I would like also to have the following in usbhid.h, that you can reuse
>later in the patch:
>static inline bool hid_is_using_usbhid(struct hid_device *hdev)
>{
> return hdev->ll_driver == &usb_hid_driver;
>}
>

As I mentioned earlier, our input-wacom driver can't access usbhid.h.
I don't imagine a patch to make usbhid.h public would be accepted, and
I am strongly against requiring our users to have a copy of their
distro's full kernel source tree (not just development headers) merely
to compile our driver. Certainly we could create our own definition of
`hid_is_using_usbhid` within input-wacom which only uses
otherwise-public functions, but it'd be a hack that we'd have to
maintain indefinitely. I'm looking to create a patch which can work
both upstream and downstream (hence my earlier awkward
`wacom_is_real_usb` function that used only public functions).

Would it be reasonable to define `hid_is_using_usbhid` within hid.h
instead? I'm guessing the answer is "no" since the function is
transport-specific, but knowing what specific ll_driver is in use /is/
something useful to any HID driver...

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....

>
>> +
>>  static int wacom_get_report(struct hid_device *hdev, u8 type, u8
>*buf,
>>      size_t size, unsigned int retries)
>>  {
>> @@ -2026,41 +2028,37 @@ 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 = wacom->hdev->name;
>>
>> - /* 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 (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->ll_driver == &usb_hid_driver) {
>> + struct usb_interface *intf =
>to_usb_interface(wacom->hdev->dev.parent);
>> + struct usb_device *dev = interface_to_usbdev(intf);
>> + product_name = dev->product;
>> + }
>>
>> - /* get rid of trailing whitespace */
>> - if (name[strlen(name)-1] == ' ')
>> - name[strlen(name)-1] = '\0';
>> + if (wacom->hdev->bus == BUS_I2C) {
>> + 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 {
>> - /* no meaningful name retrieved. use product ID */
>> - snprintf(name, sizeof(name),
>> - "%s %X", features->name, wacom->hdev->product);
>> + 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.13.1
>>
>
>Rest looks good to me.
>
>Cheers,
>Benjamin

Re: [PATCH v3] HID: wacom: Improve generic name generation

[Benjamin Tissoires]

On Jun 29 2017 or thereabouts, Jason Gerecke wrote:
> On June 29, 2017 7:10:21 AM PDT, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >Hi Jason,
> >
> >On Jun 28 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).
> >>
> >
> >Sorry for nitpicking, but I have a couple of comments:
> >
> 
> Might as well get the patch right :)
> 
> >> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> >> ---
> >> Changes from v2:
> >>  * Use export/extern to directly gain access to the usb_hid_driver
> >struct
> >>    for comparison to ll_driver (rather than scanning through the list
> >of
> >>    USB devices for a matching parent).
> >>
> >>  drivers/hid/usbhid/hid-core.c |  1 +
> >>  drivers/hid/wacom_sys.c       | 60
> >+++++++++++++++++++++----------------------
> >>  2 files changed, 30 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/hid/usbhid/hid-core.c
> >b/drivers/hid/usbhid/hid-core.c
> >> index 83772fa7d92a..4a7a306995a9 100644
> >> --- a/drivers/hid/usbhid/hid-core.c
> >> +++ b/drivers/hid/usbhid/hid-core.c
> >> @@ -1269,6 +1269,7 @@ static struct hid_ll_driver usb_hid_driver = {
> >>   .output_report = usbhid_output_report,
> >>   .idle = usbhid_idle,
> >>  };
> >> +EXPORT_SYMBOL_GPL(usb_hid_driver);
> >
> >I would rather see this in a separate commit (in case we need to revert
> >this one, we shouldn't revert the export).
> >
> 
> Ack. Left it here momentarily to keep mail noise down while the review
> process chugs along :)
> 
> >>
> >>  static int usbhid_probe(struct usb_interface *intf, const struct
> >usb_device_id *id)
> >>  {
> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> >> index 0022c0dac88a..94d493c724c8 100644
> >> --- a/drivers/hid/wacom_sys.c
> >> +++ b/drivers/hid/wacom_sys.c
> >> @@ -22,6 +22,8 @@
> >>  #define DEV_ATTR_WO_PERM (S_IWUSR | S_IWGRP)
> >>  #define DEV_ATTR_RO_PERM (S_IRUSR | S_IRGRP)
> >>
> >> +extern struct hid_ll_driver usb_hid_driver;
> >
> >Ouch. I'd rather see that one in drivers/hid/usbhid/usbhid.h
> >
> >I would like also to have the following in usbhid.h, that you can reuse
> >later in the patch:
> >static inline bool hid_is_using_usbhid(struct hid_device *hdev)
> >{
> > return hdev->ll_driver == &usb_hid_driver;
> >}
> >
> 
> As I mentioned earlier, our input-wacom driver can't access usbhid.h.

Oops, sorry, I forgot this detail.

> I don't imagine a patch to make usbhid.h public would be accepted, and

Nah... :)

> I am strongly against requiring our users to have a copy of their
> distro's full kernel source tree (not just development headers) merely
> to compile our driver. Certainly we could create our own definition of
> `hid_is_using_usbhid` within input-wacom which only uses
> otherwise-public functions, but it'd be a hack that we'd have to
> maintain indefinitely. I'm looking to create a patch which can work
> both upstream and downstream (hence my earlier awkward
> `wacom_is_real_usb` function that used only public functions).

But why don't you just add a usbhid/usbhid.h file downstream containing
just the export and this inlined function?

You can just sync this extra header when changes are coming.

> 
> Would it be reasonable to define `hid_is_using_usbhid` within hid.h
> instead? I'm guessing the answer is "no" since the function is
> transport-specific, but knowing what specific ll_driver is in use /is/
> something useful to any HID driver...

That's a tough question. Depending on how I look, both arguments are
valid. Still, I have a slight preference for having a
hid_is_using_usbhid() function in usbhid.h instead of plain hid.h.

OTOH, I would think having a more generic approach would be fine:
bool inline bool hid_is_using_driver(struct hid_device *hdev,
		struct hid_ll_driver *driver)
{
	return dev->ll_driver == driver;
}

And we can start adding the various extern definitions of the
ll_drivers in hid.h too...

How does that sound? Jiri?

Cheers,
Benjamin

Re: [PATCH v3] HID: wacom: Improve generic name generation

[Jason Gerecke]

On 06/29/2017 09:07 AM, Benjamin Tissoires wrote:
> On Jun 29 2017 or thereabouts, Jason Gerecke wrote:
>> On June 29, 2017 7:10:21 AM PDT, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>>> Hi Jason,
>>>
>>> On Jun 28 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).
>>>>
>>>
>>> Sorry for nitpicking, but I have a couple of comments:
>>>
>>
>> Might as well get the patch right :)
>>
>>>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>>>> ---
>>>> Changes from v2:
>>>>  * Use export/extern to directly gain access to the usb_hid_driver
>>> struct
>>>>    for comparison to ll_driver (rather than scanning through the list
>>> of
>>>>    USB devices for a matching parent).
>>>>
>>>>  drivers/hid/usbhid/hid-core.c |  1 +
>>>>  drivers/hid/wacom_sys.c       | 60
>>> +++++++++++++++++++++----------------------
>>>>  2 files changed, 30 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/usbhid/hid-core.c
>>> b/drivers/hid/usbhid/hid-core.c
>>>> index 83772fa7d92a..4a7a306995a9 100644
>>>> --- a/drivers/hid/usbhid/hid-core.c
>>>> +++ b/drivers/hid/usbhid/hid-core.c
>>>> @@ -1269,6 +1269,7 @@ static struct hid_ll_driver usb_hid_driver = {
>>>>   .output_report = usbhid_output_report,
>>>>   .idle = usbhid_idle,
>>>>  };
>>>> +EXPORT_SYMBOL_GPL(usb_hid_driver);
>>>
>>> I would rather see this in a separate commit (in case we need to revert
>>> this one, we shouldn't revert the export).
>>>
>>
>> Ack. Left it here momentarily to keep mail noise down while the review
>> process chugs along :)
>>
>>>>
>>>>  static int usbhid_probe(struct usb_interface *intf, const struct
>>> usb_device_id *id)
>>>>  {
>>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>>> index 0022c0dac88a..94d493c724c8 100644
>>>> --- a/drivers/hid/wacom_sys.c
>>>> +++ b/drivers/hid/wacom_sys.c
>>>> @@ -22,6 +22,8 @@
>>>>  #define DEV_ATTR_WO_PERM (S_IWUSR | S_IWGRP)
>>>>  #define DEV_ATTR_RO_PERM (S_IRUSR | S_IRGRP)
>>>>
>>>> +extern struct hid_ll_driver usb_hid_driver;
>>>
>>> Ouch. I'd rather see that one in drivers/hid/usbhid/usbhid.h
>>>
>>> I would like also to have the following in usbhid.h, that you can reuse
>>> later in the patch:
>>> static inline bool hid_is_using_usbhid(struct hid_device *hdev)
>>> {
>>> return hdev->ll_driver == &usb_hid_driver;
>>> }
>>>
>>
>> As I mentioned earlier, our input-wacom driver can't access usbhid.h.
> 
> Oops, sorry, I forgot this detail.
> 
>> I don't imagine a patch to make usbhid.h public would be accepted, and
> 
> Nah... :)
> 
>> I am strongly against requiring our users to have a copy of their
>> distro's full kernel source tree (not just development headers) merely
>> to compile our driver. Certainly we could create our own definition of
>> `hid_is_using_usbhid` within input-wacom which only uses
>> otherwise-public functions, but it'd be a hack that we'd have to
>> maintain indefinitely. I'm looking to create a patch which can work
>> both upstream and downstream (hence my earlier awkward
>> `wacom_is_real_usb` function that used only public functions).
> 
> But why don't you just add a usbhid/usbhid.h file downstream containing
> just the export and this inlined function?
> 
> You can just sync this extra header when changes are coming.
> 

I'd not considered this. Keeping watch over that header for changes is
still not ideal, but at least its copy/paste instead of different code.
I think I could accept that maintenance burden.

>>
>> Would it be reasonable to define `hid_is_using_usbhid` within hid.h
>> instead? I'm guessing the answer is "no" since the function is
>> transport-specific, but knowing what specific ll_driver is in use /is/
>> something useful to any HID driver...
> 
> That's a tough question. Depending on how I look, both arguments are
> valid. Still, I have a slight preference for having a
> hid_is_using_usbhid() function in usbhid.h instead of plain hid.h.
> 
> OTOH, I would think having a more generic approach would be fine:
> bool inline bool hid_is_using_driver(struct hid_device *hdev,
> 		struct hid_ll_driver *driver)
> {
> 	return dev->ll_driver == driver;
> }
> 
> And we can start adding the various extern definitions of the
> ll_drivers in hid.h too...
> 
> How does that sound? Jiri?
> 
> Cheers,
> Benjamin
>
>From my viewpoint this would be ideal since it doesn't require us to
copy and indefinitely maintain upstream code locally.

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....

Re: [PATCH v3] HID: wacom: Improve generic name generation

[Jiri Kosina]

On Thu, 29 Jun 2017, Benjamin Tissoires wrote:

> OTOH, I would think having a more generic approach would be fine:
> bool inline bool hid_is_using_driver(struct hid_device *hdev,
> 		struct hid_ll_driver *driver)
> {
> 	return dev->ll_driver == driver;
> }
> 
> And we can start adding the various extern definitions of the
> ll_drivers in hid.h too...
> 
> How does that sound? Jiri?

Yeah, I find it slightly ugly, but at least it's generic :)

-- 
Jiri Kosina
SUSE Labs

[PATCH v4 1/2] HID: introduce hid_is_using_ll_driver

[Jason Gerecke]

Although HID itself is transport-agnostic, occasionally a driver may
want to interact with the low-level transport that a device is connected
through. To do this, we need to know what kind of bus is in use. The
first guess may be to look at the 'bus' field of the 'struct hid_device',
but this field may be emulated in some cases (e.g. uhid).

More ideally, we can check which ll_driver a device is using. This
function introduces a 'hid_is_using_ll_driver' function and makes the
'struct hid_ll_driver' of the four most common transports accessible
through hid.h.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/i2c-hid/i2c-hid.c |  3 ++-
 drivers/hid/uhid.c            |  3 ++-
 drivers/hid/usbhid/hid-core.c |  3 ++-
 include/linux/hid.h           | 11 +++++++++++
 net/bluetooth/hidp/core.c     |  3 ++-
 5 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index fb55fb4c39fc..fef2cfafcedd 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -794,7 +794,7 @@ static int i2c_hid_power(struct hid_device *hid, int lvl)
 	return 0;
 }
 
-static struct hid_ll_driver i2c_hid_ll_driver = {
+struct hid_ll_driver i2c_hid_ll_driver = {
 	.parse = i2c_hid_parse,
 	.start = i2c_hid_start,
 	.stop = i2c_hid_stop,
@@ -804,6 +804,7 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
 	.output_report = i2c_hid_output_report,
 	.raw_request = i2c_hid_raw_request,
 };
+EXPORT_SYMBOL_GPL(i2c_hid_ll_driver);
 
 static int i2c_hid_init_irq(struct i2c_client *client)
 {
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 7f8ff39ed44b..6f819f144cb4 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -369,7 +369,7 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
 	return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);
 }
 
-static struct hid_ll_driver uhid_hid_driver = {
+struct hid_ll_driver uhid_hid_driver = {
 	.start = uhid_hid_start,
 	.stop = uhid_hid_stop,
 	.open = uhid_hid_open,
@@ -378,6 +378,7 @@ static struct hid_ll_driver uhid_hid_driver = {
 	.raw_request = uhid_hid_raw_request,
 	.output_report = uhid_hid_output_report,
 };
+EXPORT_SYMBOL_GPL(uhid_hid_driver);
 
 #ifdef CONFIG_COMPAT
 
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 83772fa7d92a..9bef8ece135f 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1256,7 +1256,7 @@ static int usbhid_idle(struct hid_device *hid, int report, int idle,
 	return hid_set_idle(dev, ifnum, report, idle);
 }
 
-static struct hid_ll_driver usb_hid_driver = {
+struct hid_ll_driver usb_hid_driver = {
 	.parse = usbhid_parse,
 	.start = usbhid_start,
 	.stop = usbhid_stop,
@@ -1269,6 +1269,7 @@ static struct hid_ll_driver usb_hid_driver = {
 	.output_report = usbhid_output_report,
 	.idle = usbhid_idle,
 };
+EXPORT_SYMBOL_GPL(usb_hid_driver);
 
 static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5be325d890d9..a09c5f37dda9 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -765,6 +765,17 @@ struct hid_ll_driver {
 	int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
 };
 
+extern struct hid_ll_driver i2c_hid_ll_driver;
+extern struct hid_ll_driver hidp_hid_driver;
+extern struct hid_ll_driver uhid_hid_driver;
+extern struct hid_ll_driver usb_hid_driver;
+
+static inline bool hid_is_using_ll_driver(struct hid_device *hdev,
+		struct hid_ll_driver *driver)
+{
+	return hdev->ll_driver == driver;
+}
+
 #define	PM_HINT_FULLON	1<<5
 #define PM_HINT_NORMAL	1<<1
 
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 0bec4588c3c8..1c3615636bd0 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -733,7 +733,7 @@ static void hidp_stop(struct hid_device *hid)
 	hid->claimed = 0;
 }
 
-static struct hid_ll_driver hidp_hid_driver = {
+struct hid_ll_driver hidp_hid_driver = {
 	.parse = hidp_parse,
 	.start = hidp_start,
 	.stop = hidp_stop,
@@ -742,6 +742,7 @@ static struct hid_ll_driver hidp_hid_driver = {
 	.raw_request = hidp_raw_request,
 	.output_report = hidp_output_report,
 };
+EXPORT_SYMBOL_GPL(hidp_hid_driver);
 
 /* This function sets up the hid device. It does not add it
    to the HID system. That is done in hidp_add_connection(). */
-- 
2.13.3

[PATCH v4 2/2] HID: wacom: Improve generic name generation

[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).

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 0022c0dac88a..236fd54aa938 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2026,41 +2026,37 @@ 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 = wacom->hdev->name;
 
-			/* 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 (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 (hid_is_using_ll_driver(wacom->hdev, &usb_hid_driver)) {
+			struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent);
+			struct usb_device *dev = interface_to_usbdev(intf);
+			product_name = dev->product;
+		}
 
-			/* get rid of trailing whitespace */
-			if (name[strlen(name)-1] == ' ')
-				name[strlen(name)-1] = '\0';
+		if (wacom->hdev->bus == BUS_I2C) {
+			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 {
-			/* no meaningful name retrieved. use product ID */
-			snprintf(name, sizeof(name),
-				 "%s %X", features->name, wacom->hdev->product);
+			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.13.3

Re: [PATCH v4 2/2] HID: wacom: Improve generic name generation

[Benjamin Tissoires]

On Jul 24 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@wacom.com>
> ---

For the series:
Acked-By: Benjamin Tissoires <benjamin.tissoires@redhat.com>

>  drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 0022c0dac88a..236fd54aa938 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2026,41 +2026,37 @@ 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 = wacom->hdev->name;
>  
> -			/* 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 (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 (hid_is_using_ll_driver(wacom->hdev, &usb_hid_driver)) {
> +			struct usb_interface *intf = to_usb_interface(wacom->hdev->dev.parent);
> +			struct usb_device *dev = interface_to_usbdev(intf);
> +			product_name = dev->product;
> +		}
>  
> -			/* get rid of trailing whitespace */
> -			if (name[strlen(name)-1] == ' ')
> -				name[strlen(name)-1] = '\0';
> +		if (wacom->hdev->bus == BUS_I2C) {
> +			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 {
> -			/* no meaningful name retrieved. use product ID */
> -			snprintf(name, sizeof(name),
> -				 "%s %X", features->name, wacom->hdev->product);
> +			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.13.3
> 

Re: [PATCH v4 2/2] HID: wacom: Improve generic name generation

[Jiri Kosina]

On Wed, 26 Jul 2017, Benjamin Tissoires wrote:

> On Jul 24 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@wacom.com>
> > ---
> 
> For the series:
> Acked-By: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Now in for-4.14/wacom. Thanks,

-- 
Jiri Kosina
SUSE Labs