Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler

From: Stephen Boyd <sboyd@kernel.org>
To: Fenglin Wu <quic_fenglinw@quicinc.com>,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: collinsd@codeaurora.org, subbaram@codeaurora.org,
	quic_fenglinw@quicinc.com
Subject: Re: [RESEND PATCH v1 3/9] spmi: pmic-arb: check apid against limits before calling irq handler
Date: Tue, 12 Oct 2021 11:02:18 -0700	[thread overview]
Message-ID: <163406173869.936959.6395787327312518099@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <1631860384-26608-4-git-send-email-quic_fenglinw@quicinc.com>

Quoting Fenglin Wu (2021-09-16 23:32:58)
> From: David Collins <collinsd@codeaurora.org>
> 
> Check that the apid for an SPMI interrupt falls between the
> min_apid and max_apid that can be handled by the APPS processor
> before invoking the per-apid interrupt handler:
> periph_interrupt().
> 
> This avoids an access violation in rare cases where the status
> bit is set for an interrupt that is not owned by the APPS
> processor.
> 
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---

Fixes? BTW, a lot of these patches are irqchip specific. It would be
good to get review from irqchip maintainers. Maybe we should split the
irqchip driver off via the auxiliary bus so that irqchip maintainers can
review. Please Cc them on irqchip related patches.

IRQCHIP DRIVERS
M:      Thomas Gleixner <tglx@linutronix.de>
M:      Marc Zyngier <maz@kernel.org>

>  drivers/spmi/spmi-pmic-arb.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 4d7ad004..c4adc06 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -535,6 +535,12 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>                         id = ffs(status) - 1;
>                         status &= ~BIT(id);
>                         apid = id + i * 32;
> +                       if (apid < pmic_arb->min_apid
> +                           || apid > pmic_arb->max_apid) {

The || goes on the line above. What about making a local variable for
first and last and then shifting by 5 in the loop?

int first = pmic_arb->min_apid;
int last = pmic_arb->max_apid;

for (i = first >> 5; i <= last >> 5; i++)

	if (apid < first || apid > last)

> +                               WARN_ONCE(true, "spurious spmi irq received for apid=%d\n",
> +                                       apid);

Is there any way to recover from this? Or once the mapping is wrong
we're going to get interrupts that we don't know what to do with
forever?

> +                               continue;
> +                       }
>                         enable = readl_relaxed(
>                                         ver_ops->acc_enable(pmic_arb, apid));
>                         if (enable & SPMI_PIC_ACC_ENABLE_BIT)