All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kenny Levinsen <kl@kl.wtf>
To: Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Douglas Anderson <dianders@chromium.org>,
	Hans de Goede <hdegoede@redhat.com>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	Kenny Levinsen <kl@kl.wtf>
Subject: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor
Date: Sun, 31 Mar 2024 20:24:40 +0200	[thread overview]
Message-ID: <20240331182440.14477-1-kl@kl.wtf> (raw)

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 acknowledgement 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 acknowledgement, 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 acknowledgement, and as reading quickly breaks the report
descriptor on the Sensel touchpad, revert to waiting for reset
acknowledgement 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")
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2df1ab3c31cc..72d2bccf5621 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -735,9 +735,12 @@ 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;
@@ -767,16 +770,8 @@ static int i2c_hid_parse(struct hid_device *hid)
 		}
 	}
 
-	/*
-	 * 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;
 
-- 
2.44.0


             reply	other threads:[~2024-03-31 18:24 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-31 18:24 Kenny Levinsen [this message]
2024-04-02 11:06 ` [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor Hans de Goede
2024-04-02 11:30   ` Kenny Levinsen
2024-04-02 11:43     ` Hans de Goede
2024-04-22 17:10 ` Linux regression tracking (Thorsten Leemhuis)
2024-04-23 14:59   ` Benjamin Tissoires
2024-04-24 16:56     ` regression fixes sitting in subsystem git trees for a week or longer (was: Re: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor) Thorsten Leemhuis
2024-04-24 18:53       ` Linus Torvalds
2024-04-25  8:25         ` regression fixes sitting in subsystem git trees for a week or longer Thorsten Leemhuis
2024-04-25  8:44           ` Benjamin Tissoires
2024-04-25  9:33             ` Linux regression tracking (Thorsten Leemhuis)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240331182440.14477-1-kl@kl.wtf \
    --to=kl@kl.wtf \
    --cc=benjamin.tissoires@redhat.com \
    --cc=dianders@chromium.org \
    --cc=hdegoede@redhat.com \
    --cc=jikos@kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.