Re: [PATCH 2/2] drm/msm/dp: Return IRQ_NONE for unhandled interrupts

[Kuogee Hsieh <quic_khsieh@quicinc.com>]

On 1/19/2023 2:53 PM, Douglas Anderson wrote:
> If our interrupt handler gets called and we don't really handle the
> interrupt then we should return IRQ_NONE. The current interrupt
> handler didn't do this, so let's fix it.
>
> NOTE: for some of the cases it's clear that we should return IRQ_NONE
> and some cases it's clear that we should return IRQ_HANDLED. However,
> there are a few that fall somewhere in between. Specifically, the
> documentation for when to return IRQ_NONE vs. IRQ_HANDLED is probably
> best spelled out in the commit message of commit d9e4ad5badf4
> ("Document that IRQ_NONE should be returned when IRQ not actually
> handled"). That commit makes it clear that we should return
> IRQ_HANDLED if we've done something to make the interrupt stop
> happening.
>
> The case where it's unclear is, for instance, in dp_aux_isr() after
> we've read the interrupt using dp_catalog_aux_get_irq() and confirmed
> that "isr" is non-zero. The function dp_catalog_aux_get_irq() not only
> reads the interrupts but it also "ack"s all the interrupts that are
> returned. For an "unknown" interrupt this has a very good chance of
> actually stopping the interrupt from happening. That would mean we've
> identified that it's our device and done something to stop them from
> happening and should return IRQ_HANDLED. Specifically, it should be
> noted that most interrupts that need "ack"ing are ones that are
> one-time events and doing an "ack" is enough to clear them. However,
> since these interrupts are unknown then, by definition, it's unknown
> if "ack"ing them is truly enough to clear them. It's possible that we
> also need to remove the original source of the interrupt. In this
> case, IRQ_NONE would be a better choice.
>
> Given that returning an occasional IRQ_NONE isn't the absolute end of
> the world, however, let's choose that course of action. The IRQ
> framework will forgive a few IRQ_NONE returns now and again (and it
> won't even log them, which is why we have to log them ourselves). This
> means that if we _do_ end hitting an interrupt where "ack"ing isn't
> enough the kernel will eventually detect the problem and shut our
> device down.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>   drivers/gpu/drm/msm/dp/dp_aux.c     | 12 +++++++-----
>   drivers/gpu/drm/msm/dp/dp_aux.h     |  2 +-
>   drivers/gpu/drm/msm/dp/dp_ctrl.c    | 10 ++++++++--
>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  2 +-
>   drivers/gpu/drm/msm/dp/dp_display.c |  8 +++++---
>   5 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 34ad08ae6eb9..59e323b7499d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -368,14 +368,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>   	return ret;
>   }
>   
> -void dp_aux_isr(struct drm_dp_aux *dp_aux)
> +irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux)
>   {
>   	u32 isr;
>   	struct dp_aux_private *aux;
>   
>   	if (!dp_aux) {
>   		DRM_ERROR("invalid input\n");
> -		return;
> +		return IRQ_NONE;
>   	}
>   
>   	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> @@ -384,11 +384,11 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>   
>   	/* no interrupts pending, return immediately */
>   	if (!isr)
> -		return;
> +		return IRQ_NONE;
>   
>   	if (!aux->cmd_busy) {
>   		DRM_ERROR("Unexpected DP AUX IRQ %#010x when not busy\n", isr);
> -		return;
> +		return IRQ_NONE;
>   	}
>   
>   	/*
> @@ -420,10 +420,12 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux)
>   			aux->aux_error_num = DP_AUX_ERR_DEFER;
>   	} else {
>   		DRM_WARN("Unexpected interrupt: %#010x\n", isr);
> -		return;
> +		return IRQ_NONE;
>   	}
>   
>   	complete(&aux->comp);
> +
> +	return IRQ_HANDLED;
>   }
>   
>   void dp_aux_reconfig(struct drm_dp_aux *dp_aux)
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
> index e930974bcb5b..511305da4f66 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -11,7 +11,7 @@
>   
>   int dp_aux_register(struct drm_dp_aux *dp_aux);
>   void dp_aux_unregister(struct drm_dp_aux *dp_aux);
> -void dp_aux_isr(struct drm_dp_aux *dp_aux);
> +irqreturn_t dp_aux_isr(struct drm_dp_aux *dp_aux);
>   void dp_aux_init(struct drm_dp_aux *dp_aux);
>   void dp_aux_deinit(struct drm_dp_aux *dp_aux);
>   void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index dd26ca651a05..1a5377ef1967 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1979,27 +1979,33 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>   	return ret;
>   }
>   
> -void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
> +irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
>   {
>   	struct dp_ctrl_private *ctrl;
>   	u32 isr;
> +	irqreturn_t ret = IRQ_NONE;
>   
>   	if (!dp_ctrl)
> -		return;
> +		return IRQ_NONE;
>   
>   	ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>   
>   	isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog);
can you add (!isr) check and return IRQ_NONE here to be consistent with 
dp_aux_isr()?
>
> +
>   	if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) {
>   		drm_dbg_dp(ctrl->drm_dev, "dp_video_ready\n");
>   		complete(&ctrl->video_comp);
> +		ret = IRQ_HANDLED;
>   	}
>   
>   	if (isr & DP_CTRL_INTR_IDLE_PATTERN_SENT) {
>   		drm_dbg_dp(ctrl->drm_dev, "idle_patterns_sent\n");
>   		complete(&ctrl->idle_comp);
> +		ret = IRQ_HANDLED;
>   	}
> +
> +	return ret;
>   }
>   
>   struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 9f29734af81c..c3af06dc87b1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -25,7 +25,7 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
>   int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
>   int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
>   void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
> -void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
> +irqreturn_t dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
>   void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl);
>   struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>   			struct dp_panel *panel,	struct drm_dp_aux *aux,
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 7ff60e5ff325..8996adbc5bd3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1192,7 +1192,7 @@ static int dp_hpd_event_thread_start(struct dp_display_private *dp_priv)
>   static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
>   {
>   	struct dp_display_private *dp = dev_id;
> -	irqreturn_t ret = IRQ_HANDLED;
> +	irqreturn_t ret = IRQ_NONE;
>   	u32 hpd_isr_status;
>   
>   	if (!dp) {
> @@ -1220,13 +1220,15 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
>   
>   		if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
>   			dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> +
> +		ret = IRQ_HANDLED;
>   	}
>   
>   	/* DP controller isr */
> -	dp_ctrl_isr(dp->ctrl);
> +	ret |= dp_ctrl_isr(dp->ctrl);
>   
>   	/* DP aux isr */
> -	dp_aux_isr(dp->aux);
> +	ret |= dp_aux_isr(dp->aux);
>   
>   	return ret;
>   }