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

[Lee Jones <lee@kernel.org>]

On Tue, 11 Aug 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
> 
> 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 :)

Okay, leave it with me.

-- 
Lee Jones