diff mbox series

[v3] HID: i2c-hid: Revert to await reset ACK before reading report descriptor

Message ID 20240402111004.161434-1-hdegoede@redhat.com
State New
Headers show
Series [v3] HID: i2c-hid: Revert to await reset ACK before reading report descriptor | expand

Commit Message

Hans de Goede April 2, 2024, 11:10 a.m. UTC
From: Kenny Levinsen <kl@kl.wtf>

In af93a167eda9, i2c_hid_parse was changed to continue with reading the
report descriptor before waiting for reset to be acknowledged.

This has lead to two regressions:

1. We fail to handle reset acknowledgment if it happens while reading
   the report descriptor. The transfer sets I2C_HID_READ_PENDING, which
   causes the IRQ handler to return without doing anything.

   This affects both a Wacom touchscreen and a Sensel touchpad.

2. On a Sensel touchpad, reading the report descriptor this quickly
   after reset results in all zeroes or partial zeroes.

The issues were observed on the Lenovo Thinkpad Z16 Gen 2.

The change in question was made based on a Microsoft article[0] stating
that Windows 8 *may* read the report descriptor in parallel with
awaiting reset acknowledgment, intended as a slight reset performance
optimization. Perhaps they only do this if reset is not completing
quickly enough for their tastes?

As the code is not currently ready to read registers in parallel with a
pending reset acknowledgment, and as reading quickly breaks the report
descriptor on the Sensel touchpad, revert to waiting for reset
acknowledgment before proceeding to read the report descriptor.

[0]: https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management

Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor")
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2271136
Cc: stable@vger.kernel.org
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
Link: https://lore.kernel.org/r/20240331182440.14477-1-kl@kl.wtf
[hdegoede@redhat.com Drop no longer necessary abort_reset error exit path]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

Comments

Mark Pearson April 2, 2024, 6:09 p.m. UTC | #1
On Tue, Apr 2, 2024, at 7:10 AM, Hans de Goede wrote:
> From: Kenny Levinsen <kl@kl.wtf>
>
> In af93a167eda9, i2c_hid_parse was changed to continue with reading the
> report descriptor before waiting for reset to be acknowledged.
>
> This has lead to two regressions:
>
> 1. We fail to handle reset acknowledgment if it happens while reading
>    the report descriptor. The transfer sets I2C_HID_READ_PENDING, which
>    causes the IRQ handler to return without doing anything.
>
>    This affects both a Wacom touchscreen and a Sensel touchpad.
>
> 2. On a Sensel touchpad, reading the report descriptor this quickly
>    after reset results in all zeroes or partial zeroes.
>
> The issues were observed on the Lenovo Thinkpad Z16 Gen 2.
>
> The change in question was made based on a Microsoft article[0] stating
> that Windows 8 *may* read the report descriptor in parallel with
> awaiting reset acknowledgment, intended as a slight reset performance
> optimization. Perhaps they only do this if reset is not completing
> quickly enough for their tastes?
>
> As the code is not currently ready to read registers in parallel with a
> pending reset acknowledgment, and as reading quickly breaks the report
> descriptor on the Sensel touchpad, revert to waiting for reset
> acknowledgment before proceeding to read the report descriptor.
>
> [0]: 
> https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management
>
> Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to 
> after reading the report-descriptor")
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2271136
> Cc: stable@vger.kernel.org
> Signed-off-by: Kenny Levinsen <kl@kl.wtf>
> Link: https://lore.kernel.org/r/20240331182440.14477-1-kl@kl.wtf
> [hdegoede@redhat.com Drop no longer necessary abort_reset error exit 
> path]
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c 
> b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 2df1ab3c31cc..13d67d7c67b4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -735,12 +735,15 @@ static int i2c_hid_parse(struct hid_device *hid)
>  	mutex_lock(&ihid->reset_lock);
>  	do {
>  		ret = i2c_hid_start_hwreset(ihid);
> -		if (ret)
> +		if (ret == 0)
> +			ret = i2c_hid_finish_hwreset(ihid);
> +		else
>  			msleep(1000);
>  	} while (tries-- > 0 && ret);
> +	mutex_unlock(&ihid->reset_lock);
> 
>  	if (ret)
> -		goto abort_reset;
> +		return ret;
> 
>  	use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name,
>  								&rsize);
> @@ -750,11 +753,8 @@ static int i2c_hid_parse(struct hid_device *hid)
>  		i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
>  	} else {
>  		rdesc = kzalloc(rsize, GFP_KERNEL);
> -
> -		if (!rdesc) {
> -			ret = -ENOMEM;
> -			goto abort_reset;
> -		}
> +		if (!rdesc)
> +			return -ENOMEM;
> 
>  		i2c_hid_dbg(ihid, "asking HID report descriptor\n");
> 
> @@ -763,23 +763,10 @@ static int i2c_hid_parse(struct hid_device *hid)
>  					    rdesc, rsize);
>  		if (ret) {
>  			hid_err(hid, "reading report descriptor failed\n");
> -			goto abort_reset;
> +			goto out;
>  		}
>  	}
> 
> -	/*
> -	 * Windows directly reads the report-descriptor after sending reset
> -	 * and then waits for resets completion afterwards. Some touchpads
> -	 * actually wait for the report-descriptor to be read before signalling
> -	 * reset completion.
> -	 */
> -	ret = i2c_hid_finish_hwreset(ihid);
> -abort_reset:
> -	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> -	mutex_unlock(&ihid->reset_lock);
> -	if (ret)
> -		goto out;
> -
>  	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
> 
>  	ret = hid_parse_report(hid, rdesc, rsize);

Tested on my Z13 G2 system and confirmed this fixes the issue.

Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Jiri Kosina April 3, 2024, 12:02 p.m. UTC | #2
On Tue, 2 Apr 2024, Mark Pearson wrote:

> On Tue, Apr 2, 2024, at 7:10 AM, Hans de Goede wrote:
> > From: Kenny Levinsen <kl@kl.wtf>
> >
> > In af93a167eda9, i2c_hid_parse was changed to continue with reading the
> > report descriptor before waiting for reset to be acknowledged.
> >
> > This has lead to two regressions:
> >
> > 1. We fail to handle reset acknowledgment if it happens while reading
> >    the report descriptor. The transfer sets I2C_HID_READ_PENDING, which
> >    causes the IRQ handler to return without doing anything.
> >
> >    This affects both a Wacom touchscreen and a Sensel touchpad.
> >
> > 2. On a Sensel touchpad, reading the report descriptor this quickly
> >    after reset results in all zeroes or partial zeroes.
> >
> > The issues were observed on the Lenovo Thinkpad Z16 Gen 2.
> >
> > The change in question was made based on a Microsoft article[0] stating
> > that Windows 8 *may* read the report descriptor in parallel with
> > awaiting reset acknowledgment, intended as a slight reset performance
> > optimization. Perhaps they only do this if reset is not completing
> > quickly enough for their tastes?
> >
> > As the code is not currently ready to read registers in parallel with a
> > pending reset acknowledgment, and as reading quickly breaks the report
> > descriptor on the Sensel touchpad, revert to waiting for reset
> > acknowledgment before proceeding to read the report descriptor.
> >
> > [0]: 
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management
> >
> > Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to 
> > after reading the report-descriptor")
> > Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2271136
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Kenny Levinsen <kl@kl.wtf>
> > Link: https://lore.kernel.org/r/20240331182440.14477-1-kl@kl.wtf
> > [hdegoede@redhat.com Drop no longer necessary abort_reset error exit 
> > path]
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
[ .. snip .. ]
> Tested on my Z13 G2 system and confirmed this fixes the issue.
> 
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Applied to hid.git#for-6.9/upstream-fixes. Thanks,
diff mbox series

Patch

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2df1ab3c31cc..13d67d7c67b4 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -735,12 +735,15 @@  static int i2c_hid_parse(struct hid_device *hid)
 	mutex_lock(&ihid->reset_lock);
 	do {
 		ret = i2c_hid_start_hwreset(ihid);
-		if (ret)
+		if (ret == 0)
+			ret = i2c_hid_finish_hwreset(ihid);
+		else
 			msleep(1000);
 	} while (tries-- > 0 && ret);
+	mutex_unlock(&ihid->reset_lock);
 
 	if (ret)
-		goto abort_reset;
+		return ret;
 
 	use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name,
 								&rsize);
@@ -750,11 +753,8 @@  static int i2c_hid_parse(struct hid_device *hid)
 		i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
 	} else {
 		rdesc = kzalloc(rsize, GFP_KERNEL);
-
-		if (!rdesc) {
-			ret = -ENOMEM;
-			goto abort_reset;
-		}
+		if (!rdesc)
+			return -ENOMEM;
 
 		i2c_hid_dbg(ihid, "asking HID report descriptor\n");
 
@@ -763,23 +763,10 @@  static int i2c_hid_parse(struct hid_device *hid)
 					    rdesc, rsize);
 		if (ret) {
 			hid_err(hid, "reading report descriptor failed\n");
-			goto abort_reset;
+			goto out;
 		}
 	}
 
-	/*
-	 * Windows directly reads the report-descriptor after sending reset
-	 * and then waits for resets completion afterwards. Some touchpads
-	 * actually wait for the report-descriptor to be read before signalling
-	 * reset completion.
-	 */
-	ret = i2c_hid_finish_hwreset(ihid);
-abort_reset:
-	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
-	mutex_unlock(&ihid->reset_lock);
-	if (ret)
-		goto out;
-
 	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
 
 	ret = hid_parse_report(hid, rdesc, rsize);