Re: [PATCH v8 08/21] drm/bridge: synopsys: dw-dp: Free output_fmts when none are valid

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbalanced phy_power_off() and regulator_disable() if phy_power_on() fails in dw_dp_link_enable().
- [High] NULL pointer dereference in dw_dp_hpd_work() and dw_dp_link_retrain() due to missing bridge.dev check.
--

--- Patch [8]: [PATCH v8 08/21] drm/bridge: synopsys: dw-dp: Free output_fmts when none are valid ---
commit 749c8cb81f0227b8f3795cdf2bcba3b9e0038179
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Free output_fmts when none are valid

If dw_dp_bandwidth_ok() returns false for all formats, *num_output_fmts
might end up becoming 0. This commit frees output_fmts within the function
in that case to avoid leaking memory.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 760e1e0f8234b..7127928c26506 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1804,6 +1804,11 @@ static u32 *dw_dp_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>  		output_fmts[j++] = fmt->bus_format;
>  	}
>  
> +	if (j == 0) {
> +		kfree(output_fmts);
> +		output_fmts = NULL;
> +	}
> +
>  	*num_output_fmts = j;
>  
>  	return output_fmts;

[Severity: High]
This isn't a bug introduced by this patch, but could there be an unbalanced
phy_power_off() if phy_power_on() fails during dw_dp_link_enable()?

In dw_dp_link_enable(), if phy_power_on() fails:

    ret = phy_power_on(dp->phy);
    if (ret)
        return ret;

It returns without incrementing the PHY's power_count. However, if the DRM
core continues tracking the bridge as enabled, it will eventually call
dw_dp_link_disable() when the bridge is disabled.

dw_dp_link_disable() does:

    phy_power_off(dp->phy);

Because the power_count was not incremented earlier, does this cause
phy_power_off() to decrement it to -1 and unconditionally call
regulator_disable(), potentially dropping power for other shared devices?

[Severity: High]
This is also a pre-existing issue, but can dw_dp_hpd_work() or
dw_dp_link_retrain() trigger a NULL pointer dereference if the bridge is
detached?

If the DRM encoder is unbound, drm_bridge_detach() explicitly sets
bridge->dev = NULL. If a physical hotplug event occurs before the bridge
device itself is unbound, the IRQ can fire and schedule dw_dp_hpd_work().

In dw_dp_hpd_work():

    drm_helper_hpd_irq_event(dp->bridge.dev);

And in dw_dp_link_retrain():

    struct drm_device *dev = dp->bridge.dev;
    ...
    ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);

Since neither function checks if dp->bridge.dev is NULL, could a hotplug
event after encoder unbind cause a kernel panic here?

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