| 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
| ||
|
Message-ID: <339f24ce-8114-2008-87d4-0e3772487f10@codeaurora.org>
Date: Tue, 14 Jul 2020 16:24:15 +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 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call
Hi,
On 7/14/2020 3:46 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@...eaurora.org> wrote:
>> Remove irq_disable callback to allow lazy disable for pdc interrupts.
>>
>> Add irq_set_wake callback that unmask interrupt in HW when drivers
>> mark interrupt for wakeup. Interrupt will be cleared in HW during
>> lazy disable if its not marked for wakeup.
>>
>> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
>> ---
>> drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
>> 1 file changed, 16 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 6ae9e1f..8beb6f7 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -36,6 +36,7 @@ struct pdc_pin_region {
>> u32 cnt;
>> };
>>
>> +static DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
>> static DEFINE_RAW_SPINLOCK(pdc_lock);
>> static void __iomem *pdc_base;
>> static struct pdc_pin_region *pdc_region;
>> @@ -87,22 +88,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>> raw_spin_unlock(&pdc_lock);
>> }
>>
>> -static void qcom_pdc_gic_disable(struct irq_data *d)
>> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
>> {
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> -
>> - pdc_enable_intr(d, false);
>> - irq_chip_disable_parent(d);
>> -}
>> -
>> -static void qcom_pdc_gic_enable(struct irq_data *d)
>> -{
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> + if (on) {
>> + pdc_enable_intr(d, true);
>> + irq_chip_enable_parent(d);
>> + set_bit(d->hwirq, pdc_wake_irqs);
>> + } else {
>> + clear_bit(d->hwirq, pdc_wake_irqs);
>> + }
>>
>> - pdc_enable_intr(d, true);
>> - irq_chip_enable_parent(d);
>> + return irq_chip_set_wake_parent(d, on);
>> }
>>
>> static void qcom_pdc_gic_mask(struct irq_data *d)
>> @@ -111,6 +107,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
>> return;
>>
>> irq_chip_mask_parent(d);
>> +
>> + if (!test_bit(d->hwirq, pdc_wake_irqs))
>> + pdc_enable_intr(d, false);
> I _think_ this will break masking, right? In other words, consider
> the following (having nothing to do with suspend/resume):
>
> 1. Driver requests an interrupt.
> 2. Driver masks interrupt (calls disable_irq())
> 3. Interrupt fires while it is masked.
> 4. Driver unmasks interrupt (calls enable_irq().
>
> After step #4 the interrupt should fire since it was only masked, not
> disabled (yes, it's super confusing that the driver calls
> disable_irq() but it expecting it to be masked--as I understand it
> that's just how it is). I haven't tested, but I suspect that's broken
> for you now (assuming you're working on a pin that wasn't a wakeup
> pin) because you won't track edges when you're "disabled".
No its not broken, it works as expected. after step #4, interrupt will fire.
>
> I suspect that the right thing to do here is to:
>
> a) Make qcom_pdc_gic_set_wake() just keep "pdc_wake_irqs" up to date
> and then call parent.
>
> b) Implement irq_suspend and irq_resume. In irq_suspend() you disable
> all interrupts that aren't in "pdc_wake_irqs". In irq_resume() you
> just re-enable all of them (masking will be handled by the parent).
>
> Would that work?
>
> ...oh, drat! The .irq_suspend() callback is only there if you're
> using "irq/generic-chip.c". OK, well unless we want to move over to
> using generic-chip we can just register for syscore ourselves. OK, I
> tested and <https://crrev.com/c/2296160> works.
I too thought of using syscore ops earlier, but syscore ops won't work
if device chooses to enter "s2idle" suspend state since they are not
invoked in s2idle entry path.
even if you register for "irq/generic-chip.c" , this driver too
registers for syscore ops only, it won't work if you enter s2idle
suspend state.
Current patch works fine with both s2idle and deep suspend states.
Thanks,
Maulik
>
>
>
>> }
>>
>> static void qcom_pdc_gic_unmask(struct irq_data *d)
>> @@ -118,6 +117,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
>> if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> return;
>>
>> + pdc_enable_intr(d, true);
>> irq_chip_unmask_parent(d);
>> }
>>
>> @@ -197,15 +197,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
>> .irq_eoi = irq_chip_eoi_parent,
>> .irq_mask = qcom_pdc_gic_mask,
>> .irq_unmask = qcom_pdc_gic_unmask,
>> - .irq_disable = qcom_pdc_gic_disable,
>> - .irq_enable = qcom_pdc_gic_enable,
>> .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
>> .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
>> .irq_retrigger = irq_chip_retrigger_hierarchy,
>> .irq_set_type = qcom_pdc_gic_set_type,
>> + .irq_set_wake = qcom_pdc_gic_set_wake,
>> .flags = IRQCHIP_MASK_ON_SUSPEND |
>> - IRQCHIP_SET_TYPE_MASKED |
>> - IRQCHIP_SKIP_SET_WAKE,
>> + IRQCHIP_SET_TYPE_MASKED,
>> .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent,
>> .irq_set_affinity = irq_chip_set_affinity_parent,
>> };
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Powered by blists - more mailing lists