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 <[email protected]> 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/[email protected]?part=2
