| 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
| ||
|
Date: Wed, 29 Jun 2022 12:20:54 +0100
From: Marc Zyngier <maz@...nel.org>
To: Jianmin Lv <lvjianmin@...ngson.cn>
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 V13 06/13] irqchip/loongson-pch-pic: Add ACPI init support
On Mon, 27 Jun 2022 12:39:50 +0100,
Jianmin Lv <lvjianmin@...ngson.cn> wrote:
>
> From: Huacai Chen <chenhuacai@...ngson.cn>
>
> We are preparing to add new Loongson (based on LoongArch, not compatible
> with old MIPS-based Loongson) support. LoongArch use ACPI other than DT
> as its boot protocol, so add ACPI init support.
Drop this paragraph.
>
> PCH-PIC/PCH-MSI stands for "Interrupt Controller" that described in
> Section 5 of "Loongson 7A1000 Bridge User Manual". For more information
> please refer Documentation/loongarch/irq-chip-model.rst.
>
> For systems with two chipsets, there are two related pch irqdomains,
> each of which has the same node id as its parent irqdomain. So we
> defined a structure to mantain the relation of node and it's parent
> irqdomain.
>
> struct acpi_vector_group {
> int node;
> struct irq_domain *parent;
> };
>
> The parent irqdomain will be set for node id in parent irqdomain driver
> before pch irqdomain is created.
>
> 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 | 11 +-
> arch/loongarch/kernel/irq.c | 2 +-
> drivers/irqchip/irq-loongson-pch-pic.c | 200 ++++++++++++++++++++++++++++-----
> 3 files changed, 179 insertions(+), 34 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/irq.h b/arch/loongarch/include/asm/irq.h
> index 48c0ce4..a444dc6 100644
> --- a/arch/loongarch/include/asm/irq.h
> +++ b/arch/loongarch/include/asm/irq.h
> @@ -48,6 +48,12 @@ static inline bool on_irq_stack(int cpu, unsigned long sp)
> #define MAX_IO_PICS 2
> #define NR_IRQS (64 + (256 * MAX_IO_PICS))
>
> +struct acpi_vector_group {
> + int node;
> + struct irq_domain *parent;
> +};
> +extern struct acpi_vector_group pch_group[MAX_IO_PICS];
> +
> #define CORES_PER_EIO_NODE 4
>
> #define LOONGSON_CPU_UART0_VEC 10 /* CPU UART0 */
> @@ -108,8 +114,7 @@ int pch_lpc_acpi_init(struct irq_domain *parent,
> struct acpi_madt_lpc_pic *acpi_pchlpc);
> struct irq_domain *pch_msi_acpi_init(struct irq_domain *parent,
> struct acpi_madt_msi_pic *acpi_pchmsi);
> -struct irq_domain *pch_pic_acpi_init(struct irq_domain *parent,
> - struct acpi_madt_bio_pic *acpi_pchpic);
> +int find_pch_pic(u32 gsi);
>
> extern struct acpi_madt_lio_pic *acpi_liointc;
> extern struct acpi_madt_eio_pic *acpi_eiointc[MAX_IO_PICS];
> @@ -123,7 +128,7 @@ struct irq_domain *pch_pic_acpi_init(struct irq_domain *parent,
> extern struct irq_domain *liointc_domain;
> extern struct fwnode_handle *pch_lpc_handle;
> extern struct irq_domain *pch_msi_domain[MAX_IO_PICS];
> -extern struct irq_domain *pch_pic_domain[MAX_IO_PICS];
> +extern struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
>
> extern irqreturn_t loongson3_ipi_interrupt(int irq, void *dev);
>
> diff --git a/arch/loongarch/kernel/irq.c b/arch/loongarch/kernel/irq.c
> index 07d6059..f2115be 100644
> --- a/arch/loongarch/kernel/irq.c
> +++ b/arch/loongarch/kernel/irq.c
> @@ -28,7 +28,7 @@
> struct irq_domain *cpu_domain;
> struct irq_domain *liointc_domain;
> struct irq_domain *pch_msi_domain[MAX_IO_PICS];
> -struct irq_domain *pch_pic_domain[MAX_IO_PICS];
> +struct acpi_vector_group pch_group[MAX_IO_PICS];
>
> /*
> * 'what should we do if we get a hw irq event on an illegal vector'.
> diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
> index a4eb8a2..170a5b9 100644
> --- a/drivers/irqchip/irq-loongson-pch-pic.c
> +++ b/drivers/irqchip/irq-loongson-pch-pic.c
> @@ -33,13 +33,40 @@
> #define PIC_REG_IDX(irq_id) ((irq_id) / PIC_COUNT_PER_REG)
> #define PIC_REG_BIT(irq_id) ((irq_id) % PIC_COUNT_PER_REG)
>
> +static int nr_pics;
> +
> struct pch_pic {
> void __iomem *base;
> struct irq_domain *pic_domain;
> + struct fwnode_handle *domain_handle;
> u32 ht_vec_base;
> raw_spinlock_t pic_lock;
> + u32 gsi_end;
> + u32 gsi_base;
> };
>
> +static struct pch_pic *pch_pic_priv[2];
Why 2? Is that related to MAX_IO_PICS being 2?
> +struct fwnode_handle *pch_pic_handle[MAX_IO_PICS];
> +
> +int find_pch_pic(u32 gsi)
> +{
> + int i;
> +
> + /* Find the PCH_PIC that manages this GSI. */
> + for (i = 0; i < MAX_IO_PICS; i++) {
> + struct pch_pic *priv = pch_pic_priv[i];
> +
> + if (!priv)
> + return -1;
> +
> + if (gsi >= priv->gsi_base && gsi <= priv->gsi_end)
> + return i;
> + }
> +
> + pr_err("ERROR: Unable to locate PCH_PIC for GSI %d\n", gsi);
> + return -1;
> +}
> +
> static void pch_pic_bitset(struct pch_pic *priv, int offset, int bit)
> {
> u32 reg;
> @@ -139,6 +166,24 @@ static void pch_pic_ack_irq(struct irq_data *d)
> .irq_set_type = pch_pic_set_type,
> };
>
> +static int pch_pic_domain_translate(struct irq_domain *d,
> + struct irq_fwspec *fwspec,
> + unsigned long *hwirq,
> + unsigned int *type)
> +{
> + struct pch_pic *priv = d->host_data;
> + struct device_node *of_node = to_of_node(fwspec->fwnode);
> +
> + if (fwspec->param_count < 1)
> + return -EINVAL;
> + if (of_node)
> + *hwirq += priv->ht_vec_base;
> + else
> + *hwirq = fwspec->param[0] - priv->gsi_base;
> + *type = IRQ_TYPE_NONE;
Have you tested this on MIPS HW? This looks like a regression on the
OF path, as it used irq_domain_translate_twocell().
> +
> + return 0;
> +}
> static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
> unsigned int nr_irqs, void *arg)
> {
> @@ -149,13 +194,13 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
> struct irq_fwspec parent_fwspec;
> struct pch_pic *priv = domain->host_data;
>
> - err = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
> + err = pch_pic_domain_translate(domain, fwspec, &hwirq, &type);
> if (err)
> return err;
>
> parent_fwspec.fwnode = domain->parent->fwnode;
> parent_fwspec.param_count = 1;
> - parent_fwspec.param[0] = hwirq + priv->ht_vec_base;
> + parent_fwspec.param[0] = hwirq;
>
> err = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> if (err)
> @@ -170,7 +215,7 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
> }
>
> static const struct irq_domain_ops pch_pic_domain_ops = {
> - .translate = irq_domain_translate_twocell,
> + .translate = pch_pic_domain_translate,
> .alloc = pch_pic_alloc,
> .free = irq_domain_free_irqs_parent,
> };
> @@ -180,7 +225,7 @@ static void pch_pic_reset(struct pch_pic *priv)
> int i;
>
> for (i = 0; i < PIC_COUNT; i++) {
> - /* Write vectored ID */
> + /* Write vector ID */
> writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i));
> /* Hardcode route to HT0 Lo */
> writeb(1, priv->base + PCH_INT_ROUTE(i));
> @@ -198,50 +243,41 @@ static void pch_pic_reset(struct pch_pic *priv)
> }
> }
>
> -static int pch_pic_of_init(struct device_node *node,
> - struct device_node *parent)
> +static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
> + struct irq_domain *parent_domain, struct fwnode_handle *domain_handle,
> + u32 gsi_base)
> {
> + int vec_count;
> struct pch_pic *priv;
> - struct irq_domain *parent_domain;
> - int err;
>
> priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
>
> raw_spin_lock_init(&priv->pic_lock);
> - priv->base = of_iomap(node, 0);
> - if (!priv->base) {
> - err = -ENOMEM;
> + priv->base = ioremap(addr, size);
> + if (!priv->base)
> goto free_priv;
> - }
>
> - parent_domain = irq_find_host(parent);
> - if (!parent_domain) {
> - pr_err("Failed to find the parent domain\n");
> - err = -ENXIO;
> - goto iounmap_base;
> - }
> + priv->domain_handle = domain_handle;
>
> - if (of_property_read_u32(node, "loongson,pic-base-vec",
> - &priv->ht_vec_base)) {
> - pr_err("Failed to determine pic-base-vec\n");
> - err = -EINVAL;
> - goto iounmap_base;
> - }
> + priv->ht_vec_base = vec_base;
Why isn't this pre-filled on the OF path as it isn't relevant to ACPI?
> + vec_count = ((readq(priv->base) >> 48) & 0xff) + 1;
Is that valid on the old HW?
> + priv->gsi_base = gsi_base;
Why isn't this pre-filled on the ACPI path as it isn't relevant to OF?
> + priv->gsi_end = gsi_base + vec_count - 1;
I don't think this needs to be ACPI specific. Just track the vector
count, which applies to both ACPI and DT.
>
> priv->pic_domain = irq_domain_create_hierarchy(parent_domain, 0,
> - PIC_COUNT,
> - of_node_to_fwnode(node),
> - &pch_pic_domain_ops,
> - priv);
> + vec_count, priv->domain_handle,
> + &pch_pic_domain_ops, priv);
> +
> if (!priv->pic_domain) {
> pr_err("Failed to create IRQ domain\n");
> - err = -ENOMEM;
> goto iounmap_base;
> }
>
> pch_pic_reset(priv);
> + pch_pic_handle[nr_pics] = domain_handle;
> + pch_pic_priv[nr_pics++] = priv;
>
> return 0;
>
> @@ -250,7 +286,111 @@ static int pch_pic_of_init(struct device_node *node,
> free_priv:
> kfree(priv);
>
> - return err;
> + return -EINVAL;
> +}
> +
> +#ifdef CONFIG_OF
> +
> +static int pch_pic_of_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + int err, vec_base;
> + struct resource res;
> + struct irq_domain *parent_domain;
> +
> + if (of_address_to_resource(node, 0, &res))
> + return -EINVAL;
> +
> + parent_domain = irq_find_host(parent);
> + if (!parent_domain) {
> + pr_err("Failed to find the parent domain\n");
> + return -ENXIO;
> + }
> +
> + if (of_property_read_u32(node, "loongson,pic-base-vec", &vec_base)) {
> + pr_err("Failed to determine pic-base-vec\n");
> + return -EINVAL;
> + }
> +
> + err = pch_pic_init(res.start, resource_size(&res), vec_base,
> + parent_domain, of_node_to_fwnode(node), 0);
> + if (err < 0)
> + return err;
> +
> + return 0;
> }
>
> IRQCHIP_DECLARE(pch_pic, "loongson,pch-pic-1.0", pch_pic_of_init);
> +
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static int __init
> +lpcintc_parse_madt(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + struct acpi_madt_lpc_pic *lpcintc_entry = (struct acpi_madt_lpc_pic *)header;
> +
> + return pch_lpc_acpi_init(pch_pic_priv[0]->pic_domain, lpcintc_entry);
> +}
> +
> +static int __init acpi_cascade_irqdomain_init(void)
> +{
> + acpi_table_parse_madt(ACPI_MADT_TYPE_LPC_PIC,
> + lpcintc_parse_madt, 0);
> + return 0;
> +}
Missing blank line between functions
> +int __init pch_pic_init_irqdomain(struct irq_domain *parent,
> + struct acpi_madt_bio_pic *acpi_pchpic)
> +{
> + int ret, vec_base;
> + struct fwnode_handle *domain_handle;
> +
> + if (!acpi_pchpic)
> + return -EINVAL;
> +
> + vec_base = acpi_pchpic->gsi_base - GSI_MIN_PCH_IRQ;
> +
> + domain_handle = irq_domain_alloc_fwnode((phys_addr_t *)acpi_pchpic);
> + if (!domain_handle) {
> + pr_err("Unable to allocate domain handle\n");
> + return -ENOMEM;
> + }
> +
> + ret = pch_pic_init(acpi_pchpic->address, acpi_pchpic->size,
> + vec_base, parent, domain_handle, acpi_pchpic->gsi_base);
> + if (ret == 0 && acpi_pchpic->id == 0)
> + acpi_cascade_irqdomain_init();
> +
> + return ret;
> +}
> +
> +struct irq_domain *acpi_get_pch_parent(int node)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_IO_PICS; i++) {
> + if (node == pch_group[i].node)
> + return pch_group[i].parent;
> + }
> + return NULL;
> +}
> +
> +static int __init pchintc_parse_madt(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + struct acpi_madt_bio_pic *pch_pic_entry = (struct acpi_madt_bio_pic *)header;
> +
> + return pch_pic_init_irqdomain(acpi_get_pch_parent((pch_pic_entry->address >> 44) & 0xf),
> + pch_pic_entry);
> +}
> +
> +static int __init pch_pic_acpi_init(void)
> +{
> + acpi_table_parse_madt(ACPI_MADT_TYPE_BIO_PIC,
Where is this defined? It only appears in the documentation, and
nowhere else...
> + pchintc_parse_madt, 0);
> +
> + return 0;
> +}
> +early_initcall(pch_pic_acpi_init);
Why can't you use IRQCHIP_ACPI_DECLARE here? This is terribly fragile,
and will eventually break. I really don't want to rely on this.
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists