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

[Pavel Machek]

Hi!

> > > 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().
>

Yep, you are right. I failed to notice the difference between _PAD_
and _PEN_ macros, and so the code did not make sense.

Sorry for the noise.

Best regards,
								Pavel
								
-- 
http://www.livejournal.com/~pavelmachek

Attachment: signature.asc
Description: PGP signature