Re: [PATCH v6 24/29] drm/msm/dp: add HPD callback for dp MST

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Use-After-Free of `mst` structure in MST HPD IRQ handler after DRM component unbind.
- [High] MST HPD IRQ handler reads ESI exactly once (no loop) and ignores link status updates (ESI[3]), breaking link recovery.
- [Medium] TOCTOU data race on `dp->plugged` and `mst_active` causes dropped hotplug events.
--

commit 72fee25edd59966ac66b4a97ac13198745881f5c
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>
    
drm/msm/dp: add HPD callback for dp MST
    
This patch adds HPD callback handling for the MST module, which is invoked from
the dp_display's HPD handler. It routes HPD messages to the MST module when
MST is active.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index c015af8688013..22b9252774620 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -416,6 +416,9 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
>  			dp->msm_dp_display.connector_type,
>  			dp->link->sink_count);
>  
> +	if (dp->plugged && dp->msm_dp_display.mst_active)
> +		return 0;
> +
>  	guard(mutex)(&dp->plugged_lock);

[Severity: Medium]
Could reading dp->plugged and mst_active outside of dp->plugged_lock cause a
race condition?

If a rapid plug/unplug sequence occurs, msm_dp_hpd_plug_handle() and
msm_dp_hpd_unplug_handle() could execute concurrently. If the unplug handler
holds the lock, the plug handler might see dp->plugged as true and return
early, skipping initialization. After the unplug handler finishes and sets
dp->plugged to false, the display would be physically plugged in but
uninitialized in software.

>  
>  	ret = pm_runtime_resume_and_get(&pdev->dev);
[ ... ]
> @@ -506,12 +509,18 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
>  {
>  	u32 sink_request;
>  	int rc = 0;
> +	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
>  
>  	/* irq_hpd can happen at either connected or disconnected state */
>  	drm_dbg_dp(dp->drm_dev, "Before, type=%d, sink_count=%d\n",
>  			dp->msm_dp_display.connector_type,
>  			dp->link->sink_count);
>  
> +	if (msm_dp_display->mst_active) {
> +		msm_dp_mst_display_hpd_irq(&dp->msm_dp_display);
> +		return 0;
> +	}
> +

[Severity: High]
Does returning early here when MST is active bypass standard DP link loss and
test request processing?

In SST mode, msm_dp_link_process_request() handles DP link service IRQ
vectors (like link loss). When mst_active is true, this path is entirely
skipped, and it does not appear that msm_dp_mst_display_hpd_irq() handles
link status updates.

>  	/* check for any test request issued by sink */
>  	rc = msm_dp_link_process_request(dp->link);
>  	if (!rc) {
[ ... ]
> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index 03f9c95ca6860..2e625a7db9819 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
[ ... ]
> @@ -326,6 +328,39 @@ int msm_dp_mst_attach_encoder(struct msm_dp *dp_display, unsigned int stream_id,
>  	return 0;
>  }
>  
> +void msm_dp_mst_display_hpd_irq(struct msm_dp *dp_display)
> +{
> +	int rc;
> +	struct msm_dp_mst *mst = dp_display->msm_dp_mst;
> +	u8 ack[8] = {};
> +	u8 esi[4];
> +	unsigned int esi_res = DP_SINK_COUNT_ESI + 1;
> +	bool handled;
> +
> +	guard(mutex)(&mst->hpd_irq_lock);

[Severity: Critical]
Can this dereference a freed mst pointer if the DRM master is unbound?

The mst structure is allocated with devm_kzalloc(dev->dev, ...) during
initialization. If the DRM master module is unloaded, devres will free this
memory. 

However, if the DP IRQ was requested on the component device and remains
active, and mst_active isn't cleared, an incoming HPD interrupt could be
routed here, causing a use-after-free when locking mst->hpd_irq_lock.

> +
> +	rc = drm_dp_dpcd_read_data(mst->dp_aux, DP_SINK_COUNT_ESI, esi, 4);
> +	if (rc < 0) {
> +		DRM_ERROR("DPCD sink status read failed, rlen=%d\n", rc);
> +		return;
> +	}
> +
> +	drm_dbg_dp(dp_display->drm_dev, "MST HPD IRQ: esi=%*ph\n", 4, esi);
> +
> +	rc = drm_dp_mst_hpd_irq_handle_event(&mst->mst_mgr, esi, ack, &handled);
> +
> +	if (handled) {
> +		rc = drm_dp_dpcd_write_byte(mst->dp_aux, esi_res, ack[1]);

[Severity: High]
Does this logic ignore link status updates and subsequent ESI messages?

The DP specification requires reading the ESI repeatedly until all status bits
are clear. This function reads the ESI exactly once, ignoring any subsequent
messages.

Additionally, this function only passes esi to
drm_dp_mst_hpd_irq_handle_event(). Because the legacy link processing in
msm_dp_irq_hpd_handle() is completely bypassed when mst_active is true,
esi[3] (DP_LINK_SERVICE_IRQ_VECTOR_ESI0) is entirely ignored. This means
the source could fail to detect link loss during MST operation and fail to
retrain the link.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-msm-dp-mst-v6-0-c91d35d6fb9e@oss.qualcomm.com?part=24