Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Gerecke, Jason" <Jason.Gerecke@wacom.com>
To:     Pavel Machek <pavel@ucw.cz>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jason Gerecke <killertofu@gmail.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        "Cheng, Ping" <Ping.Cheng@wacom.com>, Jiri Kosina <jkosina@suse.cz>
Subject: Re: [PATCH 4.19 146/191] HID: wacom: Avoid entering
 wacom_wac_pen_report for pad / battery
Thread-Topic: [PATCH 4.19 146/191] HID: wacom: Avoid entering
 wacom_wac_pen_report for pad / battery
Thread-Index: AQHWsiU8nYPLW7sITUmjvNop93TLtKm3mo+AgABsJTg=
Date:   Wed, 4 Nov 2020 14:40:38 +0000
Message-ID: <VI1PR07MB5821A87A8B0133C606C9911CEDEF0@VI1PR07MB5821.eurprd07.prod.outlook.com>
References: <20201103203232.656475008@linuxfoundation.org>
 <20201103203246.442871831@linuxfoundation.org>,<20201104075221.GA4338@amd>
In-Reply-To: <20201104075221.GA4338@amd>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 61abb1a0-671e-44ab-cf46-08d880cf98fc
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Nov 2020 14:40:38.4706
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 9655a91b-107e-4537-834e-d15e84872626
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: raEivxasx6iDhKxBNszYuDmpPJpO3BpnyHFBuiHBmxC+4dvmjCLYShGnim5BNHA1k+c7Yr3gsniZ8YmsuCaY+g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR07MB5488
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Pavel Machek <pavel@ucw.cz>=0A=
> Sent: Tuesday, November 3, 2020 11:52 PM=0A=
>=0A=
> > To correct this, we restore a version of the `WACOM_PAD_FIELD` check=0A=
> > in `wacom_wac_collection()` and return early. This effectively prevents=
=0A=
> > pad / battery collections from being reported until the very end of the=
=0A=
> > report as originally intended.=0A=
> =0A=
> Okay... but code is either wrong or very confusing:=0A=
> =0A=
> > +++ b/drivers/hid/wacom_wac.c=0A=
> > @@ -2729,7 +2729,9 @@ static int wacom_wac_collection(struct h=0A=
> >        if (report->type !=3D HID_INPUT_REPORT)=0A=
> >                return -1;=0A=
> >  =0A=
> > -     if (WACOM_PEN_FIELD(field) && wacom->wacom_wac.pen_input)=0A=
> > +     if (WACOM_PAD_FIELD(field))=0A=
> > +             return 0;=0A=
> > +     else if (WACOM_PEN_FIELD(field) && wacom->wacom_wac.pen_input)=0A=
> >                wacom_wac_pen_report(hdev, report);=0A=
> =0A=
> wacom_wac_pen_report() can never be called, because WACOM_PEN_FIELD()=0A=
> can not be true here; if it was we'd return in the line above.=0A=
=0A=
For reference, here's the definition for WACOM_PAD_FIELD() and WACOM_PEN_FI=
ELD():=0A=
=0A=
> #define WACOM_PAD_FIELD(f)	(((f)->physical =3D=3D HID_DG_TABLETFUNCTIONKE=
Y) || \=0A=
> 				 ((f)->physical =3D=3D WACOM_HID_WD_DIGITIZERFNKEYS) || \=0A=
> 				 ((f)->physical =3D=3D WACOM_HID_WD_DIGITIZERINFO))=0A=
> =0A=
> #define WACOM_PEN_FIELD(f)	(((f)->logical =3D=3D HID_DG_STYLUS) || \=0A=
> 				 ((f)->physical =3D=3D HID_DG_STYLUS) || \=0A=
> 				 ((f)->physical =3D=3D HID_DG_PEN) || \=0A=
> 				 ((f)->application =3D=3D HID_DG_PEN) || \=0A=
> 				 ((f)->application =3D=3D HID_DG_DIGITIZER) || \=0A=
> 				 ((f)->application =3D=3D WACOM_HID_WD_PEN) || \=0A=
> 				 ((f)->application =3D=3D WACOM_HID_WD_DIGITIZER) || \=0A=
> 				 ((f)->application =3D=3D WACOM_HID_G9_PEN) || \=0A=
> 				 ((f)->application =3D=3D WACOM_HID_G11_PEN))=0A=
=0A=
WACOM_PAD_FIELD() evaluates to `true` for pad data *not* pen data because p=
en data is not inside any of the 3 physical collections its looks for.=0A=
=0A=
WACOM_PEN_FIELD() evaluates to `true` for pad data *and* pen data because b=
oth types of data are inside of the Digitizer application collection.=0A=
=0A=
Without the WACOM_PAD_FIELD() check in place at the very beginning, both pa=
d and pen data would trigger a call to wacom_wac_pen_report(). This is unde=
sired: only pen data should result in that function being called. Adding th=
e check causes the function to return early for pad data while pen data fal=
ls 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().=0A=
=0A=
Jason Gerecke, Senior Linux Software Engineer=0A=
Wacom Technology Corporation=0A=
tel: 503-525-3100 ext. 3229 (direct)=0A=
http://www.wacom.com=