Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug

[sashiko-bot@kernel.org]

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

New issues:
- [High] Concurrent execution of DP link training and hardware programming between hpd_work and DRM atomic commits corrupts hardware state.
- [High] Bypassing atomic KMS for link retraining on a cable replug forcefully applies stale display timings to potentially new monitors.
- [High] Use-After-Free of the driver's private data structure due to uncanceled delayed work on driver unbind.

Pre-existing issues:
- [High] The DP lane count is permanently crippled when a display with fewer lanes is connected.
--

--- Patch [2]: [PATCH 2/2] drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug ---
commit 87e56572108019fd06745308967a4ab03655f77f
Author: Yashas D <y-d@ti.com>

drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug

When a cable is replugged while the upstream display pipeline is still
active, the bridge can retrain the DP link and re-enable the video stream
directly from the HPD interrupt work handler.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d9bd4ef8f0e21..f6f930ca1519d 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
[ ... ]
> +static void ti_sn_bridge_hpd_work(struct work_struct *work)
> +{
> +	struct ti_sn65dsi86 *pdata =
> +		container_of(work, struct ti_sn65dsi86, hpd_work);
[ ... ]
> +	ret = ti_sn_bridge_link_train(pdata, bpp, NULL);
> +	if (ret)
> +		goto notify;
> +
> +	ti_sn_bridge_set_video_timings(pdata, NULL);
> +	mutex_lock(&pdata->hpd_mutex);
> +	if (pdata->bridge_enabled)
> +		regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +				   VSTREAM_ENABLE, VSTREAM_ENABLE);
> +	mutex_unlock(&pdata->hpd_mutex);

[Severity: High]
Could this lockless invocation of ti_sn_bridge_link_train() race with a DRM
atomic commit?

If userspace reacts to the hotplug event and issues a modeset,
ti_sn_bridge_atomic_disable() clears the enabled flag but doesn't cancel the
running work. Then ti_sn_bridge_atomic_enable() executes link training
concurrently without locking.

This seems like it could cause both threads to race while programming complex DP
timing and PLL registers simultaneously, leading to hardware state machine
corruption.

[Severity: High]
Does bypassing atomic KMS for link retraining here forcefully apply stale
display timings to a potentially new monitor?

If a user unplugs the current monitor and connects a different one with a
different EDID and capabilities, forcing the old cached_mode into the
hardware could drive out-of-spec timings to the newly plugged display.

Shouldn't we rely on userspace to read the new EDID via hotplug events and
orchestrate a proper modeset?

[ ... ]
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> +				       struct drm_atomic_commit *state)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +	struct drm_connector *connector;
> +	int max_dp_lanes;
> +	unsigned int bpp;
> +	int ret;
> +
> +	connector = drm_atomic_get_new_connector_for_encoder(state,
> +							     bridge->encoder);
> +	if (!connector) {
> +		dev_err_ratelimited(pdata->dev, "Could not get the connector\n");
> +		return;
> +	}
> +
> +	max_dp_lanes = ti_sn_get_max_lanes(pdata);
> +	mutex_lock(&pdata->hpd_mutex);
> +	pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);

[Severity: High]
This is a pre-existing issue, but does this in-place min() reduction
permanently mutate the driver's maximum allowed lane capability based on the
current peripheral sink?

If a 2-lane monitor is connected, pdata->dp_lanes shrinks to 2. Connecting a
4-lane capable monitor afterward would be artificially limited to 2 lanes
since the original physical board maximum is lost.

[ ... ]
> @@ -2050,6 +2192,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>  
>  	mutex_init(&pdata->hpd_mutex);
>  	mutex_init(&pdata->comms_mutex);
> +	INIT_WORK(&pdata->hpd_work, ti_sn_bridge_hpd_work);
>  
>  	pdata->regmap = devm_regmap_init_i2c(client,
>  					     &ti_sn65dsi86_regmap_config);

[Severity: High]
Is there a chance this work item can outlive the driver's private data
structure during unbind?

Since the work is initialized here but no devm cleanup action is registered
to cancel it on teardown, if a hardware interrupt schedules the work just
before the driver is unbound, devres will free the pdata memory.

The system workqueue could then execute ti_sn_bridge_hpd_work() and
dereference the freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814120304.887993-1-y-d@ti.com?part=2