| 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: <9ab0b6ca-c515-a653-65c0-005af782da23@loongson.cn>
Date: Fri, 8 Jul 2022 14:13:02 +0800
From: Jianmin Lv <lvjianmin@...ngson.cn>
To: Marc Zyngier <maz@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
Hanjun Guo <guohanjun@...wei.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V14 10/15] irqchip/loongson-liointc: Add ACPI init support
On 2022/7/7 下午6:34, Marc Zyngier wrote:
> On Sun, 03 Jul 2022 09:45:27 +0100,
> Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>>
>> From: Huacai Chen <chenhuacai@...ngson.cn>
>>
>> LIOINTC stands for "Legacy I/O Interrupts" that described in Section
>> 11.1 of "Loongson 3A5000 Processor Reference Manual". For more
>> information please refer Documentation/loongarch/irq-chip-model.rst.
>>
>> Co-developed-by: Jianmin Lv <lvjianmin@...ngson.cn>
>> Signed-off-by: Jianmin Lv <lvjianmin@...ngson.cn>
>> Signed-off-by: Huacai Chen <chenhuacai@...ngson.cn>
>> ---
>> arch/loongarch/include/asm/irq.h | 4 +-
>> arch/loongarch/kernel/irq.c | 1 -
>> drivers/irqchip/irq-loongson-liointc.c | 227 ++++++++++++++++++++++-----------
>> 3 files changed, 154 insertions(+), 78 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
>> index 8775dc6..b4c7956 100644
>> --- a/arch/loongarch/include/asm/irq.h
>> +++ b/arch/loongarch/include/asm/irq.h
>> @@ -97,7 +97,7 @@ static inline void eiointc_enable(void)
>>
>> struct irq_domain *loongarch_cpu_irq_init(void);
>>
>> -struct irq_domain *liointc_acpi_init(struct irq_domain *parent,
>> +int liointc_acpi_init(struct irq_domain *parent,
>> struct acpi_madt_lio_pic *acpi_liointc);
>> struct irq_domain *eiointc_acpi_init(struct irq_domain *parent,
>> struct acpi_madt_eio_pic *acpi_eiointc);
>> @@ -122,7 +122,7 @@ int pch_pic_acpi_init(struct irq_domain *parent,
>> extern struct acpi_madt_bio_pic *acpi_pchpic[MAX_IO_PICS];
>>
>> extern struct irq_domain *cpu_domain;
>> -extern struct irq_domain *liointc_domain;
>> +extern struct fwnode_handle *liointc_handle;
>> extern struct fwnode_handle *pch_lpc_handle;
>> extern struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
>>
>> diff --git a/arch/loongarch/kernel/irq.c b/arch/loongarch/kernel/irq.c
>> index ce21281..b04201c 100644
>> --- a/arch/loongarch/kernel/irq.c
>> +++ b/arch/loongarch/kernel/irq.c
>> @@ -26,7 +26,6 @@
>> EXPORT_PER_CPU_SYMBOL(irq_stat);
>>
>> struct irq_domain *cpu_domain;
>> -struct irq_domain *liointc_domain;
>>
>> /*
>> * 'what should we do if we get a hw irq event on an illegal vector'.
>> diff --git a/drivers/irqchip/irq-loongson-liointc.c b/drivers/irqchip/irq-loongson-liointc.c
>> index 8d05d8b..526ade4 100644
>> --- a/drivers/irqchip/irq-loongson-liointc.c
>> +++ b/drivers/irqchip/irq-loongson-liointc.c
>> @@ -23,7 +23,7 @@
>> #endif
>>
>> #define LIOINTC_CHIP_IRQ 32
>> -#define LIOINTC_NUM_PARENT 4
>> +#define LIOINTC_NUM_PARENT 4
>> #define LIOINTC_NUM_CORES 4
>>
>> #define LIOINTC_INTC_CHIP_START 0x20
>> @@ -58,6 +58,8 @@ struct liointc_priv {
>> bool has_lpc_irq_errata;
>> };
>>
>> +struct fwnode_handle *liointc_handle;
>> +
>> static void liointc_chained_handle_irq(struct irq_desc *desc)
>> {
>> struct liointc_handler_data *handler = irq_desc_get_handler_data(desc);
>> @@ -153,97 +155,82 @@ static void liointc_resume(struct irq_chip_generic *gc)
>> irq_gc_unlock_irqrestore(gc, flags);
>> }
>>
>> -static const char * const parent_names[] = {"int0", "int1", "int2", "int3"};
>> -static const char * const core_reg_names[] = {"isr0", "isr1", "isr2", "isr3"};
>> +static int parent_irq[LIOINTC_NUM_PARENT];
>> +static u32 parent_int_map[LIOINTC_NUM_PARENT];
>> +static const char *const parent_names[] = {"int0", "int1", "int2", "int3"};
>> +static const char *const core_reg_names[] = {"isr0", "isr1", "isr2", "isr3"};
>>
>> -static void __iomem *liointc_get_reg_byname(struct device_node *node,
>> - const char *name)
>> +#ifdef CONFIG_ACPI
>> +static int liointc_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
>> + const u32 *intspec, unsigned int intsize,
>> + unsigned long *out_hwirq, unsigned int *out_type)
>> {
>> - int index = of_property_match_string(node, "reg-names", name);
>> -
>> - if (index < 0)
>> - return NULL;
>> -
>> - return of_iomap(node, index);
>> + if (WARN_ON(intsize < 1))
>> + return -EINVAL;
>> + *out_hwirq = intspec[0] - GSI_MIN_CPU_IRQ;
>> + *out_type = IRQ_TYPE_NONE;
>> + return 0;
>> }
>>
>> -static int __init liointc_of_init(struct device_node *node,
>> - struct device_node *parent)
>> +const struct irq_domain_ops acpi_irq_gc_ops = {
>
> This should be static.
>
Ok, thanks, I'll change it.
>> + .map = irq_map_generic_chip,
>> + .unmap = irq_unmap_generic_chip,
>> + .xlate = liointc_domain_xlate,
>> +};
>> +#endif
>> +
>> +static int liointc_init(phys_addr_t addr, unsigned long size, int revision,
>> + struct fwnode_handle *domain_handle, struct device_node *node)
>> {
>> + int i, err;
>> + void __iomem *base;
>> + struct irq_chip_type *ct;
>> struct irq_chip_generic *gc;
>> struct irq_domain *domain;
>> - struct irq_chip_type *ct;
>> struct liointc_priv *priv;
>> - void __iomem *base;
>> - u32 of_parent_int_map[LIOINTC_NUM_PARENT];
>> - int parent_irq[LIOINTC_NUM_PARENT];
>> - bool have_parent = FALSE;
>> - int sz, i, err = 0;
>>
>> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> if (!priv)
>> return -ENOMEM;
>>
>> - if (of_device_is_compatible(node, "loongson,liointc-2.0")) {
>> - base = liointc_get_reg_byname(node, "main");
>> - if (!base) {
>> - err = -ENODEV;
>> - goto out_free_priv;
>> - }
>> + base = ioremap(addr, size);
>> + if (!base)
>> + goto out_free_priv;
>>
>> - for (i = 0; i < LIOINTC_NUM_CORES; i++)
>> - priv->core_isr[i] = liointc_get_reg_byname(node, core_reg_names[i]);
>> - if (!priv->core_isr[0]) {
>> - err = -ENODEV;
>> - goto out_iounmap_base;
>> - }
>> - } else {
>> - base = of_iomap(node, 0);
>> - if (!base) {
>> - err = -ENODEV;
>> - goto out_free_priv;
>> - }
>> + for (i = 0; i < LIOINTC_NUM_CORES; i++)
>> + priv->core_isr[i] = base + LIOINTC_REG_INTC_STATUS;
>>
>> - for (i = 0; i < LIOINTC_NUM_CORES; i++)
>> - priv->core_isr[i] = base + LIOINTC_REG_INTC_STATUS;
>> - }
>> + for (i = 0; i < LIOINTC_NUM_PARENT; i++)
>> + priv->handler[i].parent_int_map = parent_int_map[i];
>>
>> - for (i = 0; i < LIOINTC_NUM_PARENT; i++) {
>> - parent_irq[i] = of_irq_get_byname(node, parent_names[i]);
>> - if (parent_irq[i] > 0)
>> - have_parent = TRUE;
>> - }
>> - if (!have_parent) {
>> - err = -ENODEV;
>> - goto out_iounmap_isr;
>> - }
>> + if (revision > 1) {
>> + for (i = 0; i < LIOINTC_NUM_CORES; i++) {
>> + int index = of_property_match_string(node,
>> + "reg-names", core_reg_names[i]);
>>
>> - sz = of_property_read_variable_u32_array(node,
>> - "loongson,parent_int_map",
>> - &of_parent_int_map[0],
>> - LIOINTC_NUM_PARENT,
>> - LIOINTC_NUM_PARENT);
>> - if (sz < 4) {
>> - pr_err("loongson-liointc: No parent_int_map\n");
>> - err = -ENODEV;
>> - goto out_iounmap_isr;
>> - }
>> + if (index < 0)
>> + return -EINVAL;
>>
>> - for (i = 0; i < LIOINTC_NUM_PARENT; i++)
>> - priv->handler[i].parent_int_map = of_parent_int_map[i];
>> + priv->core_isr[i] = of_iomap(node, index);
>> + }
>> + }
>>
>> /* Setup IRQ domain */
>> - domain = irq_domain_add_linear(node, 32,
>> +#ifdef CONFIG_ACPI
>> + domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>> + &acpi_irq_gc_ops, priv);
>> +#else
>> + domain = irq_domain_create_linear(domain_handle, LIOINTC_CHIP_IRQ,
>> &irq_generic_chip_ops, priv);
>> +#endif
>
> This feels wrong. The ACPI vs OF decision should be a runtime
> one. Maybe you don't have that issue yet because MIPS is only OF and
> LoongArch only ACPI, but it remains that this should be done
> dynamically.
>
> Please try to rewrite it as:
>
> if (!acpi_disabled)
> [... ACPI stuff ...]
> else
> [... OF stuff ...]
>
> This should apply to all other cases where you have similar distinctions.
>
Ok, thanks, I'll change all the patch series according to your
suggestion here.
> Thanks,
>
> M.
>
Powered by blists - more mailing lists