linux-kernel - Re: [PATCH 3/9] irqchip/loongson-pch-pic: Add ACPI init support

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Aug 2021 14:28:32 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Huacai Chen <chenhuacai@...il.com>
Cc:     Huacai Chen <chenhuacai@...ngson.cn>,
        Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Xuefeng Li <lixuefeng@...ngson.cn>,
        Jiaxun Yang <jiaxun.yang@...goat.com>
Subject: Re: [PATCH 3/9] irqchip/loongson-pch-pic: Add ACPI init support

On Thu, 12 Aug 2021 13:23:27 +0100,
Huacai Chen <chenhuacai@...il.com> wrote:
> 
[...]

> > > > +struct fwnode_handle *pch_pic_acpi_init(struct fwnode_handle *parent,
> > > > +                                     struct acpi_madt_bio_pic *acpi_pchpic)
> > > > +{
> > > > +     int count;
> > > > +     struct pch_pic *priv;
> > > > +     struct irq_domain *parent_domain;
> > > > +
> > > > +     priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > +     if (!priv)
> > > > +             return NULL;
> > > > +
> > > > +     raw_spin_lock_init(&priv->pic_lock);
> > > > +     priv->base = ioremap(acpi_pchpic->address, acpi_pchpic->size);
> > > > +     if (!priv->base)
> > > > +             goto free_priv;
> > > > +
> > > > +     priv->domain_handle = irq_domain_alloc_fwnode(priv->base);
> > > > +     if (!priv->domain_handle) {
> > > > +             pr_err("Unable to allocate domain handle\n");
> > > > +             goto iounmap_base;
> > > > +     }
> > > > +
> > > > +     priv->ht_vec_base = acpi_pchpic->gsi_base;
> > > > +     count = ((readq(priv->base) >> 48) & 0xff) + 1;
> > > > +     parent_domain = irq_find_matching_fwnode(parent, DOMAIN_BUS_ANY);
> > > > +     if (!parent_domain) {
> > > > +             pr_err("Failed to find the parent domain\n");
> > > > +             goto iounmap_base;
> > > > +     }
> > > > +
> > > > +     priv->pic_domain = irq_domain_create_hierarchy(parent_domain, 0,
> > > > +                                             count, priv->domain_handle,
> > > > +                                             &pch_pic_domain_ops, priv);
> > > > +
> > > > +     if (!priv->pic_domain) {
> > > > +             pr_err("Failed to create IRQ domain\n");
> > > > +             goto iounmap_base;
> > > > +     }
> > > > +
> > > > +     pch_pic_reset(priv);
> > > > +     pch_pic_priv[nr_pch_pics++] = priv;
> > > > +
> > > > +     register_syscore_ops(&pch_pic_syscore_ops);
> > > > +
> > > > +     return priv->domain_handle;
> > > > +
> > > > +iounmap_base:
> > > > +     iounmap(priv->base);
> > > > +free_priv:
> > > > +     kfree(priv);
> > > > +
> > > > +     return NULL;
> > > > +}
> > > > +
> > > > +#endif
> > >
> > > A lot of this code is common with its OF counterpart. How about making
> > > this logic common?
> > OK, let me think about.
> Though pch_pic_acpi_init() is similar to pch_pic_of_init(), but it is
> difficult to make a common function, because we cannot prepare
> everything before the common function. For example, ioremap() can be
> the common part, but pch_pic_acpi_init() should get the vector count
> after ioremap(). If we use an argument to distinguish the caller in
> the common function, the complexity increases and seems no benefits.

All firmware implementations allocate private data structures, irq
domains, map MMIO regions, etc. All that can be common. We even have
APIs that deal with both firmware interfaces.

As for the 'read the vector count from the HW', what does it have to
do with driving the HW using DT or ACPI? The HW doesn't *know*. If you
are conflating HW changes and firmware interfaces, then you have
already lost.

	M.

-- 
Without deviation from the norm, progress is not possible.