linux-kernel - Re: [PATCH v2 4/4] irqchip: qcom-pdc: Introduce irq_set

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: <159057454795.88029.5963412495484312088@swboyd.mtv.corp.google.com>
Date:   Wed, 27 May 2020 03:15:47 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>, bjorn.andersson@...aro.org,
        evgreen@...omium.org, linus.walleij@...aro.org, maz@...nel.org,
        mka@...omium.org
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-gpio@...r.kernel.org, agross@...nel.org, tglx@...utronix.de,
        jason@...edaemon.net, dianders@...omium.org, rnayak@...eaurora.org,
        ilina@...eaurora.org, lsrao@...eaurora.org,
        Maulik Shah <mkshah@...eaurora.org>
Subject: Re: [PATCH v2 4/4] irqchip: qcom-pdc: Introduce irq_set_wake call

Quoting Maulik Shah (2020-05-23 10:11:13)
> 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 | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index 6ae9e1f..f7c0662 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -36,6 +36,7 @@ struct pdc_pin_region {
>         u32 cnt;
>  };
>  
> +DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);

static?

>  static DEFINE_RAW_SPINLOCK(pdc_lock);
>  static void __iomem *pdc_base;
>  static struct pdc_pin_region *pdc_region;
> @@ -87,22 +88,20 @@ 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);
> -}
> +               return 0;

Shouldn't this fail if we can't set for wake?

>  
> -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)

The diff is really hard to read too. Maybe set_wake can be added first
and then the enable/disable functions removed?

> @@ -110,6 +109,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
>         if (d->hwirq == GPIO_NO_WAKE_IRQ)
>                 return;
>  
> +       if (!test_bit(d->hwirq, pdc_wake_irqs))
> +               pdc_enable_intr(d, false);
> +
>         irq_chip_mask_parent(d);
>  }
>  
> @@ -118,6 +120,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);
>  }
>  

I find these two hunks deeply confusing. I'm not sure what the
maintainers think though. I hope it would be simpler to always enable
the hwirqs in the pdc when an irq is requested and only disable it in
the pdc when the system goes to suspend and the pdc pin isn't for an irq
that's marked for wakeup. Does that break somehow?

My understanding of the hardware is that the GPIO controller has lines
directly connected to various SPI lines on the GIC and PDC has a way to
monitor those direct connections and wakeup the CPUs when they trigger
the detection logic in the PDC. The enable/disable bit in PDC gates that
logic for each wire between the GPIO controller and the GIC.

So isn't it simpler to leave the PDC monitoring pins that we care about
all the time and only stop monitoring when we enter and leave suspend?
And shouldn't the driver set something sane in qcom_pdc_init() to
disable all the pdc pins so that we don't rely on boot state to
configure pins for wakeup?

> @@ -197,15 +200,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,