Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Asynchronous firmware loading callback can execute after the device is unbound, causing a Use-After-Free on the devres-allocated `mhdp` structure. - [High] Unbalanced teardown of `hpd_work` and `devm_request_threaded_irq` allows a scheduled work to execute after `mhdp` is freed. - [High] `cdns_mhdp_bridge_atomic_duplicate_state` fails to copy `current_mode`, breaking link retraining and mishandling state lifecycles. -- commit e5ec8b954cd81adb602c1048268bad60ac236f06 Author: Maxime Ripard <[email protected]> drm/bridge: cdns-mhdp8546: Return an error pointer on allocation failure The drm_bridge_funcs.atomic_reset documentation states that the hook must return either a valid drm_bridge_state object or an ERR_PTR(). > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index 36c07b71fe04b..46779b49545bd 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > @@ -1927,7 +1927,7 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge) > > cdns_mhdp_state = kzalloc_obj(*cdns_mhdp_state); > if (!cdns_mhdp_state) > - return NULL; > + return ERR_PTR(-ENOMEM); > > __drm_atomic_helper_bridge_reset(bridge, &cdns_mhdp_state->base); > [Severity: High] This isn't a bug introduced by this patch, but could this asynchronous firmware callback cause a use-after-free if the device is unbound? If the driver is unbound and cdns_mhdp_remove() executes, it only waits up to 100ms for the firmware load to finish: cdns_mhdp_remove() { ... ret = wait_event_timeout(mhdp->fw_load_wq, mhdp->hw_state == MHDP_HW_READY, timeout); ... } If this times out, remove completes and devres frees the mhdp structure. When the asynchronous firmware subsystem later finishes, it invokes cdns_mhdp_fw_cb() which dereferences the freed mhdp pointer: cdns_mhdp_fw_cb() { struct cdns_mhdp_device *mhdp = context; ... dev_dbg(mhdp->dev, "firmware callback\n"); ... } [Severity: High] This is a pre-existing issue, but does this teardown sequence leave a scheduled work vulnerable to a use-after-free? In cdns_mhdp_remove(), the driver calls flush_work() without explicitly disabling the hardware interrupt: cdns_mhdp_remove() { ... flush_work(&mhdp->hpd_work); ... } Because devm_request_threaded_irq() was used, the IRQ handler remains active until the devres teardown phase, which occurs after remove completes. If an interrupt fires between flush_work() and the devres IRQ teardown, cdns_mhdp_irq_handler() will execute and schedule the work again: cdns_mhdp_irq_handler() { ... if (bridge_attached && (sw_ev0 & CDNS_DPTX_HPD)) { schedule_work(&mhdp->hpd_work); } ... } Devres will subsequently free the IRQ and the mhdp structure, but the work remains queued and will eventually run cdns_mhdp_hpd_work(), dereferencing freed memory. [Severity: High] This isn't a bug introduced by this patch, but I noticed cdns_mhdp_bridge_atomic_duplicate_state() doesn't seem to copy current_mode. Will this break link retraining? When a new state is allocated, the subclass-specific current_mode pointer is left NULL: cdns_mhdp_bridge_atomic_duplicate_state() { struct cdns_mhdp_bridge_state *state; state = kzalloc_obj(*state); if (!state) return NULL; __drm_atomic_helper_bridge_duplicate_state(bridge, &state->base); return &state->base; } During a non-modeset atomic commit, atomic_enable is not called, so current_mode remains NULL in the new state. The old state is destroyed, permanently freeing the previous current_mode. If a hotplug event subsequently triggers a link retrain, it fetches the current state, finds current_mode is NULL, and fails with -EINVAL, silently breaking display output: cdns_mhdp_update_link_status() { ... current_mode = cdns_bridge_state->current_mode; if (!current_mode) { ret = -EINVAL; goto out; } ... } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
