Re: [PATCH v8 04/21] drm/bridge: synopsys: dw-dp: Document missing reset line deassert

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The bridge subclasses the atomic state (`struct dw_dp_bridge_state`) but fails to implement `.atomic_create_state` correctly and does not fully copy subclass fields during `.atomic_duplicate_state`.
--

--- Patch [4]: [PATCH v8 04/21] drm/bridge: synopsys: dw-dp: Document missing reset line deassert ---
commit eae7b13a259bfd47bfead9cca99a86275cf00d9a
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

    drm/bridge: synopsys: dw-dp: Document missing reset line deassert
    
    If the driver uses devm_reset_control_get_exclusive_deasserted() instead
    of devm_reset_control_get() and thus automatically deasserts during
    probe, the SoC will hang when the device is unbound.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 6289ed06e2317..a8ae13cf91d60 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -2076,6 +2076,10 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  		return ERR_CAST(dp->hdcp_clk);
>  	}
>  
> +	/*
> +	 * This reset line is deasserted by default; asserting it hangs the SoC if the
> +	 * related power-domain is still active.
> +	 */
>  	dp->rstc = devm_reset_control_get(dev, NULL);
>  	if (IS_ERR(dp->rstc)) {
>  		dev_err_probe(dev, PTR_ERR(dp->rstc), "failed to get reset control\n");

While reviewing this, I noticed a pre-existing issue in the same file:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:
> static struct drm_bridge_state *dw_dp_bridge_atomic_duplicate_state(struct drm_bridge *bridge)
> {
> 	struct dw_dp_bridge_state *state;
> 
> 	state = kzalloc_obj(*state);
> 	if (!state)
> 		return NULL;
> 
> 	__drm_atomic_helper_bridge_duplicate_state(bridge, &state->base);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does this correctly copy
subclass fields?

The generic helper __drm_atomic_helper_bridge_duplicate_state() only copies
the base struct size, failing to carry over the subclass-specific fields
to the newly allocated state.

[ ... ]

> static const struct drm_bridge_funcs dw_dp_bridge_funcs = {
> 	.attach = dw_dp_bridge_attach,
> 	.detach = dw_dp_bridge_detach,
> 	.atomic_duplicate_state = dw_dp_bridge_atomic_duplicate_state,
> 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> 	.atomic_create_state = drm_atomic_helper_bridge_create_state,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is also a pre-existing issue, but is the initial state allocation large
enough for the subclass?

When drm_mode_config_reset() creates the initial atomic state using the
generic helper, the allocated object is only sizeof(struct drm_bridge_state).

If a hotplug event occurs before the first modeset, dw_dp_hpd_work() calls
dw_dp_link_retrain() -> dw_dp_link_downgrade(), which fetches the initial
state via dw_dp_get_bridge_state() and reads subclass fields like state->mode
and state->bpp.

Could this result in a heap out-of-bounds read because the custom subclass
state wasn't fully allocated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-synopsys-dw-dp-improvements-v8-0-ac1e6a75782f@collabora.com?part=4