linux-kernel - Re: [PATCH V1 09/15] spmi: pmic-arb: check apid enabled before calling the handler

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: <20170531203909.GG20170@codeaurora.org>
Date:   Wed, 31 May 2017 13:39:09 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Kiran Gunda <kgunda@...eaurora.org>
Cc:     Abhijeet Dharmapurikar <adharmap@...eaurora.org>,
        David Collins <collinsd@...eaurora.org>,
        Subbaraman Narayanamurthy <subbaram@...eaurora.org>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        adharmap@...cinc.com, aghayal@....qualcomm.com
Subject: Re: [PATCH V1 09/15] spmi: pmic-arb: check apid enabled before
 calling the handler

On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
> 
> The driver currently invokes the apid handler (periph_handler())

You mean periph_interrupt()?

> once it sees that the summary status bit for that apid is set.
> 
> However the hardware is designed to set that bit even if the apid
> interrupts are disabled. The driver should check whether the apid
> is indeed enabled before calling the apid handler.

Really? Wow that is awful. Or is this because ACC_ENABLE bit is
always set now and never cleared?

> 
> Signed-off-by: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
> Signed-off-by: Kiran Gunda <kgunda@...eaurora.org>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index ad34491..f8638fa 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -536,8 +536,8 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>  	void __iomem *intr = pa->intr;
>  	int first = pa->min_apid >> 5;
>  	int last = pa->max_apid >> 5;
> -	u32 status;
> -	int i, id;
> +	u32 status, enable;
> +	int i, id, apid;
>  
>  	chained_irq_enter(chip, desc);
>  
> @@ -547,7 +547,11 @@ static void pmic_arb_chained_irq(struct irq_desc *desc)
>  		while (status) {
>  			id = ffs(status) - 1;
>  			status &= ~BIT(id);
> -			periph_interrupt(pa, id + i * 32);
> +			apid = id + i * 32;
> +			enable = readl_relaxed(intr +
> +					pa->ver_ops->acc_enable(apid));

Do we need to read the hardware to figure this out? After earlier
patches in this series we would never clear the
SPMI_PIC_ACC_ENABLE_BIT after one of the irqs in a peripheral is
unmasked for the first time (which looks to be fixing a bug in
the existing driver BTW). So in practice, this should almost
always be true.

In the one case that it isn't true, we'll be handling some other
irq for another peripheral and then hardware will tell us there's
an interrupt for a peripheral that doesn't have any interrupts
unmasked. We would call periph_interrupt() and then that
shouldn't see any interrupts in the status register for that
APID. So we do some more work, but nothing happens still. Did I
miss something? What is this fixing?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project