Re: [PATCH 4.19 146/191] HID: wacom: Avoid entering wacom_wac_pen_report for pad / battery

[Gerecke, Jason]

> From: Pavel Machek <pavel@xxxxxx>
> Sent: Tuesday, November 3, 2020 11:52 PM
>
> > To correct this, we restore a version of the `WACOM_PAD_FIELD` check
> > in `wacom_wac_collection()` and return early. This effectively prevents
> > pad / battery collections from being reported until the very end of the
> > report as originally intended.
> 
> Okay... but code is either wrong or very confusing:
> 
> > +++ b/drivers/hid/wacom_wac.c
> > @@ -2729,7 +2729,9 @@ static int wacom_wac_collection(struct h
> >        if (report->type != HID_INPUT_REPORT)
> >                return -1;
> >  
> > -     if (WACOM_PEN_FIELD(field) && wacom->wacom_wac.pen_input)
> > +     if (WACOM_PAD_FIELD(field))
> > +             return 0;
> > +     else if (WACOM_PEN_FIELD(field) && wacom->wacom_wac.pen_input)
> >                wacom_wac_pen_report(hdev, report);
> 
> wacom_wac_pen_report() can never be called, because WACOM_PEN_FIELD()
> can not be true here; if it was we'd return in the line above.

For reference, here's the definition for WACOM_PAD_FIELD() and WACOM_PEN_FIELD():

> #define WACOM_PAD_FIELD(f)	(((f)->physical == HID_DG_TABLETFUNCTIONKEY) || \
> 				 ((f)->physical == WACOM_HID_WD_DIGITIZERFNKEYS) || \
> 				 ((f)->physical == WACOM_HID_WD_DIGITIZERINFO))
> 
> #define WACOM_PEN_FIELD(f)	(((f)->logical == HID_DG_STYLUS) || \
> 				 ((f)->physical == HID_DG_STYLUS) || \
> 				 ((f)->physical == HID_DG_PEN) || \
> 				 ((f)->application == HID_DG_PEN) || \
> 				 ((f)->application == HID_DG_DIGITIZER) || \
> 				 ((f)->application == WACOM_HID_WD_PEN) || \
> 				 ((f)->application == WACOM_HID_WD_DIGITIZER) || \
> 				 ((f)->application == WACOM_HID_G9_PEN) || \
> 				 ((f)->application == WACOM_HID_G11_PEN))

WACOM_PAD_FIELD() evaluates to `true` for pad data *not* pen data because pen data is not inside any of the 3 physical collections its looks for.

WACOM_PEN_FIELD() evaluates to `true` for pad data *and* pen data because both types of data are inside of the Digitizer application collection.

Without the WACOM_PAD_FIELD() check in place at the very beginning, both pad and pen data would trigger a call to wacom_wac_pen_report(). This is undesired: only pen data should result in that function being called. Adding the check causes the function to return early for pad data while pen data falls into the "else if" and is processed as before. Pad data is only reported once the entire report has been valuated by making a call to wacom_wac_pad_report() at the very end of wacom_wac_report().

Jason Gerecke, Senior Linux Software Engineer
Wacom Technology Corporation
tel: 503-525-3100 ext. 3229 (direct)
http://www.wacom.com