Re: [PATCH v4 2/5] drm/bridge: it6505: disable runtime PM on remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver leaks the runtime PM usage counter if unbound while the DisplayPort cable is connected, breaking subsequent driver binds.
- [High] Resource registration asymmetry for the DRM DP AUX adapter causes registration failures on re-attach.
- [Medium] Missing error check on I2C reads in the IRQ handler causes negative error codes to be misinterpreted as phantom interrupts.
- [Low] The PM callback structure is assigned unconditionally, violating conditional compilation guidelines and creating dead code.
--

--- Patch [2]: [PATCH v4 2/5] drm/bridge: it6505: disable runtime PM on remove ---
commit 6b8ae3352685a773af0a6bbc5a90e489dc50ff6b
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: disable runtime PM on remove

This commit disables runtime PM in the remove callback for the it6505 bridge
driver. It cancels work items before disabling runtime PM and powering off the
chip, which prevents the device from having an unbalanced disable depth after
unbind.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 7873ae6df3aff..2a8298ee6c03c 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -3655,6 +3655,7 @@ static void it6505_i2c_remove(struct i2c_client *client)
>  	cancel_work_sync(&it6505->hdcp_wait_ksv_list);
>  	cancel_delayed_work_sync(&it6505->hdcp_work);
>  	cancel_work_sync(&it6505->extcon_wq);
> +	pm_runtime_disable(&client->dev);

[Severity: High]
This is a pre-existing issue, but does the driver leak the runtime PM usage
counter if unbound while the DisplayPort cable is connected?

If the driver is unbound while a display is connected, it6505_extcon_work() has
previously called pm_runtime_get_sync() and elevated the PM usage counter.

The remove callback disables runtime PM via pm_runtime_disable() but fails to
drop the usage counter.

Because the usage count belongs to the struct device and persists across
bind/unbind cycles, a subsequent bind will start with an elevated usage count.
When extcon_work runs on the new bind, pm_runtime_get_sync() will return 1,
bypassing the it6505_poweron() sequence.

Can this cause the hardware to remain unpowered and all subsequent I2C
operations to fail?

>  	it6505_poweroff(it6505);
>  	it6505_remove_edid(it6505);
>  }

[Severity: High]
This is a pre-existing issue, but does the resource registration asymmetry for
the DRM DP AUX adapter cause registration failures on re-attach?

The driver registers the AUX channel via drm_dp_aux_register() in the
it6505_bridge_attach() callback, but unregisters it via
drm_dp_aux_unregister() in the I2C driver's it6505_i2c_remove() callback.

If the parent DRM device is unbound, the bridge is detached but the I2C driver
remains bound. When the DRM device is rebound, the bridge is attached again,
and drm_dp_aux_register() is called a second time on the same instance without
it having been unregistered.

Can this cause kernel warnings and registration failures in the I2C core?


[Severity: Medium]
This is a pre-existing issue, but can the missing error check on I2C reads in
the IRQ handler cause negative error codes to be misinterpreted as phantom
interrupts?

In it6505_int_threaded_handler(), the driver reads interrupt status registers
via it6505_read(). If the I2C read fails, it returns a negative error code:

    int_status[0] = it6505_read(it6505, INT_STATUS_01);
    int_status[1] = it6505_read(it6505, INT_STATUS_02);
    int_status[2] = it6505_read(it6505, INT_STATUS_03);

This negative integer is stored in the int_status array and passed to
it6505_test_bit(), which accesses it as an unsigned int.

Since a negative value has most of its bits set to 1, it6505_test_bit()
evaluates to true for multiple interrupt sources.

Does this cause spurious execution of interrupt sub-handlers and unpredictable
state changes?


[Severity: Low]
This is a pre-existing issue, but is the PM callback structure assigned
unconditionally?

In the it6505_i2c_driver structure, the .pm field is assigned directly to
&it6505_bridge_pm_ops.

According to the Power Management subsystem guidelines, this assignment should
be wrapped with the pm_ptr() macro to ensure the pointer becomes NULL when
CONFIG_PM is disabled.

Does this unconditional assignment create dead code and compiler warnings when
power management is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784681487.git.daniel@makrotopia.org?part=2