[PATCH] HID: Recognize "Digitizer" as a valid input application

[PATCH] HID: Recognize "Digitizer" as a valid input application

[Gerecke, Jason]

From: Jason Gerecke <jason.gerecke@wacom.com>

"Digitizer" is a generic usage that may be used by various devices but
which is particularly used by non-display pen tablets. This patch adds the
usage to the list of values matched by the IS_INPUT_APPLICATION() macro
that determines if an input device should be allocated or not.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
---
 include/linux/hid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 4363a63b9775..07803e144d98 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev)
 /* We ignore a few input applications that are not widely used */
 #define IS_INPUT_APPLICATION(a) \
 		(((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
-		|| ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \
+		|| ((a >= HID_DG_DIGITIZER) && (a <= HID_DG_WHITEBOARD)) \
 		|| (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \
 		|| (a == HID_GD_WIRELESS_RADIO_CTLS))
 
-- 
2.38.0

Re: [PATCH] HID: Recognize "Digitizer" as a valid input application

[Benjamin Tissoires]

On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote:
>
> From: Jason Gerecke <jason.gerecke@wacom.com>
>
> "Digitizer" is a generic usage that may be used by various devices but
> which is particularly used by non-display pen tablets. This patch adds the
> usage to the list of values matched by the IS_INPUT_APPLICATION() macro
> that determines if an input device should be allocated or not.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
> ---
>  include/linux/hid.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 4363a63b9775..07803e144d98 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev)
>  /* We ignore a few input applications that are not widely used */
>  #define IS_INPUT_APPLICATION(a) \
>                 (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
> -               || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \

FWIW, this has always been problematic, and I am pretty  sure this is
breaking existing devices.

Have you been running the hid-tools testsuite to see if there were any
regressions?

Cheers,
Benjamin

> +               || ((a >= HID_DG_DIGITIZER) && (a <= HID_DG_WHITEBOARD)) \
>                 || (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \
>                 || (a == HID_GD_WIRELESS_RADIO_CTLS))
>
> --
> 2.38.0
>

Re: [PATCH] HID: Recognize "Digitizer" as a valid input application

[Jason Gerecke]

On Wed, Oct 19, 2022 at 8:40 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote:
> >
> > From: Jason Gerecke <jason.gerecke@wacom.com>
> >
> > "Digitizer" is a generic usage that may be used by various devices but
> > which is particularly used by non-display pen tablets. This patch adds the
> > usage to the list of values matched by the IS_INPUT_APPLICATION() macro
> > that determines if an input device should be allocated or not.
> >
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
> > ---
> >  include/linux/hid.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index 4363a63b9775..07803e144d98 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev)
> >  /* We ignore a few input applications that are not widely used */
> >  #define IS_INPUT_APPLICATION(a) \
> >                 (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
> > -               || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \
>
> FWIW, this has always been problematic, and I am pretty  sure this is
> breaking existing devices.
>
> Have you been running the hid-tools testsuite to see if there were any
> regressions?
>
> Cheers,
> Benjamin
>

I was slightly worried that this usage might have been explicitly
excluded for some compatibility reason, but I didn't see anything in
the commit history that said that. I also had a hard time convincing
myself that allocating an input device for a weird device where it is
unnecessary would cause too much trouble.

I didn't see any regressions when running the hid-tools testsuite. The
output from the 5.15.74 kernel with / without the patch applied is
almost identical (there are numerous test failures in test_tablet.py
even in the unpatched case; maybe I should re-run with Linus's latest
master instead?)

Jason

> > +               || ((a >= HID_DG_DIGITIZER) && (a <= HID_DG_WHITEBOARD)) \
> >                 || (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \
> >                 || (a == HID_GD_WIRELESS_RADIO_CTLS))
> >
> > --
> > 2.38.0
> >
>

Re: [PATCH] HID: Recognize "Digitizer" as a valid input application

[Jason Gerecke]

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


On Thu, Oct 20, 2022 at 7:48 AM Jason Gerecke <killertofu@gmail.com> wrote:
>
> On Wed, Oct 19, 2022 at 8:40 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote:
> > >
> > > From: Jason Gerecke <jason.gerecke@wacom.com>
> > >
> > > "Digitizer" is a generic usage that may be used by various devices but
> > > which is particularly used by non-display pen tablets. This patch adds the
> > > usage to the list of values matched by the IS_INPUT_APPLICATION() macro
> > > that determines if an input device should be allocated or not.
> > >
> > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > > Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
> > > ---
> > >  include/linux/hid.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > > index 4363a63b9775..07803e144d98 100644
> > > --- a/include/linux/hid.h
> > > +++ b/include/linux/hid.h
> > > @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev)
> > >  /* We ignore a few input applications that are not widely used */
> > >  #define IS_INPUT_APPLICATION(a) \
> > >                 (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
> > > -               || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \
> >
> > FWIW, this has always been problematic, and I am pretty  sure this is
> > breaking existing devices.
> >
> > Have you been running the hid-tools testsuite to see if there were any
> > regressions?
> >
> > Cheers,
> > Benjamin
> >
>
> I was slightly worried that this usage might have been explicitly
> excluded for some compatibility reason, but I didn't see anything in
> the commit history that said that. I also had a hard time convincing
> myself that allocating an input device for a weird device where it is
> unnecessary would cause too much trouble.
>
> I didn't see any regressions when running the hid-tools testsuite. The
> output from the 5.15.74 kernel with / without the patch applied is
> almost identical (there are numerous test failures in test_tablet.py
> even in the unpatched case; maybe I should re-run with Linus's latest
> master instead?)
>
> Jason
>

(Apologies for this doubled message, Benjamin -- I didn't "reply all"
on my previous attempt...)

I re-ran the tests with the Linus's latest 6.1-rc2 and have good
results with / without the patch there. The test_tablet.py failures I
previously saw no longer occur, so seem to be specific to stable.
There are still two tests that fail regardless of if the patch is
applied or not, but the patch doesn't seem to introduce any new
failures of its own.

Jason

Re: [PATCH] HID: Recognize "Digitizer" as a valid input application

[Jason Gerecke]

> On Thu, Oct 20, 2022 at 7:48 AM Jason Gerecke <killertofu@gmail.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 8:40 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote:
> > > >
> > > > From: Jason Gerecke <jason.gerecke@wacom.com>
> > > >
> > > > "Digitizer" is a generic usage that may be used by various devices but
> > > > which is particularly used by non-display pen tablets. This patch adds the
> > > > usage to the list of values matched by the IS_INPUT_APPLICATION() macro
> > > > that determines if an input device should be allocated or not.
> > > >
> > > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > > > Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
> > > > ---
> > > >  include/linux/hid.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > > > index 4363a63b9775..07803e144d98 100644
> > > > --- a/include/linux/hid.h
> > > > +++ b/include/linux/hid.h
> > > > @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev)
> > > >  /* We ignore a few input applications that are not widely used */
> > > >  #define IS_INPUT_APPLICATION(a) \
> > > >                 (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
> > > > -               || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \
> > >
> > > FWIW, this has always been problematic, and I am pretty  sure this is
> > > breaking existing devices.
> > >
> > > Have you been running the hid-tools testsuite to see if there were any
> > > regressions?
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
> > I was slightly worried that this usage might have been explicitly
> > excluded for some compatibility reason, but I didn't see anything in
> > the commit history that said that. I also had a hard time convincing
> > myself that allocating an input device for a weird device where it is
> > unnecessary would cause too much trouble.
> >
> > I didn't see any regressions when running the hid-tools testsuite. The
> > output from the 5.15.74 kernel with / without the patch applied is
> > almost identical (there are numerous test failures in test_tablet.py
> > even in the unpatched case; maybe I should re-run with Linus's latest
> > master instead?)
> >
> > Jason
> >
>
> (Apologies for this doubled message, Benjamin -- I didn't "reply all"
> on my previous attempt...)
>
> I re-ran the tests with the Linus's latest 6.1-rc2 and have good
> results with / without the patch there. The test_tablet.py failures I
> previously saw no longer occur, so seem to be specific to stable.
> There are still two tests that fail regardless of if the patch is
> applied or not, but the patch doesn't seem to introduce any new
> failures of its own.
>
> Jason

Still waiting to hear back about this. Are positive results from the
hid-tools tests sufficient, or is there additional work that should be
done for this patch?

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] HID: Recognize "Digitizer" as a valid input application

[José Expósito]

Hi Jason,

> Still waiting to hear back about this. Are positive results from the
> hid-tools tests sufficient, or is there additional work that should be
> done for this patch?

A while ago a similar patch was sent [1] and it was in a similar status
to your patch for a while, so I decided to fix the issue in the UCLogic
driver [2].

I can not tell you if this patch needs additional work, but you might be
interested in fixing it in the Wacom driver, where it'd be easier for you
to test that no regressions are introduced.

I hope this helps a bit,
Jose

[1] https://lore.kernel.org/linux-input/20220804151832.30373-1-openglfreak@googlemail.com/
[2] https://lore.kernel.org/linux-input/d08049f2-443b-f769-cfde-629cdfb96fc0@alexyzhang.dev/T/

Re: [PATCH] HID: Recognize "Digitizer" as a valid input application

[Jason Gerecke]

Thanks for the response, Jose.

Unfortunately this isn't an issue that we can fix in the Wacom driver
since it affects devices that don't use the Wacom driver. While we
could theoretically adopt affected VID:PIDs into the Wacom driver on a
case-by-case basis, there would be a large time lag between us
adopting a device and users running the required kernel. This should
really be something that is fixed in the hid-generic driver, even if
it means some pain trying to ensure we don't break things in the
process... I'm still hoping to hear something back from Benjamin about
my hid-tools test results or if additional testing is needed.

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

On Sat, Dec 3, 2022 at 3:43 AM José Expósito <jose.exposito89@gmail.com> wrote:
>
> Hi Jason,
>
> > Still waiting to hear back about this. Are positive results from the
> > hid-tools tests sufficient, or is there additional work that should be
> > done for this patch?
>
> A while ago a similar patch was sent [1] and it was in a similar status
> to your patch for a while, so I decided to fix the issue in the UCLogic
> driver [2].
>
> I can not tell you if this patch needs additional work, but you might be
> interested in fixing it in the Wacom driver, where it'd be easier for you
> to test that no regressions are introduced.
>
> I hope this helps a bit,
> Jose
>
> [1] https://lore.kernel.org/linux-input/20220804151832.30373-1-openglfreak@googlemail.com/
> [2] https://lore.kernel.org/linux-input/d08049f2-443b-f769-cfde-629cdfb96fc0@alexyzhang.dev/T/

Re: [PATCH] HID: Recognize "Digitizer" as a valid input application

[Jason Gerecke]

Following up on this since it has been a while since the last attempt.
I'm still hoping to get this patch merged if possible :)

The patch still applies cleanly against the for-6.4/core branch and
the hid-tools test suite still runs without any (new*) test failures
when the patch is applied. Please let me know if there's anything I
could / should do to continue moving this forward.


*There are four failures in test_sony.py, but they occur on the
for-6.4/core branch regardless of if the patch is applied or not.

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


On Thu, Dec 15, 2022 at 8:11 AM Jason Gerecke <killertofu@gmail.com> wrote:
>
> Thanks for the response, Jose.
>
> Unfortunately this isn't an issue that we can fix in the Wacom driver
> since it affects devices that don't use the Wacom driver. While we
> could theoretically adopt affected VID:PIDs into the Wacom driver on a
> case-by-case basis, there would be a large time lag between us
> adopting a device and users running the required kernel. This should
> really be something that is fixed in the hid-generic driver, even if
> it means some pain trying to ensure we don't break things in the
> process... I'm still hoping to hear something back from Benjamin about
> my hid-tools test results or if additional testing is needed.
>
> 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....
>
> On Sat, Dec 3, 2022 at 3:43 AM José Expósito <jose.exposito89@gmail.com> wrote:
> >
> > Hi Jason,
> >
> > > Still waiting to hear back about this. Are positive results from the
> > > hid-tools tests sufficient, or is there additional work that should be
> > > done for this patch?
> >
> > A while ago a similar patch was sent [1] and it was in a similar status
> > to your patch for a while, so I decided to fix the issue in the UCLogic
> > driver [2].
> >
> > I can not tell you if this patch needs additional work, but you might be
> > interested in fixing it in the Wacom driver, where it'd be easier for you
> > to test that no regressions are introduced.
> >
> > I hope this helps a bit,
> > Jose
> >
> > [1] https://lore.kernel.org/linux-input/20220804151832.30373-1-openglfreak@googlemail.com/
> > [2] https://lore.kernel.org/linux-input/d08049f2-443b-f769-cfde-629cdfb96fc0@alexyzhang.dev/T/

Re: [PATCH] HID: Recognize "Digitizer" as a valid input application

[Benjamin Tissoires]

On Wed, 19 Oct 2022 08:18:32 -0700, Gerecke, Jason wrote:
> "Digitizer" is a generic usage that may be used by various devices but
> which is particularly used by non-display pen tablets. This patch adds the
> usage to the list of values matched by the IS_INPUT_APPLICATION() macro
> that determines if an input device should be allocated or not.
> 
> 

Applied to hid/hid.git (for-6.4/core), thanks!

[1/1] HID: Recognize "Digitizer" as a valid input application
      https://git.kernel.org/hid/hid/c/a0f5276716c8

Cheers,
-- 
Benjamin Tissoires <benjamin.tissoires@redhat.com>