Re: [PATCH v7 01/18] 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 3 potential issue(s) to consider:

New issues:
- [High] Premature global registration of the DRM bridge before resource initialization.
- [High] Incorrect teardown order in `dw_dp_unbind()` causes hardware access after PHY shutdown.

Pre-existing issues:
- [High] Missing cancellation of asynchronous hotplug work leads to Use-After-Free.
--

--- Patch [1]: [PATCH v7 01/18] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback ---
commit d953a018bc82561ebc8b90aad3a536efae9649c9
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Date:   Tue Jul 28 17:40:37 2026 +0000

    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 most of the
    early resource acquisitions.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 3445c82e6f50e..67d9cd0bf5fd4 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> +void dw_dp_unbind(struct dw_dp *dp)
> +{
> +	disable_irq(dp->irq);
>  	phy_exit(dp->phy);
> +	drm_dp_aux_unregister(&dp->aux);
>  }

[Severity: High]
Is this teardown order correct? Calling phy_exit() before
drm_dp_aux_unregister() powers down the PHY while the AUX device is still
registered.

If an active AUX transfer is waiting for completion in dw_dp_aux_transfer(),
disable_irq() will prevent the completion from firing, causing a timeout. When
it wakes up, it will attempt to read hardware registers. Because the PHY has
already been powered down, wouldn't accessing these registers trigger a bus
error?

> +EXPORT_SYMBOL_GPL(dw_dp_unbind);
>  
> -struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
> -			 const struct dw_dp_plat_data *plat_data)
> +struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_data *plat_data)
>  {

[ ... ]

>  	dp->dev = dev;
>  	dp->pixel_mode = plat_data->pixel_mode;
> -
>  	dp->plat_data.max_link_rate = plat_data->max_link_rate;
> +
>  	bridge = &dp->bridge;
> +	bridge->of_node = dev->of_node;
> +	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
> +	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
> +	bridge->ycbcr_420_allowed = true;
> +
> +	ret = devm_drm_bridge_add(dev, bridge);
> +	if (ret)
> +		return ERR_PTR(ret);
> +

[Severity: High]
Is it safe to register the bridge here?

By calling devm_drm_bridge_add() early in dw_dp_probe() before resources like
dp->regmap, dp->phy, and the dp->aux adapter are initialized, this makes the
bridge globally visible to other drivers prematurely.

If a concurrent driver discovers the bridge and initiates operations like
drm_bridge_edid_read(), could it attempt to use the uninitialized dp->aux
adapter and crash?

>  	mutex_init(&dp->irq_lock);
>  	INIT_WORK(&dp->hpd_work, dw_dp_hpd_work);

[Severity: High]
This is a pre-existing issue, but is dp->hpd_work cancelled anywhere during
driver teardown?

The work struct is initialized and tied to the lifetime of the dp object, but
neither dw_dp_unbind() nor the remove function seem to call cancel_work_sync().

Could this lead to a use-after-free if the workqueue continues executing and
accesses the freed dp memory after the device is unbound?

>  	init_completion(&dp->complete);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-synopsys-dw-dp-improvements-v7-0-b7640fa8cf48@collabora.com?part=1