Re: [PATCH V13 06/13] irqchip/loongson-pch-pic: Add ACPI init support

From: Marc Zyngier <maz@kernel.org>
To: Jianmin Lv <lvjianmin@loongson.cn>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org, Hanjun Guo <guohanjun@huawei.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Jiaxun Yang <jiaxun.yang@flygoat.com>,
	Huacai Chen <chenhuacai@loongson.cn>
Subject: Re: [PATCH V13 06/13] irqchip/loongson-pch-pic: Add ACPI init support
Date: Thu, 30 Jun 2022 08:22:47 +0100	[thread overview]
Message-ID: <87o7yaehtk.wl-maz@kernel.org> (raw)
In-Reply-To: <aa8321d5-9839-9ccf-6284-766719e980ea@loongson.cn>

On Thu, 30 Jun 2022 05:36:40 +0100,
Jianmin Lv <lvjianmin@loongson.cn> wrote:
> 
>

[...]

> >> +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...
> > 
> 
> It's defined in the patch to ACPICA(which has been merged, please to
> see
> https://github.com/acpica/acpica/commit/1dc530059a3e6202e941e6a9478cf30f092bfb47).
> the patch will be synchronized to linux kernel by maintainer of ACPICA.

How can I take this patch upstream if it doesn't even compile? Please
make this patch part of your series. There are tons of patches that
need Acks from the ACPI maintainers, this is only one of them.

>
> 
> >> +			      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.
> > 
> 
> In early time, the change here is implemented using
> IRQCHIP_ACPI_DECLARE, but we found that calling order(during
> irqchip_init) of the entry declared using IRQCHIP_ACPI_DECLARE is
> depended on the compiling order(driver order in Makefile) of the
> driver. For removing the dependency to the compiling order, the new
> way here is used(I looked into ARM, it seems that GIC driver uses
> IRQCHIP_ACPI_DECLARE, and ITS driver uses early_initcall too.).

It's not quite the same. The ITS part that uses early_initcall isn't
an actual driver (it only registers a domain, and nothing else).

There is also no guarantee that the initcalls at the same priority
will execute in any order, and you already have at least two such
initcalls (pch_pic_acpi_init, pch_msi_acpi_init, and I haven't quite
understood how the rest is probed yet).

One possibility would be to drive the whole probing from the root
interrupt controller, which is similar to what GICv3 does for the
actual ITS driver. You already do this sort of stuff in the CPUINTC
driver, so adding to this shouldn't be too hard.

	M.

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