| 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: <8f6e4cc51a53f580538b879cafcd06c3@kernel.org>
Date: Tue, 08 Sep 2020 08:41:56 +0100
From: Marc Zyngier <maz@...nel.org>
To: Zhen Lei <thunder.leizhen@...wei.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Rob Herring <robh+dt@...nel.org>,
devicetree <devicetree@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
Haoyu Lv <lvhaoyu@...wei.com>, Libin <huawei.libin@...wei.com>,
Kefeng Wang <wangkefeng.wang@...wei.com>
Subject: Re: [PATCH v2 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain
On 2020-09-08 08:11, Zhen Lei wrote:
> Add support to use dw-apb-ictl as primary interrupt controller.
>
> Suggested-by: Marc Zyngier <maz@...nel.org>
> Signed-off-by: Zhen Lei <thunder.leizhen@...wei.com>
> Tested-by: Haoyu Lv <lvhaoyu@...wei.com>
> ---
> drivers/irqchip/Kconfig | 2 +-
> drivers/irqchip/irq-dw-apb-ictl.c | 75 +++++++++++++++++++++++++++++--
> 2 files changed, 73 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bfc9719dbcdc..7c2d1c8fa551 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -148,7 +148,7 @@ config DAVINCI_CP_INTC
> config DW_APB_ICTL
> bool
> select GENERIC_IRQ_CHIP
> - select IRQ_DOMAIN
> + select IRQ_DOMAIN_HIERARCHY
>
> config FARADAY_FTINTC010
> bool
> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
> b/drivers/irqchip/irq-dw-apb-ictl.c
> index aa6214da0b1f..405861322596 100644
> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> @@ -17,6 +17,7 @@
> #include <linux/irqchip/chained_irq.h>
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
> +#include <asm/exception.h>
>
> #define APB_INT_ENABLE_L 0x00
> #define APB_INT_ENABLE_H 0x04
> @@ -26,6 +27,30 @@
> #define APB_INT_FINALSTATUS_H 0x34
> #define APB_INT_BASE_OFFSET 0x04
>
> +/*
> + * irq domain of the primary interrupt controller. Currently, only one
> is
> + * supported.
By definition, there is only one primary interrupt controller.
> + */
> +static struct irq_domain *dw_apb_ictl_irq_domain;
> +
> +static void __exception_irq_entry dw_apb_ictl_handle_irq(struct
> pt_regs *regs)
> +{
> + struct irq_domain *d = dw_apb_ictl_irq_domain;
> + int n;
> +
> + for (n = 0; n < d->revmap_size; n += 32) {
> + struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, n);
> + u32 stat = readl_relaxed(gc->reg_base + APB_INT_FINALSTATUS_L);
> +
> + while (stat) {
> + u32 hwirq = ffs(stat) - 1;
> +
> + handle_domain_irq(d, hwirq, regs);
> + stat &= ~(1 << hwirq);
nit: prefer BIT(hwirq)
> + }
> + }
> +}
> +
> static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc)
> {
> struct irq_domain *d = irq_desc_get_handler_data(desc);
> @@ -50,6 +75,30 @@ static void dw_apb_ictl_handle_irq_cascaded(struct
> irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> +static int dw_apb_ictl_irq_domain_alloc(struct irq_domain *domain,
> unsigned int virq,
> + unsigned int nr_irqs, void *arg)
> +{
> + int i, ret;
> + irq_hw_number_t hwirq;
> + unsigned int type = IRQ_TYPE_NONE;
> + struct irq_fwspec *fwspec = arg;
> +
> + ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < nr_irqs; i++)
> + irq_map_generic_chip(domain, virq + i, hwirq + i);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops dw_apb_ictl_irq_domain_ops = {
> + .translate = irq_domain_translate_onecell,
> + .alloc = dw_apb_ictl_irq_domain_alloc,
> + .free = irq_domain_free_irqs_top,
> +};
> +
> #ifdef CONFIG_PM
> static void dw_apb_ictl_resume(struct irq_data *d)
> {
> @@ -78,11 +127,24 @@ static int __init dw_apb_ictl_init(struct
> device_node *np,
> const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops;
> irq_flow_handler_t flow_handler = handle_level_irq;
>
> + if (dw_apb_ictl_irq_domain) {
> + pr_err("%pOF: a hierarchy irq domain is already exist.\n", np);
> + return -EBUSY;
How can this happen?
> + }
> +
> /* Map the parent interrupt for the chained handler */
> parent_irq = irq_of_parse_and_map(np, 0);
> if (parent_irq <= 0) {
> - pr_err("%pOF: unable to parse irq\n", np);
> - return -EINVAL
Checking for an output interrupt is not the way to check for a chained
interrupt controller. That's what the parent device_node is for (no
parent or parent == self denotes a primary controller).
;
> + /* It's used as secondary interrupt controller */
> + if (of_find_property(np, "interrupts", NULL)) {
> + pr_err("%pOF: unable to parse irq\n", np);
> + return -EINVAL;
> + }
> +
> + /* It's used as the primary interrupt controller */
> + parent_irq = 0;
> + domain_ops = &dw_apb_ictl_irq_domain_ops;
> + flow_handler = handle_fasteoi_irq;
Why? This irqchip obviously doesn't support an EOI method since you
setting it to a NOP callback below. From what I understand, this
controller should use handle_level_irq, just like its chained version.
> }
>
> ret = of_address_to_resource(np, 0, &r);
> @@ -145,10 +207,17 @@ static int __init dw_apb_ictl_init(struct
> device_node *np,
> gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit;
> gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit;
> gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume;
> + if (!parent_irq)
> + gc->chip_types[0].chip.irq_eoi = irq_gc_noop;
> }
>
> - irq_set_chained_handler_and_data(parent_irq,
> + if (parent_irq) {
> + irq_set_chained_handler_and_data(parent_irq,
> dw_apb_ictl_handle_irq_cascaded, domain);
> + } else {
> + dw_apb_ictl_irq_domain = domain;
> + set_handle_irq(dw_apb_ictl_handle_irq);
> + }
>
> return 0;
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists