HID: wacom: Improve generic name generation

PATCH In review Jason Gerecke 3 participants 10 messages Jun 28, 2017 – Jul 27, 2017

Subsystems (5)

HID Bluetooth USB HID/hidbp drivers (USB keyboards, mice, remote controls, ...) HID wacom driver +1 more subsystems

lore

Share & tools

lore

Jason Gerecke

author

2017-06-28 21:19 UTC #0 lore raw

PATCH v3 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).

Signed-off-by Jason Gerecke <[email protected]>

---
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(-)

drivers/hid/usbhid/hid-core.c +1 -0

index 83772fa7d92a..4a7a306995a9 100644
@@ -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)
 {

drivers/hid/wacom_sys.c +29 ~~-31~~

76 lines not on this page · raw message

Signature · 1 line

2.13.1

Benjamin Tissoires

2017-06-29 14:10 UTC #1 lore raw

Hi Jason,

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

Quoted 8 lines

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:

Quoted 16 lines

Signed-off-by Jason Gerecke <[email protected]>
---
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(-)
drivers/hid/usbhid/hid-core.c +0 -0
index 83772fa7d92a..4a7a306995a9 100644

drivers/hid/usbhid/hid-core.c static struct hid_ll_driver usb_hid_driver = { · line 1272

Quoted 4 lines

drivers/hid/usbhid/hid-core.c +1 -0

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

drivers/hid/wacom_sys.c line 25

Quoted 12 lines

drivers/hid/usbhid/hid-core.c +0 -0

 static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
 {

drivers/hid/wacom_sys.c +1 -0

index 0022c0dac88a..94d493c724c8 100644
@@ -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;
}

Quoted 76 lines

drivers/hid/wacom_sys.c +28 ~~-31~~

+
 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));
 	}

Signature · 1 line

2.13.1

Rest looks good to me.

Cheers,
Benjamin

Jason Gerecke

author

2017-06-29 15:35 UTC #2 lore raw

Quoted 24 lines · Benjamin Tissoires

Hi Jason,
Quoted 1 line · Jason Gerecke
The 'wacom_update_name' function is responsible for producing names
for
Quoted 1 line
the input device nodes based on the hardware device name. Commit
f2209d4
Quoted 1 line
added the ability to strip off prefixes like "Wacom Co.,Ltd." where
the
Quoted 3 lines
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
Quoted 1 line
matching it. We're loath to extend the number of cases out endlessly
and
Quoted 3 lines
so instead try to be smarter about name generation.
We observe that the cause of the redundant prefixes is HID combining
the
Quoted 1 line
manufacturer and product strings of USB devices together. By using
the
Quoted 3 lines
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

Quoted 7 lines

string
Quoted 3 lines
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 :)

Quoted 16 lines

Quoted 4 lines
Signed-off-by Jason Gerecke <[email protected]>
---
Changes from v2:
* Use export/extern to directly gain access to the usb_hid_driver
struct
Quoted 1 line
for comparison to ll_driver (rather than scanning through the list
of
Quoted 4 lines
USB devices for a matching parent).
 drivers/hid/usbhid/hid-core.c |  1 +
 drivers/hid/wacom_sys.c       | 60
+++++++++++++++++++++----------------------
Review
Quoted 3 lines
 2 files changed, 30 insertions(+), 31 deletions(-)
(quoted diff) +0 -0
b/drivers/hid/usbhid/hid-core.c

Quoted 12 lines

drivers/hid/usbhid/hid-core.c static struct hid_ll_driver usb_hid_driver = { · line 1272
Quoted 8 lines
index 83772fa7d92a..4a7a306995a9 100644
drivers/hid/usbhid/hid-core.c +1 -0
@@ -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 :)

Quoted 16 lines

drivers/hid/usbhid/hid-core.c static struct hid_ll_driver usb_hid_driver = { · line 1273
Quoted 2 lines
drivers/hid/usbhid/hid-core.c +0 -0
 static int usbhid_probe(struct usb_interface *intf, const struct
usb_device_id *id)
drivers/hid/wacom_sys.c line 25
Quoted 10 lines
drivers/hid/usbhid/hid-core.c +0 -0
 {
drivers/hid/wacom_sys.c +1 -0
index 0022c0dac88a..94d493c724c8 100644
@@ -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

Quoted 7 lines

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

Quoted 86 lines

drivers/hid/wacom_sys.c line 27

Quoted 3 lines

drivers/hid/wacom_sys.c +1 -0

+
 static int wacom_get_report(struct hid_device *hdev, u8 type, u8

*buf,

drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom

Quoted 3 lines

drivers/hid/wacom_sys.c +0 -0

     size_t size, unsigned int retries)
 {

@@ -2026,41 +2028,37 @@ static void wacom_update_name(struct wacom

*wacom, const char *suffix)

drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom · line 2029

Quoted 3 lines

drivers/hid/wacom_sys.c +0 -0

  /* Generic devices name unspecified */
  if ((features->type == HID_GENERIC) && !strcmp("Wacom HID",

features->name)) {

Quoted 19 lines

drivers/hid/wacom_sys.c +1 ~~-17~~

- 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);

drivers/hid/wacom_sys.c static void wacom_update_name(struct wacom *wacom, const char *suffix)

Quoted 10 lines

drivers/hid/wacom_sys.c +2 -8

- 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);

Quoted 37 lines

drivers/hid/wacom_sys.c +24 -7

+ 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));
  }
--

Quoted 2 lines

2.13.1

Rest looks good to me.

Cheers,
Benjamin

Benjamin Tissoires

2017-06-29 16:07 UTC #3 lore raw

Quoted 92 lines · Jason Gerecke

Quoted 24 lines · Benjamin Tissoires
Hi Jason,
Quoted 1 line · Jason Gerecke
The 'wacom_update_name' function is responsible for producing names
for
Quoted 1 line
the input device nodes based on the hardware device name. Commit
f2209d4
Quoted 1 line
added the ability to strip off prefixes like "Wacom Co.,Ltd." where
the
Quoted 3 lines
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
Quoted 1 line
matching it. We're loath to extend the number of cases out endlessly
and
Quoted 3 lines
so instead try to be smarter about name generation.
We observe that the cause of the redundant prefixes is HID combining
the
Quoted 1 line
manufacturer and product strings of USB devices together. By using
the
Quoted 3 lines
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
Quoted 7 lines
string
Quoted 3 lines
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 :)
Quoted 16 lines
Quoted 4 lines
Signed-off-by Jason Gerecke <[email protected]>
---
Changes from v2:
* Use export/extern to directly gain access to the usb_hid_driver
struct
Quoted 1 line
for comparison to ll_driver (rather than scanning through the list
of
Quoted 4 lines
USB devices for a matching parent).
 drivers/hid/usbhid/hid-core.c |  1 +
 drivers/hid/wacom_sys.c       | 60
+++++++++++++++++++++----------------------
Review
Quoted 3 lines
 2 files changed, 30 insertions(+), 31 deletions(-)
(quoted diff) +0 -0
b/drivers/hid/usbhid/hid-core.c
Quoted 12 lines
drivers/hid/usbhid/hid-core.c static struct hid_ll_driver usb_hid_driver = { · line 1272
Quoted 8 lines
index 83772fa7d92a..4a7a306995a9 100644
drivers/hid/usbhid/hid-core.c +1 -0
@@ -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 :)
Quoted 16 lines
drivers/hid/usbhid/hid-core.c static struct hid_ll_driver usb_hid_driver = { · line 1273
Quoted 2 lines
drivers/hid/usbhid/hid-core.c +0 -0
 static int usbhid_probe(struct usb_interface *intf, const struct
usb_device_id *id)
drivers/hid/wacom_sys.c line 25
Quoted 10 lines
drivers/hid/usbhid/hid-core.c +0 -0
 {
drivers/hid/wacom_sys.c +1 -0
index 0022c0dac88a..94d493c724c8 100644
@@ -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
Quoted 7 lines
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;
}

Quoted 1 line

As I mentioned earlier, our input-wacom driver can't access usbhid.h.

Oops, sorry, I forgot this detail.

Quoted 1 line

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

Nah... :)

Quoted 8 lines

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.

Quoted 5 lines

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

Jason Gerecke

author

2017-06-29 17:13 UTC #4 lore raw

Quoted 110 lines · Benjamin Tissoires

Quoted 92 lines · Jason Gerecke
Quoted 24 lines · Benjamin Tissoires
Hi Jason,
Quoted 1 line · Jason Gerecke
The 'wacom_update_name' function is responsible for producing names
for
Quoted 1 line
the input device nodes based on the hardware device name. Commit
f2209d4
Quoted 1 line
added the ability to strip off prefixes like "Wacom Co.,Ltd." where
the
Quoted 3 lines
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
Quoted 1 line
matching it. We're loath to extend the number of cases out endlessly
and
Quoted 3 lines
so instead try to be smarter about name generation.
We observe that the cause of the redundant prefixes is HID combining
the
Quoted 1 line
manufacturer and product strings of USB devices together. By using
the
Quoted 3 lines
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
Quoted 7 lines
string
Quoted 3 lines
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 :)
Quoted 16 lines
Quoted 4 lines
Signed-off-by Jason Gerecke <[email protected]>
---
Changes from v2:
* Use export/extern to directly gain access to the usb_hid_driver
struct
Quoted 1 line
for comparison to ll_driver (rather than scanning through the list
of
Quoted 4 lines
USB devices for a matching parent).
 drivers/hid/usbhid/hid-core.c |  1 +
 drivers/hid/wacom_sys.c       | 60
+++++++++++++++++++++----------------------
Review
Quoted 3 lines
 2 files changed, 30 insertions(+), 31 deletions(-)
(quoted diff) +0 -0
b/drivers/hid/usbhid/hid-core.c
Quoted 12 lines
drivers/hid/usbhid/hid-core.c static struct hid_ll_driver usb_hid_driver = { · line 1272
Quoted 8 lines
index 83772fa7d92a..4a7a306995a9 100644
drivers/hid/usbhid/hid-core.c +1 -0
@@ -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 :)
Quoted 16 lines
drivers/hid/usbhid/hid-core.c static struct hid_ll_driver usb_hid_driver = { · line 1273
Quoted 2 lines
drivers/hid/usbhid/hid-core.c +0 -0
 static int usbhid_probe(struct usb_interface *intf, const struct
usb_device_id *id)
drivers/hid/wacom_sys.c line 25
Quoted 10 lines
drivers/hid/usbhid/hid-core.c +0 -0
 {
drivers/hid/wacom_sys.c +1 -0
index 0022c0dac88a..94d493c724c8 100644
@@ -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
Quoted 7 lines
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;
}
Quoted 1 line
As I mentioned earlier, our input-wacom driver can't access usbhid.h.
Oops, sorry, I forgot this detail.
Quoted 1 line
I don't imagine a patch to make usbhid.h public would be accepted, and
Nah... :)
Quoted 8 lines
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).

Quoted 5 lines

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.

Quoted 20 lines

Quoted 5 lines
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...

Quoted 6 lines

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.

Jiri Kosina

maintainer

2017-07-20 13:41 UTC #5 lore raw

In reply to Benjamin Tissoires

Quoted 11 lines · Benjamin Tissoires

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 :)

Signature · 2 lines

Jiri Kosina
SUSE Labs

Jason Gerecke

author

2017-07-24 16:46 UTC #6 lore raw

PATCH v4 1/2 HID: introduce hid_is_using_ll_driver

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 <[email protected]>

---
 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(-)

drivers/hid/i2c-hid/i2c-hid.c +2 -1

index fb55fb4c39fc..fef2cfafcedd 100644
@@ -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)
 {

drivers/hid/uhid.c +2 -1

index 7f8ff39ed44b..6f819f144cb4 100644
@@ -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

drivers/hid/usbhid/hid-core.c +2 -1

index 83772fa7d92a..9bef8ece135f 100644
@@ -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)
 {

include/linux/hid.h +11 -0

index 5be325d890d9..a09c5f37dda9 100644
@@ -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

net/bluetooth/hidp/core.c +2 -1

index 0bec4588c3c8..1c3615636bd0 100644
@@ -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(). */

Signature · 1 line

2.13.3

Jason Gerecke

author

2017-07-24 16:46 UTC #7 lore raw

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

Signed-off-by Jason Gerecke <[email protected]>

---
 drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

drivers/hid/wacom_sys.c +27 ~~-31~~

index 0022c0dac88a..236fd54aa938 100644
@@ -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));
 	}

Signature · 1 line

2.13.3

Benjamin Tissoires

Acked-by on v4 2/2

2017-07-26 11:03 UTC #8 lore raw

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

For the series:

Acked-by Benjamin Tissoires <[email protected]>

Quoted 79 lines

 drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

drivers/hid/wacom_sys.c +27 ~~-31~~

index 0022c0dac88a..236fd54aa938 100644
@@ -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));
 	}

Signature · 1 line

2.13.3

Jiri Kosina

maintainer

2017-07-27 13:15 UTC #9 lore raw

Quoted 18 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 5 lines

Quoted 2 lines
Signed-off-by Jason Gerecke <[email protected]>
---
For the series:
Acked-by Benjamin Tissoires <[email protected]>

Now in for-4.14/wacom. Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs