[PATCH 0/2] Expose firmware name to identify tablets in libwacom

[PATCH 0/2] Expose firmware name to identify tablets in libwacom

[José Expósito]

Hi everyone,

This series exposes the UCLogic tablets firmware name to user userspace
via sysfs.

libwacom can use it to identify this family of tablets, which has been
very difficult until now because vendor keep reusing the same VIP/PID.

This is the PR consuming the new interface:
https://github.com/linuxwacom/libwacom/pull/620

Finally, I created a tool to generate a list of firmware name <-> table
model for UCLogic-like drawing tablets:
https://github.com/JoseExposito/uclogic-firmware-names

Best wishes,
José Expósito

José Expósito (2):
  HID: uclogic: Store firmware name in params
  HID: uclogic: Expose firmware name via sysfs

 drivers/hid/hid-uclogic-core.c   | 16 ++++++++++++++++
 drivers/hid/hid-uclogic-params.c | 14 +++++++-------
 drivers/hid/hid-uclogic-params.h |  5 +++++
 3 files changed, 28 insertions(+), 7 deletions(-)

-- 
2.44.0

[PATCH 1/2] HID: uclogic: Store firmware name in params

[José Expósito]

A future patch will need to access the firmware name to expose it to
userspace via sysfs.

Store it in `uclogic_params->fw_name`.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/hid/hid-uclogic-params.c | 14 +++++++-------
 drivers/hid/hid-uclogic-params.h |  5 +++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
index 9859dad36495..79ec3eb80f84 100644
--- a/drivers/hid/hid-uclogic-params.c
+++ b/drivers/hid/hid-uclogic-params.c
@@ -121,6 +121,7 @@ void uclogic_params_hid_dbg(const struct hid_device *hdev,
 		params->invalid ? "true" : "false");
 	hid_dbg(hdev, ".desc_ptr = %p\n", params->desc_ptr);
 	hid_dbg(hdev, ".desc_size = %u\n", params->desc_size);
+	hid_dbg(hdev, ".fw_name = %s\n", params->fw_name);
 	hid_dbg(hdev, ".pen = {\n");
 	uclogic_params_pen_hid_dbg(hdev, &params->pen);
 	hid_dbg(hdev, "\t}\n");
@@ -652,6 +653,7 @@ void uclogic_params_cleanup(struct uclogic_params *params)
 	if (!params->invalid) {
 		size_t i;
 		kfree(params->desc_ptr);
+		kfree(params->fw_name);
 		uclogic_params_pen_cleanup(&params->pen);
 		for (i = 0; i < ARRAY_SIZE(params->frame_list); i++)
 			uclogic_params_frame_cleanup(&params->frame_list[i]);
@@ -837,7 +839,6 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
 	/* The resulting parameters (noop) */
 	struct uclogic_params p = {0, };
 	static const char transition_ver[] = "HUION_T153_160607";
-	char *ver_ptr = NULL;
 	const size_t ver_len = sizeof(transition_ver) + 1;
 	__u8 *params_ptr = NULL;
 	size_t params_len = 0;
@@ -870,14 +871,14 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
 	}
 
 	/* Try to get firmware version */
-	ver_ptr = kzalloc(ver_len, GFP_KERNEL);
-	if (ver_ptr == NULL) {
+	p.fw_name = kzalloc(ver_len, GFP_KERNEL);
+	if (!p.fw_name) {
 		rc = -ENOMEM;
 		goto cleanup;
 	}
-	rc = usb_string(udev, 201, ver_ptr, ver_len);
+	rc = usb_string(udev, 201, p.fw_name, ver_len);
 	if (rc == -EPIPE) {
-		*ver_ptr = '\0';
+		*p.fw_name = '\0';
 	} else if (rc < 0) {
 		hid_err(hdev,
 			"failed retrieving Huion firmware version: %d\n", rc);
@@ -885,7 +886,7 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
 	}
 
 	/* If this is a transition firmware */
-	if (strcmp(ver_ptr, transition_ver) == 0) {
+	if (strcmp(p.fw_name, transition_ver) == 0) {
 		hid_dbg(hdev,
 			"transition firmware detected, not probing pen v2 parameters\n");
 	} else {
@@ -1028,7 +1029,6 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
 	rc = 0;
 cleanup:
 	kfree(params_ptr);
-	kfree(ver_ptr);
 	uclogic_params_cleanup(&p);
 	return rc;
 }
diff --git a/drivers/hid/hid-uclogic-params.h b/drivers/hid/hid-uclogic-params.h
index d6ffadb2f601..412c916770f5 100644
--- a/drivers/hid/hid-uclogic-params.h
+++ b/drivers/hid/hid-uclogic-params.h
@@ -232,6 +232,11 @@ struct uclogic_params {
 	 * List of event hooks.
 	 */
 	struct uclogic_raw_event_hook *event_hooks;
+	/*
+	 * Firmware name, exposed to userspace via sysfs as it is used to
+	 * identify the tablet.
+	 */
+	char *fw_name;
 };
 
 /* Driver data */
-- 
2.44.0

Re: [PATCH 1/2] HID: uclogic: Store firmware name in params

[Benjamin Tissoires]

Hi José,

On Mar 21 2024, José Expósito wrote:
> A future patch will need to access the firmware name to expose it to
> userspace via sysfs.
> 
> Store it in `uclogic_params->fw_name`.
> 
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>  drivers/hid/hid-uclogic-params.c | 14 +++++++-------
>  drivers/hid/hid-uclogic-params.h |  5 +++++
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
> index 9859dad36495..79ec3eb80f84 100644
> --- a/drivers/hid/hid-uclogic-params.c
> +++ b/drivers/hid/hid-uclogic-params.c
> @@ -121,6 +121,7 @@ void uclogic_params_hid_dbg(const struct hid_device *hdev,
>  		params->invalid ? "true" : "false");
>  	hid_dbg(hdev, ".desc_ptr = %p\n", params->desc_ptr);
>  	hid_dbg(hdev, ".desc_size = %u\n", params->desc_size);
> +	hid_dbg(hdev, ".fw_name = %s\n", params->fw_name);
>  	hid_dbg(hdev, ".pen = {\n");
>  	uclogic_params_pen_hid_dbg(hdev, &params->pen);
>  	hid_dbg(hdev, "\t}\n");
> @@ -652,6 +653,7 @@ void uclogic_params_cleanup(struct uclogic_params *params)
>  	if (!params->invalid) {
>  		size_t i;
>  		kfree(params->desc_ptr);
> +		kfree(params->fw_name);
>  		uclogic_params_pen_cleanup(&params->pen);
>  		for (i = 0; i < ARRAY_SIZE(params->frame_list); i++)
>  			uclogic_params_frame_cleanup(&params->frame_list[i]);
> @@ -837,7 +839,6 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
>  	/* The resulting parameters (noop) */
>  	struct uclogic_params p = {0, };
>  	static const char transition_ver[] = "HUION_T153_160607";
> -	char *ver_ptr = NULL;
>  	const size_t ver_len = sizeof(transition_ver) + 1;
>  	__u8 *params_ptr = NULL;
>  	size_t params_len = 0;
> @@ -870,14 +871,14 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
>  	}
>  
>  	/* Try to get firmware version */
> -	ver_ptr = kzalloc(ver_len, GFP_KERNEL);
> -	if (ver_ptr == NULL) {
> +	p.fw_name = kzalloc(ver_len, GFP_KERNEL);
> +	if (!p.fw_name) {
>  		rc = -ENOMEM;
>  		goto cleanup;
>  	}
> -	rc = usb_string(udev, 201, ver_ptr, ver_len);
> +	rc = usb_string(udev, 201, p.fw_name, ver_len);
>  	if (rc == -EPIPE) {
> -		*ver_ptr = '\0';
> +		*p.fw_name = '\0';
>  	} else if (rc < 0) {
>  		hid_err(hdev,
>  			"failed retrieving Huion firmware version: %d\n", rc);
> @@ -885,7 +886,7 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
>  	}
>  
>  	/* If this is a transition firmware */
> -	if (strcmp(ver_ptr, transition_ver) == 0) {
> +	if (strcmp(p.fw_name, transition_ver) == 0) {
>  		hid_dbg(hdev,
>  			"transition firmware detected, not probing pen v2 parameters\n");
>  	} else {
> @@ -1028,7 +1029,6 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
>  	rc = 0;
>  cleanup:
>  	kfree(params_ptr);
> -	kfree(ver_ptr);
>  	uclogic_params_cleanup(&p);
>  	return rc;
>  }
> diff --git a/drivers/hid/hid-uclogic-params.h b/drivers/hid/hid-uclogic-params.h
> index d6ffadb2f601..412c916770f5 100644
> --- a/drivers/hid/hid-uclogic-params.h
> +++ b/drivers/hid/hid-uclogic-params.h
> @@ -232,6 +232,11 @@ struct uclogic_params {
>  	 * List of event hooks.
>  	 */
>  	struct uclogic_raw_event_hook *event_hooks;
> +	/*
> +	 * Firmware name, exposed to userspace via sysfs as it is used to
> +	 * identify the tablet.
> +	 */
> +	char *fw_name;

I can't remember if this was on the table or not, but any reasons to not
use hid->uniq[64] field instead of a custom sysfs?
If there is already a value, we could just append the firmware version
with a colon (:) separator.

The main reason would be to not export a new sysfs specifically for this
driver, but it would also allow HID-BPF to have access to the value and
thus allow to detect if the device works with the given bpf program.

Cheers,
Benjamin

>  };
>  
>  /* Driver data */
> -- 
> 2.44.0
> 

Re: [PATCH 1/2] HID: uclogic: Store firmware name in params

[José Expósito]

Hi Benjamin,

Thanks a lot for the quick review :)

On Thu, Mar 21, 2024 at 04:01:06PM +0100, Benjamin Tissoires wrote:
> 
> Hi José,
> 
> On Mar 21 2024, José Expósito wrote:
> > A future patch will need to access the firmware name to expose it to
> > userspace via sysfs.
> > 
> > Store it in `uclogic_params->fw_name`.
> > 
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> >  drivers/hid/hid-uclogic-params.c | 14 +++++++-------
> >  drivers/hid/hid-uclogic-params.h |  5 +++++
> >  2 files changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-uclogic-params.c b/drivers/hid/hid-uclogic-params.c
> > index 9859dad36495..79ec3eb80f84 100644
> > --- a/drivers/hid/hid-uclogic-params.c
> > +++ b/drivers/hid/hid-uclogic-params.c
> > @@ -121,6 +121,7 @@ void uclogic_params_hid_dbg(const struct hid_device *hdev,
> >  		params->invalid ? "true" : "false");
> >  	hid_dbg(hdev, ".desc_ptr = %p\n", params->desc_ptr);
> >  	hid_dbg(hdev, ".desc_size = %u\n", params->desc_size);
> > +	hid_dbg(hdev, ".fw_name = %s\n", params->fw_name);
> >  	hid_dbg(hdev, ".pen = {\n");
> >  	uclogic_params_pen_hid_dbg(hdev, &params->pen);
> >  	hid_dbg(hdev, "\t}\n");
> > @@ -652,6 +653,7 @@ void uclogic_params_cleanup(struct uclogic_params *params)
> >  	if (!params->invalid) {
> >  		size_t i;
> >  		kfree(params->desc_ptr);
> > +		kfree(params->fw_name);
> >  		uclogic_params_pen_cleanup(&params->pen);
> >  		for (i = 0; i < ARRAY_SIZE(params->frame_list); i++)
> >  			uclogic_params_frame_cleanup(&params->frame_list[i]);
> > @@ -837,7 +839,6 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
> >  	/* The resulting parameters (noop) */
> >  	struct uclogic_params p = {0, };
> >  	static const char transition_ver[] = "HUION_T153_160607";
> > -	char *ver_ptr = NULL;
> >  	const size_t ver_len = sizeof(transition_ver) + 1;
> >  	__u8 *params_ptr = NULL;
> >  	size_t params_len = 0;
> > @@ -870,14 +871,14 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
> >  	}
> >  
> >  	/* Try to get firmware version */
> > -	ver_ptr = kzalloc(ver_len, GFP_KERNEL);
> > -	if (ver_ptr == NULL) {
> > +	p.fw_name = kzalloc(ver_len, GFP_KERNEL);
> > +	if (!p.fw_name) {
> >  		rc = -ENOMEM;
> >  		goto cleanup;
> >  	}
> > -	rc = usb_string(udev, 201, ver_ptr, ver_len);
> > +	rc = usb_string(udev, 201, p.fw_name, ver_len);
> >  	if (rc == -EPIPE) {
> > -		*ver_ptr = '\0';
> > +		*p.fw_name = '\0';
> >  	} else if (rc < 0) {
> >  		hid_err(hdev,
> >  			"failed retrieving Huion firmware version: %d\n", rc);
> > @@ -885,7 +886,7 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
> >  	}
> >  
> >  	/* If this is a transition firmware */
> > -	if (strcmp(ver_ptr, transition_ver) == 0) {
> > +	if (strcmp(p.fw_name, transition_ver) == 0) {
> >  		hid_dbg(hdev,
> >  			"transition firmware detected, not probing pen v2 parameters\n");
> >  	} else {
> > @@ -1028,7 +1029,6 @@ static int uclogic_params_huion_init(struct uclogic_params *params,
> >  	rc = 0;
> >  cleanup:
> >  	kfree(params_ptr);
> > -	kfree(ver_ptr);
> >  	uclogic_params_cleanup(&p);
> >  	return rc;
> >  }
> > diff --git a/drivers/hid/hid-uclogic-params.h b/drivers/hid/hid-uclogic-params.h
> > index d6ffadb2f601..412c916770f5 100644
> > --- a/drivers/hid/hid-uclogic-params.h
> > +++ b/drivers/hid/hid-uclogic-params.h
> > @@ -232,6 +232,11 @@ struct uclogic_params {
> >  	 * List of event hooks.
> >  	 */
> >  	struct uclogic_raw_event_hook *event_hooks;
> > +	/*
> > +	 * Firmware name, exposed to userspace via sysfs as it is used to
> > +	 * identify the tablet.
> > +	 */
> > +	char *fw_name;
> 
> I can't remember if this was on the table or not, but any reasons to not
> use hid->uniq[64] field instead of a custom sysfs?
> If there is already a value, we could just append the firmware version
> with a colon (:) separator.
> 
> The main reason would be to not export a new sysfs specifically for this
> driver, but it would also allow HID-BPF to have access to the value and
> thus allow to detect if the device works with the given bpf program.

That's a very good point.

I tested with some tablets and for HUION, the uniq field is empty.
Other vendors just put garbage there. In fact, we override it in
tablets with battery to avoid a crash [1].

So, it'll be fine to just override the garbage in that field with
an actual unique ID. Sent v2 with the sugested change [2].

Thanks,
Jose

[1] https://github.com/JoseExposito/linux/commit/9845a1304a06805c3193c58283ca428c32c14fa7#diff-b3e9836cea703e50e8febb1f09b86f92833200e7bda4c9c1ea5a1d8b569b01dbR1257-R1262
[2] https://lore.kernel.org/linux-input/20240322100210.107152-1-jose.exposito89@gmail.com/T/

> Cheers,
> Benjamin
> 
> >  };
> >  
> >  /* Driver data */
> > -- 
> > 2.44.0
> > 

[PATCH 2/2] HID: uclogic: Expose firmware name via sysfs

[José Expósito]

Some vendors reuse the same product ID for different tablets, making it
difficult for userspace to figure out which table is connected.
While matching the device name has been used in the past by userspace to
workaround this limitation, some devices have shown that this is not
always a valid approach [1].

However, if userspace could access the firmware version name, it would
be possible to know which tablet is actually connected by matching it
against a list of known firmware names [2].

This patch exposes the firmware version name via sysfs attribute.

Link: https://github.com/linuxwacom/libwacom/issues/609  [1]
Link: https://github.com/linuxwacom/libwacom/issues/610  [2]
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/hid/hid-uclogic-core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index ad74cbc9a0aa..1862ca5805a7 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -50,6 +50,17 @@ static void uclogic_inrange_timeout(struct timer_list *t)
 	input_sync(input);
 }
 
+static ssize_t fw_name_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
+
+	return sysfs_emit(buf, "%s\n", drvdata->params.fw_name);
+}
+
+static DEVICE_ATTR_RO(fw_name);
+
 static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 					unsigned int *rsize)
 {
@@ -217,6 +228,10 @@ static int uclogic_probe(struct hid_device *hdev,
 		goto failure;
 	}
 
+	rc = device_create_file(&hdev->dev, &dev_attr_fw_name);
+	if (rc)
+		hid_warn(hdev, "Unable to create sysfs attribute \"fw_name\", errno %d\n", rc);
+
 	return 0;
 failure:
 	/* Assume "remove" might not be called if "probe" failed */
@@ -477,6 +492,7 @@ static void uclogic_remove(struct hid_device *hdev)
 	del_timer_sync(&drvdata->inrange_timer);
 	hid_hw_stop(hdev);
 	kfree(drvdata->desc_ptr);
+	device_remove_file(&hdev->dev, &dev_attr_fw_name);
 	uclogic_params_cleanup(&drvdata->params);
 }
 
-- 
2.44.0