[PATCH] HID: i2c-hid: Skip SET_POWER on shutdown for panel followers

[PATCH] HID: i2c-hid: Skip SET_POWER on shutdown for panel followers

[Justin Yeh]

When an i2c-hid device is a panel follower, drm_atomic_helper_shutdown()
unprepares the panel before device_shutdown() gets around to the i2c
client. The panel_unpreparing() callback already calls
i2c_hid_core_suspend() with force_poweroff set, which sends
SET_POWER(SLEEP) and then drops the device's regulators.

So by the time i2c_hid_core_shutdown() runs, the controller is unpowered
and can no longer complete a transfer. On MT8196 the SET_POWER(SLEEP)
issued there burns the full i2c-mt65xx bus timeout before failing,
adding two seconds to every reboot.

Skip the redundant SET_POWER for panel followers, matching how
i2c_hid_core_pm_suspend() and friends already bail out for them.
free_irq() and the shutdown_tail() hook are unaffected.

Fixes: 96a37bfd232a ("HID: i2c-hid: Support being a panel follower")
Signed-off-by: Justin Yeh <justin.yeh@mediatek.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 0ff07fdab442..2e787c17256d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1355,7 +1355,9 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
-	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+	if (!ihid->is_panel_follower)
+		i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+
 	free_irq(client->irq, ihid);
 
 	i2c_hid_core_shutdown_tail(ihid);
-- 
2.45.2

Re: [PATCH] HID: i2c-hid: Skip SET_POWER on shutdown for panel followers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The i2c-hid driver passes an unaligned, embedded structure to a DMA-safe I2C transfer, violating DMA safety rules for cacheline alignment.
- [High] Missing deregistration of panel follower in the shutdown path allows asynchronous panel callbacks to execute on a freed IRQ action.
--

commit 1cf5a880978c5891efe7abbc9352dfefd3ad2697
Author: Justin Yeh <justin.yeh@mediatek.com>
HID: i2c-hid: Skip SET_POWER on shutdown for panel followers

This commit modifies the shutdown sequence for i2c-hid devices that act as
panel followers. It skips the SET_POWER command to avoid bus timeouts on
reboot, as the panel's unpreparing callback already powers off the device.

> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 0e725a0f0abee..13f3dcc52440f 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1353,7 +1353,9 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
>  {
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  
> -	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
> +	if (!ihid->is_panel_follower)
> +		i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
> +
>  	free_irq(client->irq, ihid);

[Severity: High]
This is a pre-existing issue, but does missing deregistration of the panel
follower in the shutdown path allow asynchronous panel callbacks to execute
on a freed IRQ action?

The commit relies on an implicit assumption that the DRM display controller
shuts down and unprepares the panel before the i2c-hid device shuts down.

However, if the probe ordering flips and i2c-hid's shutdown() runs first,
it calls free_irq() here but fails to remove the panel follower, unlike
remove() which correctly calls drm_panel_remove_follower().

Later, when the DRM controller shuts down and unprepares the panel, the
panel invokes the i2c-hid follower's panel_unpreparing callback. This
triggers i2c_hid_core_panel_follower_suspend(), which ultimately calls
disable_irq(client->irq) after free_irq() was already executed.

Could this cause a kernel WARNING and an unbalanced IRQ depth during system
reboot or shutdown?

>  
>  	i2c_hid_core_shutdown_tail(ihid);

While reviewing this code, I also noticed another pre-existing issue regarding
DMA safety in the driver's probe path.

In i2c_hid_core_fetch_hid_descriptor():

    error = i2c_hid_read_register(ihid,
                                  ihid->wHIDDescRegister,
                                  &ihid->hdesc,
                                  sizeof(ihid->hdesc));

This passes &ihid->hdesc (which is embedded inside the heap-allocated
struct i2c_hid and not independently cacheline aligned) as the receive
buffer.

Then in i2c_hid_xfer(), the I2C_M_DMA_SAFE flag is explicitly set:

    msgs[n].flags = (client->flags & I2C_M_TEN) |
                    I2C_M_RD | I2C_M_DMA_SAFE;
    msgs[n].len = recv_len;
    msgs[n].buf = recv_buf;

[Severity: High]
This isn't a bug introduced by this patch, but does explicitly setting
I2C_M_DMA_SAFE on an unaligned, embedded buffer violate cacheline alignment
rules for DMA-safe I2C transfers?

Could this cause memory corruption of adjacent fields in struct i2c_hid
during the DMA transaction due to cacheline sharing on cache-incoherent
architectures, bypassing the I2C core's bounce buffering?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904035009.4114363-1-justin.yeh@mediatek.com?part=1

Re: [PATCH] HID: i2c-hid: Skip SET_POWER on shutdown for panel followers

[Doug Anderson]

Hi,

On Thu, Sep 3, 2026 at 8:50 PM Justin Yeh <justin.yeh@mediatek.com> wrote:
>
> When an i2c-hid device is a panel follower, drm_atomic_helper_shutdown()
> unprepares the panel before device_shutdown() gets around to the i2c
> client. The panel_unpreparing() callback already calls
> i2c_hid_core_suspend() with force_poweroff set, which sends
> SET_POWER(SLEEP) and then drops the device's regulators.
>
> So by the time i2c_hid_core_shutdown() runs, the controller is unpowered
> and can no longer complete a transfer. On MT8196 the SET_POWER(SLEEP)
> issued there burns the full i2c-mt65xx bus timeout before failing,
> adding two seconds to every reboot.

I don't think there is any guarantee here though, is there? In your
case the panel might have been powered off before
i2c_hid_core_shutdown(), but is there something that ensures that? I'm
not aware of it.

Would it be better to follow in the example of i2c_hid_core_remove()
and just remove ourselves as a panel follower? That would
automatically call the power-off functions if needed, right?

-Doug