lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5c49a2d2-2c67-502c-ca1a-5c3acddd5797@quicinc.com>
Date:   Mon, 30 Jan 2023 09:58:51 -0800
From:   Kuogee Hsieh <quic_khsieh@...cinc.com>
To:     Douglas Anderson <dianders@...omium.org>,
        Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC:     Stephen Boyd <swboyd@...omium.org>,
        Alex Deucher <alexander.deucher@....com>,
        Bjorn Andersson <quic_bjorande@...cinc.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        "Javier Martinez Canillas" <javierm@...hat.com>,
        Johan Hovold <johan+linaro@...nel.org>,
        Lyude Paul <lyude@...hat.com>,
        "Sankeerth Billakanti" <quic_sbillaka@...cinc.com>,
        Sean Paul <sean@...rly.run>,
        "Thomas Zimmermann" <tzimmermann@...e.de>,
        <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] drm/msm/dp: Return IRQ_NONE for unhandled
 interrupts


On 1/26/2023 5:09 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@...omium.org>
Tested-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
Reviewed-by: Kuogee Hsieh <quic_khsieh@...cinc.com>

> ---
>
> (no changes since v1)
>
>   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 84f9e3e5f964..8e3b677f35e6 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_NONE;
>   	} 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);
>   
> +
>   	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 bde1a7ce442f..b5343c9f1c1e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1204,7 +1204,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) {
> @@ -1232,13 +1232,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;
>   }

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ