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

[Benjamin Tissoires <bentiss@kernel.org>]

On Aug 11 2026, Jason Gerecke wrote:
> On Mon, Aug 10, 2026 at 3:57 AM Lee Jones <lee@kernel.org> 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 <lee@kernel.org>
> > > ---
> > >
> > > 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

or C: do not put random heuristic and lie to userspace.

The problem comes from the fact that the driver knows after the fact
that a button comes from a different HID node and is redirected in its
own input device.

So: either enforce that the pad device is registered first
(-EPROBE_DEFER), or defer the registering of the touch input node when
both have been parsed/analyzed.

But anyway, we should not call input_register on the touch while we
haven't parsed the pad. period.

Having such a heuristic on a USB composite device feels like shooting
ourselves in the foot.

Cheers,
Benjamin

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