Re: [PATCH v8 26/29] drm/msm/dp: wake threaded handler for HPD IRQs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/msm/dp: ret initialized to IRQ_HANDLED in
  msm_dp_display_irq_handler()
--

commit ee3e939cb1174f2996abe5c5ad053dbd42ebbbaa
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: wake threaded handler for HPD IRQs

This commit fixes msm_dp_display_irq_handler() to return IRQ_WAKE_THREAD
when an HPD interrupt is pending, and updates msm_dp_ctrl_isr() to report
DP_INTERRUPT_STATUS2 interrupts as IRQ_HANDLED.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 82fb3ae750f42..5623693d9242e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -3018,6 +3018,9 @@ irqreturn_t msm_dp_ctrl_isr(struct msm_dp_ctrl *msm_dp_ctrl,
>  	}
>  
>  	isr = msm_dp_ctrl_get_interrupt(ctrl);
> +	/* All status2 sources are ACKed by msm_dp_ctrl_get_interrupt(). */
> +	if (isr & DP_INTERRUPT_STATUS2)
> +		ret = IRQ_HANDLED;
>  
>  	if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) {
>  		drm_dbg_dp(ctrl->drm_dev, "dp_video_ready\n");
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8ea3d258a98f3..40b2f6e120cf0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1091,23 +1091,24 @@ static irqreturn_t msm_dp_display_irq_handler(int irq, void *dev_id)
>  	u32 hpd_isr_status;
>  	unsigned long flags;
>  	irqreturn_t ret = IRQ_HANDLED;
> +	bool hpd_irq_pending;
>  
>  	hpd_isr_status = msm_dp_aux_get_hpd_intr_status(dp->aux);
> +	hpd_irq_pending = hpd_isr_status & DP_DP_HPD_INT_MASK;
>  
> -	if (hpd_isr_status & 0x0F) {
> +	if (hpd_irq_pending) {
>  		drm_dbg_dp(dp->drm_dev, "type=%d isr=0x%x\n",
>  			dp->msm_dp_display.connector_type, hpd_isr_status);
>  
>  		spin_lock_irqsave(&dp->irq_thread_lock, flags);
>  		dp->hpd_isr_status |= hpd_isr_status;
> -		ret = IRQ_WAKE_THREAD;
>  		spin_unlock_irqrestore(&dp->irq_thread_lock, flags);
>  	}
>  
>  	/* DP controller isr */
>  	ret |= msm_dp_ctrl_isr(dp->ctrl, dp->panel);
>  
> -	return ret;
> +	return hpd_irq_pending ? IRQ_WAKE_THREAD : ret;
>  }

[Severity: Medium]
Should ret in msm_dp_display_irq_handler() be initialized to IRQ_NONE (or
assigned directly from msm_dp_ctrl_isr()) instead of IRQ_HANDLED?

Because ret starts as IRQ_HANDLED (1) and msm_dp_ctrl_isr() only returns
IRQ_NONE (0) or IRQ_HANDLED (1), "ret |= msm_dp_ctrl_isr(...)" always
evaluates to IRQ_HANDLED.  This discards the return value of
msm_dp_ctrl_isr(), makes the new DP_INTERRUPT_STATUS2 check a no-op, and
prevents the IRQ core from ever seeing IRQ_NONE on unhandled or spurious
interrupts.

Also, if ret is changed to reflect msm_dp_ctrl_isr(), should the PSR
branch in msm_dp_ctrl_isr() set ret = IRQ_HANDLED when
msm_dp_ctrl_get_psr_interrupt(ctrl) returns a non-zero status?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-msm-dp-mst-v8-0-d040fb33db5f@oss.qualcomm.com?part=26