Re: [PATCH v7 09/15] drm/msm/dp: re-arrange dp_display_disable() into functional parts

[sashiko-bot@kernel.org]

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

New issues:
- [High] Replacing msm_dp_ctrl_off_link_stream() with msm_dp_ctrl_off() for the disconnected monitor case inadvertently resets the DP controller, clearing HPD interrupt masks and permanently breaking dongle unplug detection.

Pre-existing issues:
- [High] The display disable sequence invokes sleeping functions from atomic context during the bridge post_disable callback.
--

commit 4edd0bb0b0901115e8cbd3c69301ff0b85ef4340
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: re-arrange dp_display_disable() into functional parts

dp_display_disable() handles special case of when monitor is
disconnected from the dongle while the dongle stays connected
thereby needing a separate function dp_ctrl_off_link_stream()
for this. However with a slight rework this can still be handled
by keeping common paths same for regular and special case.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 2cf4c984c7ca7..e1d90b8539c27 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -714,12 +714,20 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp)
>  	if (!msm_dp_display->power_on)
>  		return 0;
>  
> +	msm_dp_panel_disable_vsc_sdp(dp->panel);
> +
> +	/* dongle is still connected but sinks are disconnected */
>  	if (dp->link->sink_count == 0) {
>  		/*
>  		 * irq_hpd with sink_count = 0
>  		 * hdmi unplugged out of dongle
>  		 */
> -		msm_dp_ctrl_off_link_stream(dp->ctrl);
> +
> +		/* set dongle to D3 (power off) mode */
> +		msm_dp_link_psm_config(dp->link, &dp->panel->link_info, true);
> +		msm_dp_ctrl_off(dp->ctrl);

[Severity: High]
Does replacing msm_dp_ctrl_off_link_stream() with msm_dp_ctrl_off() here
permanently break dongle unplug detection?

msm_dp_ctrl_off() calls msm_dp_ctrl_reset(), which performs a software
reset (DP_SW_RESET) on the controller:

	sw_reset |= DP_SW_RESET;
	msm_dp_write_ahb(ctrl, REG_DP_SW_RESET, sw_reset);

This clears the controller's register state, including REG_DP_INTR_STATUS
and REG_DP_DP_HPD_INT_MASK.

Because the dongle is still connected, the driver holds a runtime PM reference
and doesn't transition through suspend/resume. Will this cause the HPD
interrupts to remain permanently masked, causing the driver to silently miss
subsequent dongle unplug events since msm_dp_ctrl_enable_irq() is never called
again?


[Severity: High]
This isn't a bug introduced by this patch, but does calling msm_dp_ctrl_off()
here invoke sleeping functions from atomic context?

msm_dp_display_disable() is invoked from the bridge atomic disable path, which
runs in atomic context where sleeping is forbidden. However, msm_dp_ctrl_off()
calls msm_dp_ctrl_reset(), which executes:

	usleep_range(1000, 1100);

Is this safe to run in atomic commit paths?

> +		/* re-init the PHY so that we can listen to Dongle disconnect */
> +		msm_dp_ctrl_reinit_phy(dp->ctrl);

[Severity: High]
This is a pre-existing issue, but does calling msm_dp_ctrl_reinit_phy()
here also invoke sleeping functions from atomic context?

msm_dp_ctrl_reinit_phy() calls generic PHY framework functions (phy_exit()
and phy_init()) that acquire mutexes internally, which is forbidden in atomic
context.

>  	} else {
>  		/*
>  		 * unplugged interrupt

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=9