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

[sashiko-bot@kernel.org]

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