linux-kernel - Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

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: <20180924153541.GJ17420@codeaurora.org>
Date:   Mon, 24 Sep 2018 09:35:41 -0600
From:   Lina Iyer <ilina@...eaurora.org>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     bjorn.andersson@...aro.org, sboyd@...nel.org, evgreen@...omium.org,
        linus.walleij@...aro.org, rplsssn@...eaurora.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        rnayak@...eaurora.org, devicetree@...r.kernel.org,
        andy.gross@...aro.org, dianders@...omium.org
Subject: Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only
 during suspend

On Sun, Sep 23 2018 at 03:48 -0600, Marc Zyngier wrote:
>On Sat, 22 Sep 2018 18:09:09 +0100,
>Lina Iyer <ilina@...eaurora.org> wrote:
>>
>> On Sat, Sep 22 2018 at 10:29 -0600, Marc Zyngier wrote:
>> > Hi Lina,
>> >
>> > On Tue, 04 Sep 2018 22:18:08 +0100,
>> > Lina Iyer <ilina@...eaurora.org> wrote:

>> Also, I am exploring an option that was suggested by Stephen [1] to just
>> use the PDC interrupt as a parent of the GPIO IRQ and use a different
>> irqchip for the PDC interrupt. I ran into some issue with accessing
>> irqchip and irqdata of the PDC interrupt, since the irqchip was not in
>> hierarchy with the GPIO's irqchip. I haven't been able to find time to
>> resolve the issue that the set_parent_ functions, because of the
>> hierarchy.
>>
>> Essentially, we have two different mechanisms for GPIO IRQs based on
>> whether they can be woken up by the PDC interrupt.
>>
>> 	GPIO-IRQ --> PDC --> GIC
>>
>> 	GPIO-IRQ --> TLMM SUMMARY --> GIC
>>
>> Do you think the idea is feasible? It would avoid doing all this
>> enable/disable at every suspend and even during idle, when the TLMM
>> could be powered off.
>
>[me tries to page it all in again]
>
>You could have the PDC as part of the GPIO hierarchy:
>
>	GPIO -> PDC -> TLMM -> GIC
>
The PDC irqchip's parent is the GIC as set up in
drivers/irqchip/qcom-pdc.c. Wouldn't that conflict with the established
hierarchy?

>and always configure the PDC as a wake-up source. I just wonder if you
>can do that without setting up a parallel hierarchy between the PDC
>and the GIC. We already have similar things in the tree (see OMAP's
>wakeupgen), and it may be worth having a look.
Sure, will take a  look.

>The lack of interrupt
>replaying on the PDC is quite annoying (I have much stronger words in
>mind though), and I'm not sure we can easily fix that one without this
>parallel interrupt hack (you need something to inject edge interrupts
>in the TLMM).
>
The PDC replays the intterupt at the GIC, not the at the TLMM. So the
hierachy you recommended may not work as well here.
>>
>>
>> >> +			disable_irq(irqd->irq);
>> >> +			enable_irq(irq);
>> >> +		}
>> >> +	}
>> >
>> > Given that you're changing in_suspend and parsing the bitmap,
>> > shouldn't take the pdc spinlock?
>> >
>> Since we are the the only CPU running and suspend/resume (and even idle)
>> would be serialized I didn't see a reason for needing a lock.
>
>In that case, what is the purpose of 'in_suspend' if
>msm_gpio_irq_set_wake cannot happen during the suspend/resume phases?
>It all seems a bit inconsistent.
>
Well the disable_irq_wake that I call here, calls into the set_wake
callbacks. Hence the flag to indicate that we should ignore the PDC
interrupt configuration at that time. Let me see if I need to disable
the disable_irq_wake at all.

Thanks,
Lina