Re: [PATCH v3 3/5] irqchip/gic-v3-its: Split allocation from initialisation of its_node

[Marc Zyngier <maz@kernel.org>]

On Tue, 24 Oct 2023 09:48:31 +0100,
Dominic Rath <dominic.rath@ibv-augsburg.net> wrote:
> 
> Hi,
> 
> On Fri, Oct 06, 2023 at 02:59:27PM +0200, Lorenzo Pieralisi wrote:
> > From: Marc Zyngier <maz@kernel.org>
> > 
> > In order to pave the way for more fancy quirk handling without making
> > more of a mess of this terrible driver, split the allocation of the
> > ITS descriptor (its_node) from the actual probing.
> 
> it seems that this change breaks MSI-X (MSI?) reception on at least
> the TI AM64x, probably most/all of TI's recent devices (K3).
> 
> These devices rely on a quirk CONFIG_SOCIONEXT_SYNQUACER_PREITS that
> uses an address from the dts specified e.g. as
> 
>   socionext,synquacer-pre-its = <0x1000000 0x400000>;
> 
> to configure a MSI base address that differs from the ARM default.

<rant>
Why on Earth are people using a property that is specific to another
implementation? Not only the HW is braindead, but this is now tied to
whatever additional implementation quirks we find...

This is just dumb.
</rant>

> With this change, the quirk still sets its->get_msi_base and clears
> IRQ_DOMAIN_FLAG_ISOLATED_MSI from its->msi_domain_flags during
> its_of_probe, but both get overwritten again during its_probe_one
> with the defaults.
> 
> Previously the defaults would be set first and then the quirks were
> applied.

Yeah, that's clearly a regression, and I've confirmed it on my
Synquacer (which means the TI folks have accurately copied a dumb
idea). Can you please give the patch below a go on your system and
confirm asap whether it works for you?

> I have no idea whether TI's use of this quirk was "correct", but it did
> work, and since 6.6-rc6 MSI-X has been broken for us.

Just as for bad SW, the worse HW ideas get replicated. Then I write
bad SW for it.

Thanks,

	M.

From b5571a69f09733ecfa0c944cc48baced6590d024 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Tue, 24 Oct 2023 11:07:34 +0100
Subject: [PATCH] irqchip/gic-v3-its: Don't override quirk settings with
 default values

When splitting the allocation of the ITS node from its configuration,
some of the default settings were kept in the latter instead of
being moved to the former.

This has the side effect of negating some of the quirk detection that
have happened in between, amongst which the dreaded Synquacer hack
(that also affect Dominic's TI platform).

Move the initialisation of these fields early, so that they can
again be overriden by the Synquacer quirk.

Fixes: 9585a495ac93 ("irqchip/gic-v3-its: Split allocation from initialisation of its_node")
Reported by: Dominic Rath <dominic.rath@ibv-augsburg.net>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20231024084831.GA3788@JADEVM-DRA
---
 drivers/irqchip/irq-gic-v3-its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 75a2dd550625..a8c89df1a997 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5112,8 +5112,6 @@ static int __init its_probe_one(struct its_node *its)
 	}
 	its->cmd_base = (void *)page_address(page);
 	its->cmd_write = its->cmd_base;
-	its->get_msi_base = its_irq_get_msi_base;
-	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
 	err = its_alloc_tables(its);
 	if (err)
@@ -5362,6 +5360,8 @@ static struct its_node __init *its_node_init(struct resource *res,
 	its->typer = gic_read_typer(its_base + GITS_TYPER);
 	its->base = its_base;
 	its->phys_base = res->start;
+	its->get_msi_base = its_irq_get_msi_base;
+	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
 	its->numa_node = numa_node;
 	its->fwnode_handle = handle;
-- 
2.39.2


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