Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The IRQ handler dereferences a global pointer that is set to NULL on partial device unbind, leading to a race condition and a guaranteed kernel crash if an interrupt fires. - [High] Unbinding and rebinding either of the I2C drivers permanently breaks the bridge. -- --- Patch [12]: [PATCH v2 12/37] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Add atomic bridge state handlers --- commit a14f762105b409d757000a124c825edac22d569b Author: Maxime Ripard <[email protected]> drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Add atomic bridge state handlers The megachips-stdpxxxx-ge-b850v3-fw 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/megachips-stdpxxxx-ge-b850v3-fw.c > b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > index 2d02cc69f2374..25ff0a0acc934 100644 > --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c > @@ -214,6 +214,9 @@ static int ge_b850v3_lvds_attach(struct drm_bridge > *bridge, > } > > static const struct drm_bridge_funcs ge_b850v3_lvds_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 = ge_b850v3_lvds_attach, > .detect = ge_b850v3_lvds_bridge_detect, > .edid_read = ge_b850v3_lvds_edid_read, > }; [Severity: High] This is a pre-existing issue, but unbinding and rebinding either of the I2C drivers appears to permanently break the bridge. When either device unbinds, ge_b850v3_lvds_remove() clears the global ge_b850v3_lvds_ptr: drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c:ge_b850v3_lvds_remove() { ... ge_b850v3_lvds_ptr = NULL; ... } If the unbound device is rebound, ge_b850v3_lvds_init() allocates a new struct: drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c:ge_b850v3_lvds_init() { ... ge_b850v3_lvds_ptr = devm_drm_bridge_alloc(dev, struct ge_b850v3_lvds, bridge, &ge_b850v3_lvds_funcs); ... } Because the other device's probe function does not run again, the new struct never receives the i2c_client pointer for the other device. Does this cause the initialization check in stdp2690_ge_b850v3_fw_probe() to fail and prevent the bridge from registering? drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c:stdp2690_ge_b850v3_fw_probe() { ... /* Only register after both bridges are probed */ if (!ge_b850v3_lvds_ptr->stdp4028_i2c) return 0; ... } [Severity: High] This isn't a bug introduced by this patch, but could this logic also introduce a race condition leading to a NULL pointer dereference? Because the IRQ is managed via devm_request_threaded_irq() on stdp4028, it remains active until devres cleanup runs. If the interrupt fires after ge_b850v3_lvds_remove() sets ge_b850v3_lvds_ptr to NULL, the IRQ handler unconditionally dereferences the NULL pointer: drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c:ge_b850v3_lvds_irq_handler() { struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c; ... } Could this lead to a kernel panic if an interrupt occurs during or after an unbind? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
