Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing `.remove` hook for `drm_bridge_remove` causes a use-after-free upon module unload or device unbind. - [High] Sleeping function usleep_range() is called from an atomic commit path. - [High] Incorrect masking of the WPKEY password truncates the unlock key, preventing hardware configuration. -- --- Patch [13]: [PATCH v2 13/37] drm/bridge: microchip-lvds: Add atomic bridge state handlers --- commit 489328dd1eddcba7ecc56ad3f142d3a6ed0da2f5 Author: Maxime Ripard <[email protected]> drm/bridge: microchip-lvds: Add atomic bridge state handlers The microchip-lvds bridge doesn't implement any enable or disable callbacks, but is still missing the atomic state handlers. In order to remove the legacy bridge callback support from the DRM bridge core, add them. > diff --git a/drivers/gpu/drm/bridge/microchip-lvds.c > b/drivers/gpu/drm/bridge/microchip-lvds.c > index 5fb8633f43c5c..dd4ffc9f4df3f 100644 > --- a/drivers/gpu/drm/bridge/microchip-lvds.c > +++ b/drivers/gpu/drm/bridge/microchip-lvds.c > @@ -167,6 +167,9 @@ static void mchp_lvds_atomic_disable(struct drm_bridge > *bridge, > } > > static const struct drm_bridge_funcs mchp_lvds_bridge_funcs = { > + .atomic_create_state = drm_atomic_helper_bridge_create_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .attach = mchp_lvds_attach, > .atomic_enable = mchp_lvds_atomic_enable, [Severity: High] This isn't a bug introduced by this patch, but does the atomic enable path safely handle sleeping functions? The subsystem guide states that bridge atomic enable callbacks run in atomic context. However, mchp_lvds_atomic_enable() calls lvds_serialiser_on(), which has a hardware polling loop using usleep_range(): drivers/gpu/drm/bridge/microchip-lvds.c:lvds_serialiser_on() { ... usleep_range(1000, 2000); ... } Can this cause kernel warnings or potential deadlocks during the atomic commit path? > .atomic_disable = mchp_lvds_atomic_disable, > }; [Severity: High] This is a pre-existing issue, but is the WPKEY password being truncated during initialization? In lvds_serialiser_on(), the password is masked directly without shifting: drivers/gpu/drm/bridge/microchip-lvds.c:lvds_serialiser_on() { ... lvds_writel(lvds, LVDSC_WPMR, (LVDSC_WPMR_WPKEY_PSSWD & LVDSC_WPMR_WPKEY_MASK)); ... } If LVDSC_WPMR_WPKEY_PSSWD is 0x4C5644 and LVDSC_WPMR_WPKEY_MASK is GENMASK(31, 8) (0xFFFFFF00), the lower byte 'D' (0x44) gets truncated to 0x00. Does this prevent the write protection from being unlocked, causing subsequent LVDS configuration writes to be ignored by the hardware? [Severity: Critical] This is also a pre-existing issue, but does the driver leave the bridge registered on unload? The bridge is registered globally in mchp_lvds_probe() using drm_bridge_add(), but the platform driver lacks a .remove callback: drivers/gpu/drm/bridge/microchip-lvds.c:mchp_lvds_driver { .probe = mchp_lvds_probe, .driver = { .name = "microchip-lvds", .of_match_table = mchp_lvds_dt_ids, }, }; Since drm_bridge_remove() is never called to unlink the bridge, and the mchp_lvds structure is freed automatically by devres, could this lead to a use-after-free the next time the DRM core traverses the global bridge list? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
