[Deepin-Kernel-SIG] [Upstream] [linux 6.6-y] Sync some i2c-hid bugfix from mainline #607

opsiff · 2025-02-18T07:14:22Z

Commit(12):
HID: i2c-hid: Revert to using power commands to wake on resume
HID: i2c-hid: Delayed i2c resume wakeup for 0x0d42 Goodix touchpad
HID: i2c-hid: ensure various commands do not interfere with each other
HID: i2c-hid: Remove unused label in i2c_hid_set_power
HID: i2c-hid: Use address probe to wake on resume
HID: i2c-hid: Retry address probe after delay
HID: i2c-hid: Revert to await reset ACK before reading report descriptor
HID: i2c-hid: Turn missing reset ack into a warning
HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor
HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling
HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions
HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()

Summary by Sourcery

This pull request synchronizes several I2C HID bug fixes from the mainline kernel, addressing issues related to device wakeup, reset, and power management. It includes improvements to error handling, code structure, and the addition of a quirk for specific Goodix devices to ensure proper operation after resume.

Bug Fixes:

Fixes an issue where certain STM-based and Weida Tech devices require a delay after a rising clock edge to wake from deep sleep by retrying the address probe after a short sleep.
Fixes a potential race condition during device reset by protecting the reset sequence with a mutex and ensuring that feature reports are not sent during the reset process.

Enhancements:

Improves error handling in the i2c_hid_parse function by using goto-style error handling for cleaner code and easier maintenance.
Refactors the device reset process by splitting the i2c_hid_hwreset function into start and finish functions for better modularity and control.
Introduces a command lock to protect the command and raw buffers, preventing interference between different commands.
Adds a quirk to delay wakeup after resume for Goodix 27c6:0d42 devices to ensure proper device initialization and prevent input interrupt issues.

Chores:

Removes an unused label in the i2c_hid_set_power function to improve code cleanliness.

mainline inclusion from mainline-v6.8-rc1 category: bugfix i2c_hid_hwreset() is the only caller of i2c_hid_execute_reset(), fold the latter into the former. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. No functional changes intended. Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit f023605)

mainline inclusion from mainline-v6.8-rc1 category: bugfix Split i2c_hid_hwreset() into: i2c_hid_start_hwreset() which sends the PWR_ON and reset commands; and i2c_hid_finish_hwreset() which actually waits for the reset to complete. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. No functional changes intended. Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit 96d3098)

mainline inclusion from mainline-v6.8-rc1 category: bugfix Switch i2c_hid_parse() to goto style error handling. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. Note this changes the descriptor read error path to propagate the actual i2c_hid_read_register() error code (which is always negative) instead of hardcoding a -EIO return. Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit aa69d69)

…rt-descriptor mainline inclusion from mainline-v6.8-rc1 category: bugfix A recent bug made me look at Microsoft's i2c-hid docs again and I noticed the following: """ 4. Issue a RESET (Host Initiated Reset) to the Device. 5. Retrieve report descriptor from the device. Note: Steps 4 and 5 may be done in parallel to optimize for time on I²C. Since report descriptors are (a) static and (b) quite long, Windows 8 may issue a request for 5 while it is waiting for a response from the device on 4. """ Which made me think that maybe on some touchpads the reset ack is delayed till after the report descriptor is read ? Testing a T-BAO Tbook Air 12.5 with a 0911:5288 (SIPODEV SP1064?) touchpad, for which the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk was first introduced, shows that reading the report descriptor before waiting for the reset helps with the missing reset IRQ. Now the reset does get acked properly, but the ack sometimes still does not happen unfortunately. Still moving the wait for ack to after reading the report-descriptor, is probably a good idea, both to make i2c-hid's behavior closer to Windows as well as to speed up probing i2c-hid devices. While at it drop the dbg_hid() for a malloc failure, malloc failures already get logged extensively by malloc itself. Link: https://bugzilla.redhat.com/show_bug.cgi?id=2247751 Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit af93a16)

mainline inclusion from mainline-v6.8-rc1 category: bugfix On all i2c-hid devices seen sofar the reset-ack either works, or the hw is somehow buggy and does not (always) ack the reset properly, yet it still works fine. Lower the very long reset timeout to 1 second which should be plenty and change the reset not getting acked from an error into a warning. This results in a bit cleaner code and avoids the need to add more I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks in the future. Reviewed-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit 7bcf9eb)

mainline inclusion from mainline-v6.9-rc6 category: bugfix In af93a16, 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: af93a16 ("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> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit ea36bf1)

mainline inclusion from mainline-v6.10-rc1 category: bugfix Some STM microcontrollers need 400µs after rising clock edge in order to come out of their deep sleep state. This in turn means that our address probe will fail as the device is not ready to service it. Retry the probe once after a delay to see if the device came alive, otherwise treat the device as missing. Link: https://lore.kernel.org/all/20240405102436.3479210-1-lma@chromium.org/#t Co-developed-by: Radoslaw Biernacki <rad@chromium.org> Co-developed-by: Lukasz Majczak <lma@chromium.org> Signed-off-by: Kenny Levinsen <kl@kl.wtf> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit ab5ec06)

mainline inlcusion from mainline-v6.10-rc1 category: bugfix Certain devices, both from STM and Weida Tech, need to be woken up after having entered a deeper sleep state. The relevant places to wake up such device is during our initial HID probe, and after resuming. A retry for power commands was previously added to i2c_hid_set_power to wake up Weida Tech devices, but lacked sufficient sleep for STM devices. Replace the power command retry with the same address probe we using during our initial HID probe. Signed-off-by: Kenny Levinsen <kl@kl.wtf> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit 7d6f065)

mainline inclusion from mainline-v6.10-rc1 category: bugfix This label was left behind when the wake-up logic was moved from i2c_hid_set_power to i2c_hid_probe_address. Clean it up as it causes warnings-as-errors builds to fail. Fixes: bb1033c8a3ea ("HID: i2c-hid: Use address probe to wake on resume") Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by: Kenny Levinsen <kl@kl.wtf> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit d2b34fa)

mainline inclusion from mainline-v6.12-rc1 category: bugfix i2c-hid uses 2 shared buffers: command and "raw" input buffer for sending requests to peripherals and read data from peripherals when executing variety of commands. Such commands include reading of HID registers, requesting particular power mode, getting and setting reports and so on. Because all such requests use the same 2 buffers they should not execute simultaneously. Fix this by introducing "cmd_lock" mutex and acquire it whenever we needs to access ihid->cmdbuf or idid->rawbuf. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit b4ed18a)

mainline inclusion from mainline-v6.12-rc5 category: bugfix Patch for Goodix 27c6:0d42 touchpads found in Inspiron 5515 laptops. After resume from suspend, one can communicate with this device just fine. We can read data from it or request a reset, but for some reason the interrupt line will not go up when new events are available. (it can correctly respond to a reset with an interrupt tho) The only way I found to wake this device up is to send anything to it after ~1.5s mark, for example a simple read request, or power mode change. In this patch, I simply delay the resume steps with msleep, this will cause the set_power request to happen after the ~1.5s barrier causing the device to resume its event interrupts. Sleep was used rather than delayed_work to make this workaround as non-invasive as possible. [jkosina@suse.com: shortlog update] Signed-off-by: Bartłomiej Maryńczak <marynczakbartlomiej@gmail.com> Signed-off-by: Jiri Kosina <jkosina@suse.com> (cherry picked from commit 293c485)

mainline inclusion from mainline-v6.13-rc2 category: bugfix commit 7d6f065 ("HID: i2c-hid: Use address probe to wake on resume") replaced the retry of power commands with the dummy read "bus probe" we use on boot which accounts for a necessary delay before retry. This made at least one Weida device (2575:0910 in an ASUS Vivobook S14) very unhappy, as the bus probe despite being successful somehow lead to the following power command failing so hard that the device never lets go of the bus. This means that even retries of the power command would fail on a timeout as the bus remains busy. Remove the bus probe on resume and instead reintroduce retry of the power command for wake-up purposes while respecting the newly established wake-up retry timings. Fixes: 7d6f065 ("HID: i2c-hid: Use address probe to wake on resume") Cc: stable@vger.kernel.org Reported-by: Michael <auslands-kv@gmx.de> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219440 Link: https://lore.kernel.org/r/d5acb485-7377-4139-826d-4df04d21b5ed@leemhuis.info/ Signed-off-by: Kenny Levinsen <kl@kl.wtf> Link: https://patch.msgid.link/20241119235615.23902-1-kl@kl.wtf Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> (cherry picked from commit 3485143)

sourcery-ai · 2025-02-18T07:14:27Z

Reviewer's Guide by Sourcery

This pull request syncs I2C HID bug fixes from the mainline kernel. It introduces a quirk for delaying wakeup after resume for Goodix touchpads, improves reset handling with locking and power management, adds a mutex to protect command buffers, and reverts some changes related to address probing and reset acknowledgment.

Sequence diagram for i2c-hid device reset

sequenceDiagram
  participant i2c_hid_core_probe
  participant i2c_hid_start_hwreset
  participant i2c_hid_set_power
  participant i2c_hid_xfer
  participant i2c_hid_finish_hwreset

  i2c_hid_core_probe->>i2c_hid_start_hwreset: i2c_hid_hwreset()
  activate i2c_hid_start_hwreset
  i2c_hid_start_hwreset->>i2c_hid_set_power: i2c_hid_set_power(I2C_HID_PWR_ON)
  activate i2c_hid_set_power
  i2c_hid_set_power-->>i2c_hid_start_hwreset: return
  deactivate i2c_hid_set_power
  i2c_hid_start_hwreset->>i2c_hid_xfer: i2c_hid_xfer(I2C_HID_OPCODE_RESET)
  activate i2c_hid_xfer
  i2c_hid_xfer-->>i2c_hid_start_hwreset: return
  deactivate i2c_hid_xfer
  i2c_hid_start_hwreset-->>i2c_hid_core_probe: return
  deactivate i2c_hid_start_hwreset
  i2c_hid_core_probe->>i2c_hid_finish_hwreset: i2c_hid_finish_hwreset()
  activate i2c_hid_finish_hwreset
  i2c_hid_finish_hwreset->>i2c_hid_set_power: i2c_hid_set_power(I2C_HID_PWR_ON)
  activate i2c_hid_set_power
  i2c_hid_set_power-->>i2c_hid_finish_hwreset: return
  deactivate i2c_hid_set_power
  i2c_hid_finish_hwreset-->>i2c_hid_core_probe: return
  deactivate i2c_hid_finish_hwreset

File-Level Changes

Change	Details	Files
Introduces a quirk to delay wakeup after resume for Goodix touchpads and adds a probe function to handle devices needing time to wake up.	Added `I2C_HID_QUIRK_DELAY_WAKEUP_AFTER_RESUME` quirk. Added a quirk entry for Goodix 0D42 device. Added `i2c_hid_probe_address` to handle devices needing time to wake up. Added a delay for devices with the `I2C_HID_QUIRK_DELAY_WAKEUP_AFTER_RESUME` quirk in `i2c_hid_core_resume`.	`drivers/hid/i2c-hid/i2c-hid-core.c` `drivers/hid/hid-ids.h`
Improves reset handling by splitting the hardware reset process into start and finish functions, and ensuring proper locking and power management during reset.	Split `i2c_hid_hwreset` into `i2c_hid_start_hwreset` and `i2c_hid_finish_hwreset`. Replaced direct calls to `i2c_hid_execute_reset` with the new split functions in `i2c_hid_parse` and `i2c_hid_core_resume`. Added locking to `i2c_hid_start_hwreset` to prevent feature reports during reset. Ensured power is managed correctly during reset in `i2c_hid_start_hwreset`. The function `i2c_hid_finish_hwreset` now includes a `dev_warn` if the device does not acknowledge the reset within 1000ms.	`drivers/hid/i2c-hid/i2c-hid-core.c`
Improves command handling by adding a mutex to protect command and raw buffers, and using goto style error handling in `i2c_hid_parse`.	Added `cmd_lock` mutex to the `i2c_hid` struct. Added locking around command buffer access in `i2c_hid_read_register`, `i2c_hid_get_report`, `i2c_hid_set_or_send_report`, and `i2c_hid_set_power_command`. Switched `i2c_hid_parse` to goto style error handling.	`drivers/hid/i2c-hid/i2c-hid-core.c`
Reverts to using power commands to wake on resume.	Reverted the commit that used address probe to wake on resume. Reverted the commit that delayed i2c resume wakeup for 0x0d42 Goodix touchpad.	`drivers/hid/i2c-hid/i2c-hid-core.c`
Reverts to awaiting reset ACK before reading report descriptor.	Reverted the commit that moved `i2c_hid_finish_hwreset()` to after reading the report-descriptor.	`drivers/hid/i2c-hid/i2c-hid-core.c`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Generate a plan of action for an issue: Comment @sourcery-ai plan on
an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @opsiff - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider using devm_kzalloc for allocating rdesc in i2c_hid_parse to simplify memory management.
It looks like the i2c_hid_hwreset function was split into i2c_hid_start_hwreset and i2c_hid_finish_hwreset, but the resume path still calls the old function in some cases - can we refactor that?

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

deepin-ci-robot · 2025-02-18T07:16:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sourcery-ai[bot]
Once this PR has been reviewed and has the lgtm label, please ask for approval from opsiff. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jwrdegoede and others added 12 commits February 18, 2025 15:00

deepin-ci-robot requested review from huangbibo and justforlxz February 18, 2025 07:14

sourcery-ai bot approved these changes Feb 18, 2025

View reviewed changes

opsiff merged commit 475ff0f into deepin-community:linux-6.6.y Feb 18, 2025
4 of 5 checks passed

MingcongBai mentioned this pull request Mar 5, 2025

【跟踪】6.12 系列合并 6.6 补丁情况一览 deepin-community/kernel-rolling#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Deepin-Kernel-SIG] [Upstream] [linux 6.6-y] Sync some i2c-hid bugfix from mainline #607

[Deepin-Kernel-SIG] [Upstream] [linux 6.6-y] Sync some i2c-hid bugfix from mainline #607

Uh oh!

opsiff commented Feb 18, 2025 •

edited by sourcery-ai bot

Loading

Uh oh!

sourcery-ai bot commented Feb 18, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Uh oh!

deepin-ci-robot commented Feb 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[Deepin-Kernel-SIG] [Upstream] [linux 6.6-y] Sync some i2c-hid bugfix from mainline #607

[Deepin-Kernel-SIG] [Upstream] [linux 6.6-y] Sync some i2c-hid bugfix from mainline #607

Uh oh!

Conversation

opsiff commented Feb 18, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide by Sourcery

Sequence diagram for i2c-hid device reset

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

deepin-ci-robot commented Feb 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

opsiff commented Feb 18, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Feb 18, 2025 •

edited

Loading