[PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Hans de Goede]

The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
correctly binds to it.

But instead of getting multi-touch HID input reports we still get mouse
input reports and corresponding linux input (evdev) node events.

Unloading and reloading the hid-multitouch driver works around this.

Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
makes the driver work correctly the first time it is loaded.

I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
because it seems unlikely that this quirk will causes problems for
other MT_CLS_WIN_8 devices and if this device needs it other Win 8
compatible devices might need it too.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-multitouch.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 362805ddf377..f9c0429e7348 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -265,6 +265,7 @@ static const struct mt_class mt_classes[] = {
 			MT_QUIRK_IGNORE_DUPLICATES |
 			MT_QUIRK_HOVERING |
 			MT_QUIRK_CONTACT_CNT_ACCURATE |
+			MT_QUIRK_FORCE_GET_FEATURE |
 			MT_QUIRK_STICKY_FINGERS |
 			MT_QUIRK_WIN8_PTP_BUTTONS,
 		.export_all_inputs = true },
-- 
2.26.0

Re: [PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Hans de Goede]

Hi,

On 5/1/20 11:56 AM, Hans de Goede wrote:
> The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
> capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
> correctly binds to it.
> 
> But instead of getting multi-touch HID input reports we still get mouse
> input reports and corresponding linux input (evdev) node events.
> 
> Unloading and reloading the hid-multitouch driver works around this.
> 
> Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
> makes the driver work correctly the first time it is loaded.
> 
> I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
> because it seems unlikely that this quirk will causes problems for
> other MT_CLS_WIN_8 devices and if this device needs it other Win 8
> compatible devices might need it too.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Self nack for now, there are more issues with this detachable keyboard,
it sometimes does not work after being unplugged and replugged again
USB_QUIRK_DELAY_INIT seems to help a bit, but is not a total solution...

Dell has some firmware updates for the kbd. So I'll install Windows and
then update the firmware and we'll see from there.

Regards,

Hans



> ---
>   drivers/hid/hid-multitouch.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 362805ddf377..f9c0429e7348 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -265,6 +265,7 @@ static const struct mt_class mt_classes[] = {
>   			MT_QUIRK_IGNORE_DUPLICATES |
>   			MT_QUIRK_HOVERING |
>   			MT_QUIRK_CONTACT_CNT_ACCURATE |
> +			MT_QUIRK_FORCE_GET_FEATURE |
>   			MT_QUIRK_STICKY_FINGERS |
>   			MT_QUIRK_WIN8_PTP_BUTTONS,
>   		.export_all_inputs = true },
> 

Re: [PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Hans de Goede]

Hi,

On 5/1/20 8:20 PM, Hans de Goede wrote:
> Hi,
> 
> On 5/1/20 11:56 AM, Hans de Goede wrote:
>> The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
>> capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
>> correctly binds to it.
>>
>> But instead of getting multi-touch HID input reports we still get mouse
>> input reports and corresponding linux input (evdev) node events.
>>
>> Unloading and reloading the hid-multitouch driver works around this.
>>
>> Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
>> makes the driver work correctly the first time it is loaded.
>>
>> I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
>> because it seems unlikely that this quirk will causes problems for
>> other MT_CLS_WIN_8 devices and if this device needs it other Win 8
>> compatible devices might need it too.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Self nack for now, there are more issues with this detachable keyboard,
> it sometimes does not work after being unplugged and replugged again
> USB_QUIRK_DELAY_INIT seems to help a bit, but is not a total solution...
> 
> Dell has some firmware updates for the kbd. So I'll install Windows and
> then update the firmware and we'll see from there.

So after installing Windows it turns out that the kbd-dock firmware was
already fully up2date, what fun.

So it took me quite a long time to get to the bottom of this.

The problem is that the Dell K12A kbd-dock needs a HID_QUIRK_NO_INIT_REPORTS
quirk; or maybe both of HID_QUIRK_NO_INIT_REPORTS|HID_QUIRK_NOGET I've tested
with the later version and that fixes both the touchpad initially being
stuck in mouse emulation and the dock misbehaving after a hot unplug + replug.

I suspect I really only need HID_QUIRK_NO_INIT_REPORTS, I will retest with
that and submit a new patch replacing this one.

Somewhat related: to make space for the Windows install I nuked the old
Fedora 27 install which was on the machine and after installing Windows
I did a fresh Fedora 32 install in the space which I left free when
installing Windows.

This causes an interesting new problem. The touchpad worked fine
(with the quirk) in gdm, but it would stop working when I logged into
a user GNOME-session.  It took me a while to get to the bottom of
this. The problem is that the usersession ends up dbus activating
fwupd (probably through gnome-software) and fwupd does some probe
of the touchpad which puts it in a mode where it no longer generates
any events.

sudo rpm -e fwupd gnome-software

Works around this, so not a HID bug, but definitely something to keep
an eye out for if we get similar bug reports on other devices.

I will mail the fwupd maintainer about this with you in the Cc.
Note this is an unrelated issue really, but I thought you
should be aware of this.

Regards,

Hans



>> ---
>>   drivers/hid/hid-multitouch.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 362805ddf377..f9c0429e7348 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -265,6 +265,7 @@ static const struct mt_class mt_classes[] = {
>>               MT_QUIRK_IGNORE_DUPLICATES |
>>               MT_QUIRK_HOVERING |
>>               MT_QUIRK_CONTACT_CNT_ACCURATE |
>> +            MT_QUIRK_FORCE_GET_FEATURE |
>>               MT_QUIRK_STICKY_FINGERS |
>>               MT_QUIRK_WIN8_PTP_BUTTONS,
>>           .export_all_inputs = true },
>>

Re: [PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Benjamin Tissoires]

Hi Hans,

On Sat, May 2, 2020 at 2:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 5/1/20 8:20 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 5/1/20 11:56 AM, Hans de Goede wrote:
> >> The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
> >> capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
> >> correctly binds to it.
> >>
> >> But instead of getting multi-touch HID input reports we still get mouse
> >> input reports and corresponding linux input (evdev) node events.
> >>
> >> Unloading and reloading the hid-multitouch driver works around this.
> >>
> >> Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
> >> makes the driver work correctly the first time it is loaded.
> >>
> >> I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
> >> because it seems unlikely that this quirk will causes problems for
> >> other MT_CLS_WIN_8 devices and if this device needs it other Win 8
> >> compatible devices might need it too.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > Self nack for now, there are more issues with this detachable keyboard,
> > it sometimes does not work after being unplugged and replugged again
> > USB_QUIRK_DELAY_INIT seems to help a bit, but is not a total solution...
> >
> > Dell has some firmware updates for the kbd. So I'll install Windows and
> > then update the firmware and we'll see from there.
>
> So after installing Windows it turns out that the kbd-dock firmware was
> already fully up2date, what fun.
>
> So it took me quite a long time to get to the bottom of this.
>
> The problem is that the Dell K12A kbd-dock needs a HID_QUIRK_NO_INIT_REPORTS
> quirk; or maybe both of HID_QUIRK_NO_INIT_REPORTS|HID_QUIRK_NOGET I've tested

I think this is a regression introduced by the high res scrolling
patch. I have been notified that the new code actually does fetch all
features on connect, which many devices do not support.

I don't think I received the patch related to that, but basically the
problematic code is at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n1558

The issue is that we should only fetch the current report if the
HID_GD_RESOLUTION_MULTIPLIER is present. Or we break things.

Cheers,
Benjamin

> with the later version and that fixes both the touchpad initially being
> stuck in mouse emulation and the dock misbehaving after a hot unplug + replug.
>
> I suspect I really only need HID_QUIRK_NO_INIT_REPORTS, I will retest with
> that and submit a new patch replacing this one.
>
> Somewhat related: to make space for the Windows install I nuked the old
> Fedora 27 install which was on the machine and after installing Windows
> I did a fresh Fedora 32 install in the space which I left free when
> installing Windows.
>
> This causes an interesting new problem. The touchpad worked fine
> (with the quirk) in gdm, but it would stop working when I logged into
> a user GNOME-session.  It took me a while to get to the bottom of
> this. The problem is that the usersession ends up dbus activating
> fwupd (probably through gnome-software) and fwupd does some probe
> of the touchpad which puts it in a mode where it no longer generates
> any events.
>
> sudo rpm -e fwupd gnome-software
>
> Works around this, so not a HID bug, but definitely something to keep
> an eye out for if we get similar bug reports on other devices.
>
> I will mail the fwupd maintainer about this with you in the Cc.
> Note this is an unrelated issue really, but I thought you
> should be aware of this.
>
> Regards,
>
> Hans
>
>
>
> >> ---
> >>   drivers/hid/hid-multitouch.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> >> index 362805ddf377..f9c0429e7348 100644
> >> --- a/drivers/hid/hid-multitouch.c
> >> +++ b/drivers/hid/hid-multitouch.c
> >> @@ -265,6 +265,7 @@ static const struct mt_class mt_classes[] = {
> >>               MT_QUIRK_IGNORE_DUPLICATES |
> >>               MT_QUIRK_HOVERING |
> >>               MT_QUIRK_CONTACT_CNT_ACCURATE |
> >> +            MT_QUIRK_FORCE_GET_FEATURE |
> >>               MT_QUIRK_STICKY_FINGERS |
> >>               MT_QUIRK_WIN8_PTP_BUTTONS,
> >>           .export_all_inputs = true },
> >>
>

Re: [PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Hans de Goede]

Hi,

On 5/4/20 9:39 AM, Benjamin Tissoires wrote:
> Hi Hans,
> 
> On Sat, May 2, 2020 at 2:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 5/1/20 8:20 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 5/1/20 11:56 AM, Hans de Goede wrote:
>>>> The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
>>>> capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
>>>> correctly binds to it.
>>>>
>>>> But instead of getting multi-touch HID input reports we still get mouse
>>>> input reports and corresponding linux input (evdev) node events.
>>>>
>>>> Unloading and reloading the hid-multitouch driver works around this.
>>>>
>>>> Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
>>>> makes the driver work correctly the first time it is loaded.
>>>>
>>>> I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
>>>> because it seems unlikely that this quirk will causes problems for
>>>> other MT_CLS_WIN_8 devices and if this device needs it other Win 8
>>>> compatible devices might need it too.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Self nack for now, there are more issues with this detachable keyboard,
>>> it sometimes does not work after being unplugged and replugged again
>>> USB_QUIRK_DELAY_INIT seems to help a bit, but is not a total solution...
>>>
>>> Dell has some firmware updates for the kbd. So I'll install Windows and
>>> then update the firmware and we'll see from there.
>>
>> So after installing Windows it turns out that the kbd-dock firmware was
>> already fully up2date, what fun.
>>
>> So it took me quite a long time to get to the bottom of this.
>>
>> The problem is that the Dell K12A kbd-dock needs a HID_QUIRK_NO_INIT_REPORTS
>> quirk; or maybe both of HID_QUIRK_NO_INIT_REPORTS|HID_QUIRK_NOGET I've tested
> 
> I think this is a regression introduced by the high res scrolling
> patch. I have been notified that the new code actually does fetch all
> features on connect, which many devices do not support.
> 
> I don't think I received the patch related to that, but basically the
> problematic code is at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n1558
> 
> The issue is that we should only fetch the current report if the
> HID_GD_RESOLUTION_MULTIPLIER is present. Or we break things.

I don't think that this is related to the high-res scrolling stuff.

The errors I'm seeing on a bad hotplug are coming from
drivers/hid/hid-multitouch.c: mt_get_feature()

Also quite a few other multi-touch devices have a HID_QUIRK_NO_INIT_REPORTS
quirk, at least a bunch of surface keyboards do; and if I'm reading the
git log correctly then at one point in time we used to have a
HID_QUIRK_NO_INIT_REPORTS for at least some of the multi-touch classes
inside hid-multitouch.c. At least there is a commit titled:

"HID: multitouch: do not set HID_QUIRK_NO_INIT_REPORTS"

Which suggests that one point we did set it inside the multi-touch
driver; and I'm wondering since a bunch of surface keyboards need this
if setting this inside the multi-touch driver would not get us closer
to windows behavior.

Anyways if you have an alternative fix you want me to test, let me know.

Regards,

Hans




> 
> Cheers,
> Benjamin
> 
>> with the later version and that fixes both the touchpad initially being
>> stuck in mouse emulation and the dock misbehaving after a hot unplug + replug.
>>
>> I suspect I really only need HID_QUIRK_NO_INIT_REPORTS, I will retest with
>> that and submit a new patch replacing this one.
>>
>> Somewhat related: to make space for the Windows install I nuked the old
>> Fedora 27 install which was on the machine and after installing Windows
>> I did a fresh Fedora 32 install in the space which I left free when
>> installing Windows.
>>
>> This causes an interesting new problem. The touchpad worked fine
>> (with the quirk) in gdm, but it would stop working when I logged into
>> a user GNOME-session.  It took me a while to get to the bottom of
>> this. The problem is that the usersession ends up dbus activating
>> fwupd (probably through gnome-software) and fwupd does some probe
>> of the touchpad which puts it in a mode where it no longer generates
>> any events.
>>
>> sudo rpm -e fwupd gnome-software
>>
>> Works around this, so not a HID bug, but definitely something to keep
>> an eye out for if we get similar bug reports on other devices.
>>
>> I will mail the fwupd maintainer about this with you in the Cc.
>> Note this is an unrelated issue really, but I thought you
>> should be aware of this.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>> ---
>>>>    drivers/hid/hid-multitouch.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>>>> index 362805ddf377..f9c0429e7348 100644
>>>> --- a/drivers/hid/hid-multitouch.c
>>>> +++ b/drivers/hid/hid-multitouch.c
>>>> @@ -265,6 +265,7 @@ static const struct mt_class mt_classes[] = {
>>>>                MT_QUIRK_IGNORE_DUPLICATES |
>>>>                MT_QUIRK_HOVERING |
>>>>                MT_QUIRK_CONTACT_CNT_ACCURATE |
>>>> +            MT_QUIRK_FORCE_GET_FEATURE |
>>>>                MT_QUIRK_STICKY_FINGERS |
>>>>                MT_QUIRK_WIN8_PTP_BUTTONS,
>>>>            .export_all_inputs = true },
>>>>
>>
> 

Re: [PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Benjamin Tissoires]

Hi Hans,

[sorry for the delay, I am knee deep in fdo admin stuffs]

On Mon, May 4, 2020 at 11:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 5/4/20 9:39 AM, Benjamin Tissoires wrote:
> > Hi Hans,
> >
> > On Sat, May 2, 2020 at 2:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 5/1/20 8:20 PM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 5/1/20 11:56 AM, Hans de Goede wrote:
> >>>> The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
> >>>> capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
> >>>> correctly binds to it.
> >>>>
> >>>> But instead of getting multi-touch HID input reports we still get mouse
> >>>> input reports and corresponding linux input (evdev) node events.
> >>>>
> >>>> Unloading and reloading the hid-multitouch driver works around this.
> >>>>
> >>>> Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
> >>>> makes the driver work correctly the first time it is loaded.
> >>>>
> >>>> I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
> >>>> because it seems unlikely that this quirk will causes problems for
> >>>> other MT_CLS_WIN_8 devices and if this device needs it other Win 8
> >>>> compatible devices might need it too.
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>
> >>> Self nack for now, there are more issues with this detachable keyboard,
> >>> it sometimes does not work after being unplugged and replugged again
> >>> USB_QUIRK_DELAY_INIT seems to help a bit, but is not a total solution...
> >>>
> >>> Dell has some firmware updates for the kbd. So I'll install Windows and
> >>> then update the firmware and we'll see from there.
> >>
> >> So after installing Windows it turns out that the kbd-dock firmware was
> >> already fully up2date, what fun.
> >>
> >> So it took me quite a long time to get to the bottom of this.
> >>
> >> The problem is that the Dell K12A kbd-dock needs a HID_QUIRK_NO_INIT_REPORTS
> >> quirk; or maybe both of HID_QUIRK_NO_INIT_REPORTS|HID_QUIRK_NOGET I've tested
> >
> > I think this is a regression introduced by the high res scrolling
> > patch. I have been notified that the new code actually does fetch all
> > features on connect, which many devices do not support.
> >
> > I don't think I received the patch related to that, but basically the
> > problematic code is at
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n1558
> >
> > The issue is that we should only fetch the current report if the
> > HID_GD_RESOLUTION_MULTIPLIER is present. Or we break things.
>
> I don't think that this is related to the high-res scrolling stuff.

Well, it is in the way that the high-res scrolling changed the way we talked to HID device. Before that, I carefully ensured that hid-generic would never issue a get_feature or get_input, and after, it does.

>
> The errors I'm seeing on a bad hotplug are coming from
> drivers/hid/hid-multitouch.c: mt_get_feature()

It might be that the device doesn't like to be polled too often on one feature and gets in a stuck mode.

>
> Also quite a few other multi-touch devices have a HID_QUIRK_NO_INIT_REPORTS
> quirk, at least a bunch of surface keyboards do; and if I'm reading the
> git log correctly then at one point in time we used to have a
> HID_QUIRK_NO_INIT_REPORTS for at least some of the multi-touch classes
> inside hid-multitouch.c. At least there is a commit titled:
>
> "HID: multitouch: do not set HID_QUIRK_NO_INIT_REPORTS"
>
> Which suggests that one point we did set it inside the multi-touch
> driver; and I'm wondering since a bunch of surface keyboards need this
> if setting this inside the multi-touch driver would not get us closer
> to windows behavior.

This quirk is legacy, and should have stayed that way (well, it doesn't
hurt anyway). As mentioned, in the past, the hid core stack used to fetch
all input and feature reports on plug in. This was known to break many
devices, and we had to use the no-init-report quirk for them. Then, we
realized that it was not correct to do that way, and I removed this
behavior. However, I couldn't remove the quirk entirely because of hiddev
IIRC. In the hiddev case, userspace expects the device to have known values
for the features, but that is not 100% working. So to not break userspace,
I had to keep that quirk around for this use case.

>
> Anyways if you have an alternative fix you want me to test, let me know.

Peter has a patch, we were waiting for the reporter to test it, but it's
been crickets since last week.

Here it is:

---
Subject: [DRAFT PATCH] HID: input: do not run GET_REPORT unless there's a Resolution Multiplier

From: Peter Hutterer <peter.hutterer@who-t.net>

Some devices take GET_REPORT as an instruction to change the mode, or
toggle some other value, or possibly do unspeakable things to kittens.
So we must not execute GET_REPORT against a device unless we're sure
that there's a Resolution Multiplier in that report that makes it all
worth it.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 drivers/hid/hid-input.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git drivers/hid/hid-input.c drivers/hid/hid-input.c
index dea9cc65bf80..a54824d451bf 100644
--- drivers/hid/hid-input.c
+++ drivers/hid/hid-input.c
@@ -1560,21 +1560,12 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
 {
 	struct hid_usage *usage;
 	bool update_needed = false;
+	bool get_report_completed = false;
 	int i, j;
 
 	if (report->maxfield == 0)
 		return false;
 
-	/*
-	 * If we have more than one feature within this report we
-	 * need to fill in the bits from the others before we can
-	 * overwrite the ones for the Resolution Multiplier.
-	 */
-	if (report->maxfield > 1) {
-		hid_hw_request(hid, report, HID_REQ_GET_REPORT);
-		hid_hw_wait(hid);
-	}
-
 	for (i = 0; i < report->maxfield; i++) {
 		__s32 value = use_logical_max ?
 			      report->field[i]->logical_maximum :
@@ -1593,6 +1584,17 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
 			if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
 				continue;
 
+			/*
+			 * If we have more than one feature within this report we
+			 * need to fill in the bits from the others before we can
+			 * overwrite the ones for the Resolution Multiplier.
+			 */
+			if (!get_report_completed && report->maxfield > 1) {
+				hid_hw_request(hid, report, HID_REQ_GET_REPORT);
+				hid_hw_wait(hid);
+				get_report_completed = true;
+			}
+
 			report->field[i]->value[j] = value;
 			update_needed = true;
 		}
-- 
2.26.0

Cheers,
Benjamin

>
> Regards,
>
> Hans
>
>
>
>
> >
> > Cheers,
> > Benjamin
> >
> >> with the later version and that fixes both the touchpad initially being
> >> stuck in mouse emulation and the dock misbehaving after a hot unplug + replug.
> >>
> >> I suspect I really only need HID_QUIRK_NO_INIT_REPORTS, I will retest with
> >> that and submit a new patch replacing this one.
> >>
> >> Somewhat related: to make space for the Windows install I nuked the old
> >> Fedora 27 install which was on the machine and after installing Windows
> >> I did a fresh Fedora 32 install in the space which I left free when
> >> installing Windows.
> >>
> >> This causes an interesting new problem. The touchpad worked fine
> >> (with the quirk) in gdm, but it would stop working when I logged into
> >> a user GNOME-session.  It took me a while to get to the bottom of
> >> this. The problem is that the usersession ends up dbus activating
> >> fwupd (probably through gnome-software) and fwupd does some probe
> >> of the touchpad which puts it in a mode where it no longer generates
> >> any events.
> >>
> >> sudo rpm -e fwupd gnome-software
> >>
> >> Works around this, so not a HID bug, but definitely something to keep
> >> an eye out for if we get similar bug reports on other devices.
> >>
> >> I will mail the fwupd maintainer about this with you in the Cc.
> >> Note this is an unrelated issue really, but I thought you
> >> should be aware of this.
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>>> ---
> >>>>    drivers/hid/hid-multitouch.c | 1 +
> >>>>    1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> >>>> index 362805ddf377..f9c0429e7348 100644
> >>>> --- a/drivers/hid/hid-multitouch.c
> >>>> +++ b/drivers/hid/hid-multitouch.c
> >>>> @@ -265,6 +265,7 @@ static const struct mt_class mt_classes[] = {
> >>>>                MT_QUIRK_IGNORE_DUPLICATES |
> >>>>                MT_QUIRK_HOVERING |
> >>>>                MT_QUIRK_CONTACT_CNT_ACCURATE |
> >>>> +            MT_QUIRK_FORCE_GET_FEATURE |
> >>>>                MT_QUIRK_STICKY_FINGERS |
> >>>>                MT_QUIRK_WIN8_PTP_BUTTONS,
> >>>>            .export_all_inputs = true },
> >>>>
> >>
> >
>

Re: [PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Peter Hutterer]

On Wed, May 13, 2020 at 10:00:30AM +0200, Benjamin Tissoires wrote:
> Hi Hans,
> 
> [sorry for the delay, I am knee deep in fdo admin stuffs]
> 
> On Mon, May 4, 2020 at 11:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 5/4/20 9:39 AM, Benjamin Tissoires wrote:
> > > Hi Hans,
> > >
> > > On Sat, May 2, 2020 at 2:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > >>
> > >> Hi,
> > >>
> > >> On 5/1/20 8:20 PM, Hans de Goede wrote:
> > >>> Hi,
> > >>>
> > >>> On 5/1/20 11:56 AM, Hans de Goede wrote:
> > >>>> The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
> > >>>> capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
> > >>>> correctly binds to it.
> > >>>>
> > >>>> But instead of getting multi-touch HID input reports we still get mouse
> > >>>> input reports and corresponding linux input (evdev) node events.
> > >>>>
> > >>>> Unloading and reloading the hid-multitouch driver works around this.
> > >>>>
> > >>>> Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
> > >>>> makes the driver work correctly the first time it is loaded.
> > >>>>
> > >>>> I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
> > >>>> because it seems unlikely that this quirk will causes problems for
> > >>>> other MT_CLS_WIN_8 devices and if this device needs it other Win 8
> > >>>> compatible devices might need it too.
> > >>>>
> > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >>>
> > >>> Self nack for now, there are more issues with this detachable keyboard,
> > >>> it sometimes does not work after being unplugged and replugged again
> > >>> USB_QUIRK_DELAY_INIT seems to help a bit, but is not a total solution...
> > >>>
> > >>> Dell has some firmware updates for the kbd. So I'll install Windows and
> > >>> then update the firmware and we'll see from there.
> > >>
> > >> So after installing Windows it turns out that the kbd-dock firmware was
> > >> already fully up2date, what fun.
> > >>
> > >> So it took me quite a long time to get to the bottom of this.
> > >>
> > >> The problem is that the Dell K12A kbd-dock needs a HID_QUIRK_NO_INIT_REPORTS
> > >> quirk; or maybe both of HID_QUIRK_NO_INIT_REPORTS|HID_QUIRK_NOGET I've tested
> > >
> > > I think this is a regression introduced by the high res scrolling
> > > patch. I have been notified that the new code actually does fetch all
> > > features on connect, which many devices do not support.
> > >
> > > I don't think I received the patch related to that, but basically the
> > > problematic code is at
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n1558
> > >
> > > The issue is that we should only fetch the current report if the
> > > HID_GD_RESOLUTION_MULTIPLIER is present. Or we break things.
> >
> > I don't think that this is related to the high-res scrolling stuff.
> 
> Well, it is in the way that the high-res scrolling changed the way we talked to HID device. Before that, I carefully ensured that hid-generic would never issue a get_feature or get_input, and after, it does.
> 
> >
> > The errors I'm seeing on a bad hotplug are coming from
> > drivers/hid/hid-multitouch.c: mt_get_feature()
> 
> It might be that the device doesn't like to be polled too often on one feature and gets in a stuck mode.
> 
> >
> > Also quite a few other multi-touch devices have a HID_QUIRK_NO_INIT_REPORTS
> > quirk, at least a bunch of surface keyboards do; and if I'm reading the
> > git log correctly then at one point in time we used to have a
> > HID_QUIRK_NO_INIT_REPORTS for at least some of the multi-touch classes
> > inside hid-multitouch.c. At least there is a commit titled:
> >
> > "HID: multitouch: do not set HID_QUIRK_NO_INIT_REPORTS"
> >
> > Which suggests that one point we did set it inside the multi-touch
> > driver; and I'm wondering since a bunch of surface keyboards need this
> > if setting this inside the multi-touch driver would not get us closer
> > to windows behavior.
> 
> This quirk is legacy, and should have stayed that way (well, it doesn't
> hurt anyway). As mentioned, in the past, the hid core stack used to fetch
> all input and feature reports on plug in. This was known to break many
> devices, and we had to use the no-init-report quirk for them. Then, we
> realized that it was not correct to do that way, and I removed this
> behavior. However, I couldn't remove the quirk entirely because of hiddev
> IIRC. In the hiddev case, userspace expects the device to have known values
> for the features, but that is not 100% working. So to not break userspace,
> I had to keep that quirk around for this use case.
> 
> >
> > Anyways if you have an alternative fix you want me to test, let me know.
> 
> Peter has a patch, we were waiting for the reporter to test it, but it's
> been crickets since last week.

sorry, my fault. I have the tested-by from the reporter but I haven't yet
been able to find the time to verify it doesn't break the resolution
multiplier. so it lack's my own test.

Cheers,
   Peter

> 
> Here it is:
> 
> ---
> Subject: [DRAFT PATCH] HID: input: do not run GET_REPORT unless there's a Resolution Multiplier
> 
> From: Peter Hutterer <peter.hutterer@who-t.net>
> 
> Some devices take GET_REPORT as an instruction to change the mode, or
> toggle some other value, or possibly do unspeakable things to kittens.
> So we must not execute GET_REPORT against a device unless we're sure
> that there's a Resolution Multiplier in that report that makes it all
> worth it.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
>  drivers/hid/hid-input.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git drivers/hid/hid-input.c drivers/hid/hid-input.c
> index dea9cc65bf80..a54824d451bf 100644
> --- drivers/hid/hid-input.c
> +++ drivers/hid/hid-input.c
> @@ -1560,21 +1560,12 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
>  {
>  	struct hid_usage *usage;
>  	bool update_needed = false;
> +	bool get_report_completed = false;
>  	int i, j;
>  
>  	if (report->maxfield == 0)
>  		return false;
>  
> -	/*
> -	 * If we have more than one feature within this report we
> -	 * need to fill in the bits from the others before we can
> -	 * overwrite the ones for the Resolution Multiplier.
> -	 */
> -	if (report->maxfield > 1) {
> -		hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> -		hid_hw_wait(hid);
> -	}
> -
>  	for (i = 0; i < report->maxfield; i++) {
>  		__s32 value = use_logical_max ?
>  			      report->field[i]->logical_maximum :
> @@ -1593,6 +1584,17 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
>  			if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
>  				continue;
>  
> +			/*
> +			 * If we have more than one feature within this report we
> +			 * need to fill in the bits from the others before we can
> +			 * overwrite the ones for the Resolution Multiplier.
> +			 */
> +			if (!get_report_completed && report->maxfield > 1) {
> +				hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> +				hid_hw_wait(hid);
> +				get_report_completed = true;
> +			}
> +
>  			report->field[i]->value[j] = value;
>  			update_needed = true;
>  		}
> -- 
> 2.26.0
> 
> Cheers,
> Benjamin
> 
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> > >
> > > Cheers,
> > > Benjamin
> > >
> > >> with the later version and that fixes both the touchpad initially being
> > >> stuck in mouse emulation and the dock misbehaving after a hot unplug + replug.
> > >>
> > >> I suspect I really only need HID_QUIRK_NO_INIT_REPORTS, I will retest with
> > >> that and submit a new patch replacing this one.
> > >>
> > >> Somewhat related: to make space for the Windows install I nuked the old
> > >> Fedora 27 install which was on the machine and after installing Windows
> > >> I did a fresh Fedora 32 install in the space which I left free when
> > >> installing Windows.
> > >>
> > >> This causes an interesting new problem. The touchpad worked fine
> > >> (with the quirk) in gdm, but it would stop working when I logged into
> > >> a user GNOME-session.  It took me a while to get to the bottom of
> > >> this. The problem is that the usersession ends up dbus activating
> > >> fwupd (probably through gnome-software) and fwupd does some probe
> > >> of the touchpad which puts it in a mode where it no longer generates
> > >> any events.
> > >>
> > >> sudo rpm -e fwupd gnome-software
> > >>
> > >> Works around this, so not a HID bug, but definitely something to keep
> > >> an eye out for if we get similar bug reports on other devices.
> > >>
> > >> I will mail the fwupd maintainer about this with you in the Cc.
> > >> Note this is an unrelated issue really, but I thought you
> > >> should be aware of this.
> > >>
> > >> Regards,
> > >>
> > >> Hans
> > >>
> > >>
> > >>
> > >>>> ---
> > >>>>    drivers/hid/hid-multitouch.c | 1 +
> > >>>>    1 file changed, 1 insertion(+)
> > >>>>
> > >>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > >>>> index 362805ddf377..f9c0429e7348 100644
> > >>>> --- a/drivers/hid/hid-multitouch.c
> > >>>> +++ b/drivers/hid/hid-multitouch.c
> > >>>> @@ -265,6 +265,7 @@ static const struct mt_class mt_classes[] = {
> > >>>>                MT_QUIRK_IGNORE_DUPLICATES |
> > >>>>                MT_QUIRK_HOVERING |
> > >>>>                MT_QUIRK_CONTACT_CNT_ACCURATE |
> > >>>> +            MT_QUIRK_FORCE_GET_FEATURE |
> > >>>>                MT_QUIRK_STICKY_FINGERS |
> > >>>>                MT_QUIRK_WIN8_PTP_BUTTONS,
> > >>>>            .export_all_inputs = true },
> > >>>>
> > >>
> > >
> >
> 

Re: [PATCH] HID: multitouch: Add MT_QUIRK_FORCE_GET_FEATURE to MT_CLS_WIN_8 quirks

[Hans de Goede]

Hi,

On 5/13/20 10:00 AM, Benjamin Tissoires wrote:
> Hi Hans,
> 
> [sorry for the delay, I am knee deep in fdo admin stuffs]
> 
> On Mon, May 4, 2020 at 11:31 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 5/4/20 9:39 AM, Benjamin Tissoires wrote:
>>> Hi Hans,
>>>
>>> On Sat, May 2, 2020 at 2:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 5/1/20 8:20 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 5/1/20 11:56 AM, Hans de Goede wrote:
>>>>>> The touchpad on the Dell Venue 11 Pro 7130's keyboard-dock is multi-touch
>>>>>> capable, using HID_GROUP_MULTITOUCH_WIN_8 and the hid-multitouch driver
>>>>>> correctly binds to it.
>>>>>>
>>>>>> But instead of getting multi-touch HID input reports we still get mouse
>>>>>> input reports and corresponding linux input (evdev) node events.
>>>>>>
>>>>>> Unloading and reloading the hid-multitouch driver works around this.
>>>>>>
>>>>>> Adding the MT_QUIRK_FORCE_GET_FEATURE quirk to the MT_CLS_WIN_8 quirks
>>>>>> makes the driver work correctly the first time it is loaded.
>>>>>>
>>>>>> I've chosen to add this quirk to the generic MT_CLS_WIN_8 quirks
>>>>>> because it seems unlikely that this quirk will causes problems for
>>>>>> other MT_CLS_WIN_8 devices and if this device needs it other Win 8
>>>>>> compatible devices might need it too.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>
>>>>> Self nack for now, there are more issues with this detachable keyboard,
>>>>> it sometimes does not work after being unplugged and replugged again
>>>>> USB_QUIRK_DELAY_INIT seems to help a bit, but is not a total solution...
>>>>>
>>>>> Dell has some firmware updates for the kbd. So I'll install Windows and
>>>>> then update the firmware and we'll see from there.
>>>>
>>>> So after installing Windows it turns out that the kbd-dock firmware was
>>>> already fully up2date, what fun.
>>>>
>>>> So it took me quite a long time to get to the bottom of this.
>>>>
>>>> The problem is that the Dell K12A kbd-dock needs a HID_QUIRK_NO_INIT_REPORTS
>>>> quirk; or maybe both of HID_QUIRK_NO_INIT_REPORTS|HID_QUIRK_NOGET I've tested
>>>
>>> I think this is a regression introduced by the high res scrolling
>>> patch. I have been notified that the new code actually does fetch all
>>> features on connect, which many devices do not support.
>>>
>>> I don't think I received the patch related to that, but basically the
>>> problematic code is at
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n1558
>>>
>>> The issue is that we should only fetch the current report if the
>>> HID_GD_RESOLUTION_MULTIPLIER is present. Or we break things.
>>
>> I don't think that this is related to the high-res scrolling stuff.
> 
> Well, it is in the way that the high-res scrolling changed the way we talked to HID device. Before that, I carefully ensured that hid-generic would never issue a get_feature or get_input, and after, it does.
> 
>>
>> The errors I'm seeing on a bad hotplug are coming from
>> drivers/hid/hid-multitouch.c: mt_get_feature()
> 
> It might be that the device doesn't like to be polled too often on one feature and gets in a stuck mode.

I don't think so. I believe the reason why I did not notice this before
is because I simply did not really test it before.

As I mentioned at the start of the thread, I noticed this while working
on SW_TABLET_MODE support, and thus doing a lot of attaching / detaching
of the keyboard. I believe that this issue has existed since we stopped
setting HID_QUIRK_NO_INIT_REPORTS in the hid-multitouch code.

Anyways the proof is in the pudding, see below.

>> Also quite a few other multi-touch devices have a HID_QUIRK_NO_INIT_REPORTS
>> quirk, at least a bunch of surface keyboards do; and if I'm reading the
>> git log correctly then at one point in time we used to have a
>> HID_QUIRK_NO_INIT_REPORTS for at least some of the multi-touch classes
>> inside hid-multitouch.c. At least there is a commit titled:
>>
>> "HID: multitouch: do not set HID_QUIRK_NO_INIT_REPORTS"
>>
>> Which suggests that one point we did set it inside the multi-touch
>> driver; and I'm wondering since a bunch of surface keyboards need this
>> if setting this inside the multi-touch driver would not get us closer
>> to windows behavior.
> 
> This quirk is legacy, and should have stayed that way (well, it doesn't
> hurt anyway). As mentioned, in the past, the hid core stack used to fetch
> all input and feature reports on plug in. This was known to break many
> devices, and we had to use the no-init-report quirk for them. Then, we
> realized that it was not correct to do that way, and I removed this
> behavior. However, I couldn't remove the quirk entirely because of hiddev
> IIRC. In the hiddev case, userspace expects the device to have known values
> for the features, but that is not 100% working. So to not break userspace,
> I had to keep that quirk around for this use case.
> 
>>
>> Anyways if you have an alternative fix you want me to test, let me know.
> 
> Peter has a patch, we were waiting for the reporter to test it, but it's
> been crickets since last week.
> 
> Here it is:
> 
>
> Subject: [DRAFT PATCH] HID: input: do not run GET_REPORT unless there's a Resolution Multiplier

Ok, so I've given this a try with the HID_QUIRK_NO_INIT_REPORTS patch
I did reverted and the kbd and touchpad failed to send events after
the first detach + re-attach.

With the HID_QUIRK_NO_INIT_REPORTS patch in place detach + re-attach is
no problem.

Regards,

Hans