| Message ID | 20210216194154.111950-1-jason.gerecke@wacom.com |
|---|---|
| State | Accepted |
| Commit | 88f38846bfb1a452a3d47e38aeab20a4ceb74294 |
| Headers | show |
| Series | HID: wacom: Ignore attempts to overwrite the touch_max value from HID | expand |
On Tue, 16 Feb 2021, Jason Gerecke wrote: > The `wacom_feature_mapping` function is careful to only set the the > touch_max value a single time, but this care does not extend to the > `wacom_wac_finger_event` function. In particular, if a device sends > multiple HID_DG_CONTACTMAX items in a single feature report, the > driver will end up retaining the value of last item. > > The HID descriptor for the Cintiq Companion 2 does exactly this. It > incorrectly sets a "Report Count" of 2, which will cause the driver > to process two HID_DG_CONTACTCOUNT items. The first item has the actual > count, while the second item should have been declared as a constant > zero. The constant zero is the value the driver ends up using, however, > since it is the last HID_DG_CONTACTCOUNT in the report. > > Report ID (16), > Usage (Contact Count Maximum), ; Contact count maximum (55h, static value) > Report Count (2), > Logical Maximum (10), > Feature (Variable), > > To address this, we add a check that the touch_max is not already set > within the `wacom_wac_finger_event` function that processes the > HID_DG_TOUCHMAX item. We emit a warning if the value is set and ignore > the updated value. > > This could potentially cause problems if there is a tablet which has > a similar issue but requires the last item to be used. This is unlikely, > however, since it would have to have a different non-zero value for > HID_DG_CONTACTMAX earlier in the same report, which makes no sense > except in the case of a firmware bug. Note that cases where the > HID_DG_CONTACTMAX items are in different reports is already handled > (and similarly ignored) by `wacom_feature_mapping` as mentioned above. > > Link: https://github.com/linuxwacom/input-wacom/issues/223 > Fixes: 184eccd40389 ("HID: wacom: generic: read HID_DG_CONTACTMAX from any feature report") > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > CC: stable@vger.kernel.org > --- > drivers/hid/wacom_wac.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c > index 1bd0eb71559c..44d715c12f6a 100644 > --- a/drivers/hid/wacom_wac.c > +++ b/drivers/hid/wacom_wac.c > @@ -2600,7 +2600,12 @@ static void wacom_wac_finger_event(struct hid_device *hdev, > wacom_wac->is_invalid_bt_frame = !value; > return; > case HID_DG_CONTACTMAX: > - features->touch_max = value; > + if (!features->touch_max) { > + features->touch_max = value; > + } else { > + hid_warn(hdev, "%s: ignoring attempt to overwrite non-zero touch_max " > + "%d -> %d\n", __func__, features->touch_max, value); > + } > return; Applied, thanks Jason.
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c index 1bd0eb71559c..44d715c12f6a 100644 --- a/drivers/hid/wacom_wac.c +++ b/drivers/hid/wacom_wac.c @@ -2600,7 +2600,12 @@ static void wacom_wac_finger_event(struct hid_device *hdev, wacom_wac->is_invalid_bt_frame = !value; return; case HID_DG_CONTACTMAX: - features->touch_max = value; + if (!features->touch_max) { + features->touch_max = value; + } else { + hid_warn(hdev, "%s: ignoring attempt to overwrite non-zero touch_max " + "%d -> %d\n", __func__, features->touch_max, value); + } return; }
The `wacom_feature_mapping` function is careful to only set the the touch_max value a single time, but this care does not extend to the `wacom_wac_finger_event` function. In particular, if a device sends multiple HID_DG_CONTACTMAX items in a single feature report, the driver will end up retaining the value of last item. The HID descriptor for the Cintiq Companion 2 does exactly this. It incorrectly sets a "Report Count" of 2, which will cause the driver to process two HID_DG_CONTACTCOUNT items. The first item has the actual count, while the second item should have been declared as a constant zero. The constant zero is the value the driver ends up using, however, since it is the last HID_DG_CONTACTCOUNT in the report. Report ID (16), Usage (Contact Count Maximum), ; Contact count maximum (55h, static value) Report Count (2), Logical Maximum (10), Feature (Variable), To address this, we add a check that the touch_max is not already set within the `wacom_wac_finger_event` function that processes the HID_DG_TOUCHMAX item. We emit a warning if the value is set and ignore the updated value. This could potentially cause problems if there is a tablet which has a similar issue but requires the last item to be used. This is unlikely, however, since it would have to have a different non-zero value for HID_DG_CONTACTMAX earlier in the same report, which makes no sense except in the case of a firmware bug. Note that cases where the HID_DG_CONTACTMAX items are in different reports is already handled (and similarly ignored) by `wacom_feature_mapping` as mentioned above. Link: https://github.com/linuxwacom/input-wacom/issues/223 Fixes: 184eccd40389 ("HID: wacom: generic: read HID_DG_CONTACTMAX from any feature report") Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> CC: stable@vger.kernel.org --- drivers/hid/wacom_wac.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)