[PATCH 1/2] hid: multitouch: add module paramter to override quirks

[PATCH 1/2] hid: multitouch: add module paramter to override quirks

[Tao Jin]

There is a sysfs interface to specify the quirks. However, some
quirk bits such as HID_QUIRK_MULTI_INPUT are only read once during
init/probe. Setting the quirks in sysfs does not make any change to the
behaviors related to those bits. Also, it is hard to wait for udev to
modify the quirks in case there is a rule given the current init and
state machine structure.

A simple kernel paramter is provided so that any time a custom quirk
needs to be tested can be easily applied. This enables the users to test
out which bits are indeed necessary with just a rmmod then
insmod/modprobe cycle. Thus, new hardware support can be added very
soon and unneccsary mildly costly quirks using mutex and timer can also
be removed based on user's self-test.

Co-Developed-by: Bingchen Gong <gongbingchen@gmail.com>
Signed-off-by: Bingchen Gong <gongbingchen@gmail.com>
Signed-off-by: Tao Jin <tao-j@outlook.com>
---
 drivers/hid/hid-multitouch.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 3ea57f3..c6d64f8 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -398,6 +398,10 @@ static const struct mt_class mt_classes[] = {
 	{ }
 };
 
+static int override_quirks = -1;
+module_param(override_quirks, int, 0444);
+MODULE_PARM_DESC(override_quirks, "Signed integer to override quirks in mtclass, must >= 0 to enable override.");
+
 static ssize_t mt_show_quirks(struct device *dev,
 			   struct device_attribute *attr,
 			   char *buf)
@@ -1749,7 +1753,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (id->group != HID_GROUP_MULTITOUCH_WIN_8)
 		hdev->quirks |= HID_QUIRK_MULTI_INPUT;
 
-	if (mtclass->quirks & MT_QUIRK_FORCE_MULTI_INPUT) {
+	if (override_quirks >= 0) {
+		hid_info(hdev, "overriding quirks with: %d(0x%x)", override_quirks, override_quirks);
+		td->mtclass.quirks = override_quirks;
+	}
+
+	if (td->mtclass.quirks & MT_QUIRK_FORCE_MULTI_INPUT) {
 		hdev->quirks &= ~HID_QUIRK_INPUT_PER_APP;
 		hdev->quirks |= HID_QUIRK_MULTI_INPUT;
	}
@@ -1760,7 +1769,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret != 0)
 		return ret;
 
-	if (mtclass->quirks & MT_QUIRK_FIX_CONST_CONTACT_ID)
+	if (td->mtclass.quirks & MT_QUIRK_FIX_CONST_CONTACT_ID)
 		mt_fix_const_fields(hdev, HID_DG_CONTACTID);
 
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
-- 
2.35.1

Re: [PATCH 1/2] hid: multitouch: add module paramter to override quirks

[Benjamin Tissoires]

Hi,

On Mon, Apr 18, 2022 at 5:18 AM Tao Jin <tao-j@outlook.com> wrote:
>
> There is a sysfs interface to specify the quirks. However, some
> quirk bits such as HID_QUIRK_MULTI_INPUT are only read once during
> init/probe. Setting the quirks in sysfs does not make any change to the
> behaviors related to those bits. Also, it is hard to wait for udev to
> modify the quirks in case there is a rule given the current init and
> state machine structure.
>
> A simple kernel paramter is provided so that any time a custom quirk
> needs to be tested can be easily applied. This enables the users to test
> out which bits are indeed necessary with just a rmmod then
> insmod/modprobe cycle. Thus, new hardware support can be added very
> soon and unneccsary mildly costly quirks using mutex and timer can also
> be removed based on user's self-test.

I don't think the last sentence above is adding anything. I don't see
where the mutex and timers are related to this patch.

>
> Co-Developed-by: Bingchen Gong <gongbingchen@gmail.com>
> Signed-off-by: Bingchen Gong <gongbingchen@gmail.com>
> Signed-off-by: Tao Jin <tao-j@outlook.com>
> ---
>  drivers/hid/hid-multitouch.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 3ea57f3..c6d64f8 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -398,6 +398,10 @@ static const struct mt_class mt_classes[] = {
>         { }
>  };
>
> +static int override_quirks = -1;
> +module_param(override_quirks, int, 0444);
> +MODULE_PARM_DESC(override_quirks, "Signed integer to override quirks in mtclass, must >= 0 to enable override.");
> +
>  static ssize_t mt_show_quirks(struct device *dev,
>                            struct device_attribute *attr,
>                            char *buf)
> @@ -1749,7 +1753,12 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         if (id->group != HID_GROUP_MULTITOUCH_WIN_8)
>                 hdev->quirks |= HID_QUIRK_MULTI_INPUT;
>
> -       if (mtclass->quirks & MT_QUIRK_FORCE_MULTI_INPUT) {
> +       if (override_quirks >= 0) {
> +               hid_info(hdev, "overriding quirks with: %d(0x%x)", override_quirks, override_quirks);
> +               td->mtclass.quirks = override_quirks;
> +       }
> +
> +       if (td->mtclass.quirks & MT_QUIRK_FORCE_MULTI_INPUT) {
>                 hdev->quirks &= ~HID_QUIRK_INPUT_PER_APP;
>                 hdev->quirks |= HID_QUIRK_MULTI_INPUT;
>         }
> @@ -1760,7 +1769,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>         if (ret != 0)
>                 return ret;
>
> -       if (mtclass->quirks & MT_QUIRK_FIX_CONST_CONTACT_ID)
> +       if (td->mtclass.quirks & MT_QUIRK_FIX_CONST_CONTACT_ID)

The 2 changes "s/mtclass->quirks/td->mtclass.quirks/" should probably
be added as a separate patch (before this one). The reason is that
hid_parse might change the quirks values, and
MT_QUIRK_FIX_CONST_CONTACT_ID and MT_QUIRK_FORCE_MULTI_INPUT might be
different from the class definition at the end of probe, so
definitively, this is a bug (without consequences) that should be
addressed separately.

Cheers,
Benjamin

>                 mt_fix_const_fields(hdev, HID_DG_CONTACTID);
>
>         ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> --
> 2.35.1
>