Re: [PATCH v2 12/37] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Add atomic bridge state handlers

[sashiko-bot@kernel.org]

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 <mripard@kernel.org>

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/20260707-drm-all-atomic-bridges-v2-0-21d03cbca446@kernel.org?part=12