Re: [PATCH 4.19 027/125] HID: wacom: generic: Treat serial number and related fields as unsigned

[Gerecke, Jason]

> From: Jiri Kosina <jikos@xxxxxxxxxx>
> Sent: Wednesday, November 13, 2019 3:02 AM
> To: Pavel Machek <pavel@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx <stable@xxxxxxxxxxxxxxx>; Gerecke, Jason <Jason.Gerecke@xxxxxxxxx>; Skomra, Aaron <Aaron.Skomra@xxxxxxxxx>
> Subject: Re: [PATCH 4.19 027/125] HID: wacom: generic: Treat serial number and related fields as unsigned
>  
> 
> [EXTERNAL]
> 
> On Wed, 13 Nov 2019, Pavel Machek wrote:
> 
> > > From: Jason Gerecke <killertofu@xxxxxxxxx>
> > >
> > > commit ff479731c3859609530416a18ddb3db5db019b66 upstream.
> > >
> > > The HID descriptors for most Wacom devices oddly declare the serial
> > > number and other related fields as signed integers. When these numbers
> > > are ingested by the HID subsystem, they are automatically sign-extended
> > > into 32-bit integers. We treat the fields as unsigned elsewhere in the
> > > kernel and userspace, however, so this sign-extension causes problems.
> > > In particular, the sign-extended tool ID sent to userspace as ABS_MISC
> > > does not properly match unsigned IDs used by xf86-input-wacom and libwacom.
> > >
> > > We introduce a function 'wacom_s32tou' that can undo the automatic sign
> > > extension performed by 'hid_snto32'. We call this function when processing
> > > the serial number and related fields to ensure that we are dealing with
> > > and reporting the unsigned form. We opt to use this method rather than
> > > adding a descriptor fixup in 'wacom_hid_usage_quirk' since it should be
> > > more robust in the face of future devices.
> >
> > > +++ b/drivers/hid/wacom.h
> > > @@ -205,6 +205,21 @@ static inline void wacom_schedule_work(s
> > >     }
> > >  }
> > >
> > > +/*
> > > + * Convert a signed 32-bit integer to an unsigned n-bit integer. Undoes
> > > + * the normally-helpful work of 'hid_snto32' for fields that use signed
> > > + * ranges for questionable reasons.
> > > + */
> > > +static inline __u32 wacom_s32tou(s32 value, __u8 n)
> > > +{
> > > +   switch (n) {
> > > +   case 8:  return ((__u8)value);
> > > +   case 16: return ((__u16)value);
> > > +   case 32: return ((__u32)value);
> > > +   }
> > > +   return value & (1 << (n - 1)) ? value & (~(~0U << n)) : value;
> > > +}
> >
> > Can we do something like:
> >

The implementation here was copied from 'hid_snto32' (except, of course,
to make it convert signed to unsigned). That function provides
justification for how it is written, but if changes make sense then
we should probably do them to both this and 'hid_snto32'.

Jason

> >     BUG_ON(n>32);
> 
> Please no BUG_ON()s in bitop helpers.
> 
> Thanks,
> 
> --
> Jiri Kosina
> SUSE Labs
>