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

["Mark Pearson" <mpearson@squebb.ca>]

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>