linux-kernel - Re: [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during init

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]

Date:   Tue, 14 Jul 2020 16:31:54 +0530
From:   Maulik Shah <mkshah@...eaurora.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Marc Zyngier <maz@...nel.org>,
        LinusW <linus.walleij@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Evan Green <evgreen@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Srinivas Rao L <lsrao@...eaurora.org>
Subject: Re: [PATCH v3 5/5] irqchip: qcom-pdc: Reset all pdc interrupts during
 init

Hi,

On 7/14/2020 3:47 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@...eaurora.org> wrote:
>> Clear previous kernel's configuration during init by resetting
>> all interrupts in enable bank to zero.
>>
>> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
>> ---
>>   drivers/irqchip/qcom-pdc.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 8beb6f7..11a9d3a 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/types.h>
>>
>> +#define PDC_MAX_IRQS_PER_REG   32
>>   #define PDC_MAX_IRQS           168
>>   #define PDC_MAX_GPIO_IRQS      256
>>
>> @@ -339,6 +340,7 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
>>   static int pdc_setup_pin_mapping(struct device_node *np)
>>   {
>>          int ret, n;
>> +       u32 reg, max_regs, max_pins = 0;
>>
>>          n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
>>          if (n <= 0 || n % 3)
>> @@ -367,8 +369,19 @@ static int pdc_setup_pin_mapping(struct device_node *np)
>>                                                   &pdc_region[n].cnt);
>>                  if (ret)
>>                          return ret;
>> +               max_pins += pdc_region[n].cnt;
>>          }
>>
>> +       if (max_pins > PDC_MAX_IRQS)
>> +               return -EINVAL;
>> +
>> +       max_regs = max_pins / PDC_MAX_IRQS_PER_REG;
>> +       if (max_pins % PDC_MAX_IRQS_PER_REG)
>> +               max_regs++;
> nit: max_regs = DIV_ROUND_UP(max_pins, PDC_MAX_IRQS_PER_REG)
>
>
>> +       for (reg = 0; reg < max_regs; reg++)
>> +               pdc_reg_write(IRQ_ENABLE_BANK, reg, 0);
> This doesn't feel correct to me, but maybe I'm misunderstanding the
> hardware (I don't think I have access to a reference manual).  Looking
> at the example in the bindings, I see:
>
> qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
>
> In that example we have mappings for PDC ports:
> 0 - 93 (count = 94)
> 94 - 108 (count = 15)
> 115 - 121 (count = 7)
>
> Notice the slight discontinuity there.  I presume that discontinuity
> is normal / allowed?  If so, if there is enough of it then I think
> your math could be wrong, though with the example you get lucky and it
> works out OK.  It's easy to see the problem with a slightly different
> example:  Imagine that you had this:
>
> 0 - 33 (count = 34)
> 94 - 108 (count = 15)
> 115 - 121 (count = 7)
>
> ...now max_pins = 56 and max_regs = 2.  So you'll init reg 0 and 1.
> ...but (IIUC) you actually should be initting 0, 1, 2, and 3.

Right, Thanks for cacthing this. I will fix in next revision.

Thanks,
Maulik

> I have no idea what might be in those discontinuous ranges and if it's
> always OK to clear, but (assuming it is) one fix is to put your
> clearing loop _inside_ the other "for" loop in this function, AKA:
>
> for (reg = pdc_region[n].pin_base / PDC_MAX_IRQS_PER_REG;
>       reg < DIV_ROUND_UP(pdc_region[n].pin_base + pdc_region[n].cnt),
>                          PDC_MAX_IRQS_PER_REG)
>       reg++)
>
> ...or another option is to keep track of the max "pin_base + cnt" and
> loop from 0 to there?  I just don't know your hardware well enough to
> tell which would be right.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation