Re: [PATCH v7 1/4] HID: wacom: Advertise SW_MUTE_DEVICE capability prior to registration

On Mon, Aug 10, 2026 at 3:57 AM Lee Jones <[email protected]> wrote:
>
> Jason,
>
> Can you confirm that you've seen this new version please?
>
> [intentional top-post]
>
>
> On Tue, 04 Aug 2026, Lee Jones wrote:
>
> > Input subsystem guidelines require that device capabilities are advertised
> > before the input device is registered.  The Wacom driver was violating
> > this by advertising the SW_MUTE_DEVICE capability post-registration in
> > wacom_set_shared_values() (and duplicating it in device-specific setup
> > cases).
> >
> > Resolve this by moving the SW_MUTE_DEVICE capability setup to
> > wacom_setup_touch_input_capabilities() for all touch devices that support
> > it, including composite USB generic touch devices.
> >
> > Additionally, replace the lookup-dependent
> > 'wacom_wac->shared->touch->product' references with 'hdev->product'
> > inside wacom_setup_touch_input_capabilities() as 'hdev' is already
> > available and represents the touch device itself.
> >
> > Fixes: d2ec58aee8b1 ("HID: wacom: generic: support generic touch switch")
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> >
> > v4 -> v5: New patch used to split out SW_MUTE_DEVICE as per Jason's request
> > v5 -> v6: Unconditionally advertise SW_MUTE_DEVICE on generic touch devices
> > v6 -> v7: Only advertise SW_MUTE_DEVICE on composite USB generic touch devices
> >
> >  drivers/hid/wacom_sys.c | 23 +++++++++++++++++------
> >  drivers/hid/wacom_wac.c | 19 +++++++++++--------
> >  2 files changed, 28 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 0eafa483b7f7..92b73ed0028f 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -2359,12 +2359,6 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
> >                       wacom_wac->shared->is_touch_on = true;
> >       }
> >
> > -     if (wacom_wac->shared->has_mute_touch_switch &&
> > -         wacom_wac->shared->touch_input) {
> > -             set_bit(EV_SW, wacom_wac->shared->touch_input->evbit);
> > -             input_set_capability(wacom_wac->shared->touch_input, EV_SW,
> > -                                  SW_MUTE_DEVICE);
> > -     }
> >  }
> >
> >  static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
> > @@ -2414,6 +2408,23 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
> >       wacom_retrieve_hid_descriptor(hdev, features);
> >       wacom_setup_device_quirks(wacom);
> >
> > +     if (features->type == HID_GENERIC &&
> > +         (features->device_type & WACOM_DEVICETYPE_TOUCH)) {
> > +             if (wacom->usbdev && wacom->usbdev->actconfig &&
> > +                 wacom->usbdev->actconfig->desc.bNumInterfaces > 1) {
> > +                     /*
> > +                      * Heuristic: Composite USB devices (like tablets with
> > +                      * pen/pad + touch) likely have a touch mute switch.
> > +                      * We flag it here to advertise the capability before
> > +                      * registration. We also set is_soft_touch_switch to
> > +                      * default touch to ON in case there is no physical
> > +                      * switch.
> > +                      */
> > +                     wacom_wac->has_mute_touch_switch = true;
> > +                     wacom_wac->is_soft_touch_switch = true;
> > +             }
> > +     }
> > +

Apologies for the delay, Lee.

[ Begin ramble ]

This probably isn't a half-bad heuristic. I'm a bit hesitant to sign
off on it, mostly because I don't like the idea of the driver
advertising the existence of non-existent hardware.

My understanding is that it is theoretically allowable to change
capabilities post-registration (assuming we take due care to spinlock
relevant structures). The problem with doing so, however, is that if
userland queries the capabilities before we make the change, that it
will be stuck with an incorrect set. Software could theoretically
re-issue EVIOCGBIT ioctls at a later point in time to refresh
capabilities, but if we're honest that doesn't actually happen.

It feels like we're in a situation where we either:

A. Over-declare device capabilities on some devices (mostly older ones
that didn't have a touch switch)

-- or --

B. Have devices with under-declared capabilities if userland opens
them fast enough

The touch switch capability lets userland know to expect switch events
and to potentially offer some kind of software display of its state.
Its presence or absence is not critical since the switch state can
often be observed in other ways (physical hardware switch location) or
toggled back into a disabled state even if not immediately observable.
It's probably marginally better to err on providing state information
in situations where controls don't actually exist, but not by much.

[ End ramble ]

With all of that out of the way, I guess that I've convinced myself
that this is probably acceptable.

The only remaining nitpick I can identify looking at this again is
that the hunks that replace 'wacom_wac->shared->touch->product' with
'hdev->product' should really be in their own standalone cleanup patch
rather than in this (slightly-) behavior modifying one.

Address that one nitpick and I'll send out my reviewed-by for the two
new patches. Apologies again for my many delays... This should be the
last one :)

Jason (she/they)
---
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....

> >       if (features->device_type == WACOM_DEVICETYPE_NONE &&
> >           features->type != WIRELESS) {
> >               error = features->type == HID_GENERIC ? -ENODEV : 0;
> > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> > index a29bf051ada7..afc82fcbb20b 100644
> > --- a/drivers/hid/wacom_wac.c
> > +++ b/drivers/hid/wacom_wac.c
> > @@ -3953,6 +3953,8 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >  int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> >                                        struct wacom_wac *wacom_wac)
> >  {
> > +     struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
> > +     struct hid_device *hdev = wacom->hdev;
> >       struct wacom_features *features = &wacom_wac->features;
> >
> >       if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
> > @@ -3963,9 +3965,12 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> >       else
> >               __set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >
> > -     if (features->type == HID_GENERIC)
> > +     if (features->type == HID_GENERIC) {
> > +             if (wacom_wac->has_mute_touch_switch)
> > +                     input_set_capability(input_dev, EV_SW, SW_MUTE_DEVICE);
> >               /* setup has already been done */
> >               return 0;
> > +     }
> >
> >       input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> >       __set_bit(BTN_TOUCH, input_dev->keybit);
> > @@ -3997,19 +4002,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> >               input_dev->evbit[0] |= BIT_MASK(EV_SW);
> >               __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> >
> > -             if (wacom_wac->shared->touch->product == 0x361) {
> > +             if (hdev->product == 0x361) {
> >                       input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> >                                            0, 12440, 4, 0);
> >                       input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> >                                            0, 8640, 4, 0);
> > -             }
> > -             else if (wacom_wac->shared->touch->product == 0x360) {
> > +             } else if (hdev->product == 0x360) {
> >                       input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> >                                            0, 8960, 4, 0);
> >                       input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> >                                            0, 5920, 4, 0);
> > -             }
> > -             else if (wacom_wac->shared->touch->product == 0x393) {
> > +             } else if (hdev->product == 0x393) {
> >                       input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> >                                            0, 6400, 4, 0);
> >                       input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> > @@ -4039,8 +4042,8 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> >               fallthrough;
> >
> >       case WACOM_27QHDT:
> > -             if (wacom_wac->shared->touch->product == 0x32C ||
> > -                 wacom_wac->shared->touch->product == 0xF6) {
> > +             if (hdev->product == 0x32C ||
> > +                 hdev->product == 0xF6) {
> >                       input_dev->evbit[0] |= BIT_MASK(EV_SW);
> >                       __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> >                       wacom_wac->has_mute_touch_switch = true;
> > --
> > 2.55.0.571.g244d577d93-goog
> >
>
> --
> Lee Jones
>