Re: [PATCH v9 02/22] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `cancel_work_sync(&dp->hpd_work)` causes a use-after-free of `dp` on driver removal.
- [High] Lockless TOCTOU check of `dp->bridge.dev` in `dw_dp_hpd_work` causes Use-After-Free or NULL pointer dereference on unbind.
- [High] Unhandled interrupt sources in `dw_dp_irq()` can lead to an interrupt storm.
- [High] Sleeping functions are called from an atomic context path (`atomic_disable`).
--

--- Patch [2]: [PATCH v9 02/22] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback ---
commit 639a2b7137e1fc5209a35b30731a14a632ba6684
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Subject: drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback
    
Currently the Synopsys DesignWare DP controller driver's bind function
requests lots of resources using device managed functions. These are
free'd on driver removal instead of at unbind time. Fix this discrepancy
by introducing a new probe helper function and moving over the whole
bind function. This results in a fully functional DRM bridge once probe
succeeded.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 112a499113095..06997208945ed 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1895,6 +1895,9 @@ static void dw_dp_hpd_work(struct work_struct *work)
>  	long_hpd = dp->hotplug.long_hpd;
>  	mutex_unlock(&dp->irq_lock);
>  
> +	if (!dp->bridge.dev)
> +		return;
> +

[Severity: High]
This is a pre-existing issue, but does this lockless check create a race
condition?

If drm_bridge_detach() executes concurrently and clears bridge->dev, can
this work function still proceed and pass a NULL or freed drm_device pointer
to drm_helper_hpd_irq_event()?

>  	dev_dbg(dp->dev, "[drm] Get hpd irq - %s\n", long_hpd ? "long" : "short");
>  
>  	if (!long_hpd) {

[ ... ]

> @@ -2005,9 +2027,8 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
>  
>  	dp->dev = dev;
>  	dp->pixel_mode = plat_data->pixel_mode;
> -
>  	dp->plat_data.max_link_rate = plat_data->max_link_rate;
> -	bridge = &dp->bridge;
> +
>  	mutex_init(&dp->irq_lock);
>  	INIT_WORK(&dp->hpd_work, dw_dp_hpd_work);

[Severity: High]
This is a pre-existing issue, but does this workqueue lack a cleanup step?

Since dp->hpd_work is initialized here but doesn't have a corresponding
devm action to cancel it on teardown, could a pending work item execute
after the dp structure is freed during driver unbind, resulting in a
use-after-free?

[ ... ]

[Severity: High]
This is a pre-existing issue, but can unhandled interrupt sources cause an
interrupt storm?

Looking at dw_dp_irq():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_irq() {
    ...
    if (value & HPD_EVENT)
        dw_dp_handle_hpd_event(dp);

    if (value & AUX_REPLY_EVENT) {
        regmap_write(dp->regmap, DW_DP_GENERAL_INTERRUPT, AUX_REPLY_EVENT);
        complete(&dp->complete);
    }

    return IRQ_HANDLED;
}

If an interrupt source other than HPD_EVENT or AUX_REPLY_EVENT fires (such
as HDCP_EVENT), the handler returns IRQ_HANDLED without clearing the event
in DW_DP_GENERAL_INTERRUPT. Will this keep the IRQ line asserted and
continuously re-enter the handler?

[Severity: High]
This is a pre-existing issue, but are sleeping functions being called from
atomic context?

The DRM documentation indicates that atomic disable callbacks run in atomic
context during non-blocking commits.

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bridge_atomic_disable() {
    ...
    dw_dp_reset(dp);
}

And dw_dp_reset() unconditionally calls sleeping functions:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_reset() {
    ...
    disable_irq(dp->irq);
    ...
    usleep_range(10, 20);
    ...
    regmap_read_poll_timeout(...);
}

Could this lead to kernel warnings or system instability when non-blocking
atomic commits occur?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-synopsys-dw-dp-improvements-v9-0-2e6e46135312@collabora.com?part=2