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

Message ID 20231006125929.48591-4-lpieralisi@kernel.org
State New
Headers
Series irqchip/gic-v3: Enable non-coherent GIC designs probing |

Commit Message

Lorenzo Pieralisi Oct. 6, 2023, 12:59 p.m. UTC
  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.

This will allow firmware-specific hooks to be added between these
two points.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 149 ++++++++++++++++++-------------
 1 file changed, 89 insertions(+), 60 deletions(-)
  

Comments

Dominic Rath Oct. 24, 2023, 8:48 a.m. UTC | #1
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.

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.

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.

Best Regards,
Dominic
  
Marc Zyngier Oct. 24, 2023, 10:18 a.m. UTC | #2
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;
  
Dominic Rath Oct. 24, 2023, 1:13 p.m. UTC | #3
On Tue, Oct 24, 2023 at 11:18:21AM +0100, Marc Zyngier wrote:
> 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?
> 

Thanks a lot, with that patch applied on top of 6.6-rc6 MSI-X interrupts
work again for the AM64x.

Best Regards,

Dominic

> > 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>

Tested-by: Dominic Rath <dominic.rath@ibv-augsburg.net>

> 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.
  

Patch

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index adde347dc890..75a2dd550625 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4965,7 +4965,7 @@  static void __init __iomem *its_map_one(struct resource *res, int *err)
 	return NULL;
 }
 
-static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
+static int its_init_domain(struct its_node *its)
 {
 	struct irq_domain *inner_domain;
 	struct msi_domain_info *info;
@@ -4979,7 +4979,7 @@  static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
 
 	inner_domain = irq_domain_create_hierarchy(its_parent,
 						   its->msi_domain_flags, 0,
-						   handle, &its_domain_ops,
+						   its->fwnode_handle, &its_domain_ops,
 						   info);
 	if (!inner_domain) {
 		kfree(info);
@@ -5030,8 +5030,7 @@  static int its_init_vpe_domain(void)
 	return 0;
 }
 
-static int __init its_compute_its_list_map(struct resource *res,
-					   void __iomem *its_base)
+static int __init its_compute_its_list_map(struct its_node *its)
 {
 	int its_number;
 	u32 ctlr;
@@ -5045,15 +5044,15 @@  static int __init its_compute_its_list_map(struct resource *res,
 	its_number = find_first_zero_bit(&its_list_map, GICv4_ITS_LIST_MAX);
 	if (its_number >= GICv4_ITS_LIST_MAX) {
 		pr_err("ITS@%pa: No ITSList entry available!\n",
-		       &res->start);
+		       &its->phys_base);
 		return -EINVAL;
 	}
 
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	ctlr &= ~GITS_CTLR_ITS_NUMBER;
 	ctlr |= its_number << GITS_CTLR_ITS_NUMBER_SHIFT;
-	writel_relaxed(ctlr, its_base + GITS_CTLR);
-	ctlr = readl_relaxed(its_base + GITS_CTLR);
+	writel_relaxed(ctlr, its->base + GITS_CTLR);
+	ctlr = readl_relaxed(its->base + GITS_CTLR);
 	if ((ctlr & GITS_CTLR_ITS_NUMBER) != (its_number << GITS_CTLR_ITS_NUMBER_SHIFT)) {
 		its_number = ctlr & GITS_CTLR_ITS_NUMBER;
 		its_number >>= GITS_CTLR_ITS_NUMBER_SHIFT;
@@ -5061,75 +5060,50 @@  static int __init its_compute_its_list_map(struct resource *res,
 
 	if (test_and_set_bit(its_number, &its_list_map)) {
 		pr_err("ITS@%pa: Duplicate ITSList entry %d\n",
-		       &res->start, its_number);
+		       &its->phys_base, its_number);
 		return -EINVAL;
 	}
 
 	return its_number;
 }
 
-static int __init its_probe_one(struct resource *res,
-				struct fwnode_handle *handle, int numa_node)
+static int __init its_probe_one(struct its_node *its)
 {
-	struct its_node *its;
-	void __iomem *its_base;
-	u64 baser, tmp, typer;
+	u64 baser, tmp;
 	struct page *page;
 	u32 ctlr;
 	int err;
 
-	its_base = its_map_one(res, &err);
-	if (!its_base)
-		return err;
-
-	pr_info("ITS %pR\n", res);
-
-	its = kzalloc(sizeof(*its), GFP_KERNEL);
-	if (!its) {
-		err = -ENOMEM;
-		goto out_unmap;
-	}
-
-	raw_spin_lock_init(&its->lock);
-	mutex_init(&its->dev_alloc_lock);
-	INIT_LIST_HEAD(&its->entry);
-	INIT_LIST_HEAD(&its->its_device_list);
-	typer = gic_read_typer(its_base + GITS_TYPER);
-	its->typer = typer;
-	its->base = its_base;
-	its->phys_base = res->start;
 	if (is_v4(its)) {
-		if (!(typer & GITS_TYPER_VMOVP)) {
-			err = its_compute_its_list_map(res, its_base);
+		if (!(its->typer & GITS_TYPER_VMOVP)) {
+			err = its_compute_its_list_map(its);
 			if (err < 0)
-				goto out_free_its;
+				goto out;
 
 			its->list_nr = err;
 
 			pr_info("ITS@%pa: Using ITS number %d\n",
-				&res->start, err);
+				&its->phys_base, err);
 		} else {
-			pr_info("ITS@%pa: Single VMOVP capable\n", &res->start);
+			pr_info("ITS@%pa: Single VMOVP capable\n", &its->phys_base);
 		}
 
 		if (is_v4_1(its)) {
-			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, typer);
+			u32 svpet = FIELD_GET(GITS_TYPER_SVPET, its->typer);
 
-			its->sgir_base = ioremap(res->start + SZ_128K, SZ_64K);
+			its->sgir_base = ioremap(its->phys_base + SZ_128K, SZ_64K);
 			if (!its->sgir_base) {
 				err = -ENOMEM;
-				goto out_free_its;
+				goto out;
 			}
 
-			its->mpidr = readl_relaxed(its_base + GITS_MPIDR);
+			its->mpidr = readl_relaxed(its->base + GITS_MPIDR);
 
 			pr_info("ITS@%pa: Using GICv4.1 mode %08x %08x\n",
-				&res->start, its->mpidr, svpet);
+				&its->phys_base, its->mpidr, svpet);
 		}
 	}
 
-	its->numa_node = numa_node;
-
 	page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO,
 				get_order(ITS_CMD_QUEUE_SZ));
 	if (!page) {
@@ -5138,12 +5112,9 @@  static int __init its_probe_one(struct resource *res,
 	}
 	its->cmd_base = (void *)page_address(page);
 	its->cmd_write = its->cmd_base;
-	its->fwnode_handle = handle;
 	its->get_msi_base = its_irq_get_msi_base;
 	its->msi_domain_flags = IRQ_DOMAIN_FLAG_ISOLATED_MSI;
 
-	its_enable_quirks(its);
-
 	err = its_alloc_tables(its);
 	if (err)
 		goto out_free_cmd;
@@ -5187,7 +5158,7 @@  static int __init its_probe_one(struct resource *res,
 		ctlr |= GITS_CTLR_ImDe;
 	writel_relaxed(ctlr, its->base + GITS_CTLR);
 
-	err = its_init_domain(handle, its);
+	err = its_init_domain(its);
 	if (err)
 		goto out_free_tables;
 
@@ -5204,11 +5175,8 @@  static int __init its_probe_one(struct resource *res,
 out_unmap_sgir:
 	if (its->sgir_base)
 		iounmap(its->sgir_base);
-out_free_its:
-	kfree(its);
-out_unmap:
-	iounmap(its_base);
-	pr_err("ITS@%pa: failed probing (%d)\n", &res->start, err);
+out:
+	pr_err("ITS@%pa: failed probing (%d)\n", &its->phys_base, err);
 	return err;
 }
 
@@ -5369,10 +5337,53 @@  static const struct of_device_id its_device_id[] = {
 	{},
 };
 
+static struct its_node __init *its_node_init(struct resource *res,
+					     struct fwnode_handle *handle, int numa_node)
+{
+	void __iomem *its_base;
+	struct its_node *its;
+	int err;
+
+	its_base = its_map_one(res, &err);
+	if (!its_base)
+		return NULL;
+
+	pr_info("ITS %pR\n", res);
+
+	its = kzalloc(sizeof(*its), GFP_KERNEL);
+	if (!its)
+		goto out_unmap;
+
+	raw_spin_lock_init(&its->lock);
+	mutex_init(&its->dev_alloc_lock);
+	INIT_LIST_HEAD(&its->entry);
+	INIT_LIST_HEAD(&its->its_device_list);
+
+	its->typer = gic_read_typer(its_base + GITS_TYPER);
+	its->base = its_base;
+	its->phys_base = res->start;
+
+	its->numa_node = numa_node;
+	its->fwnode_handle = handle;
+
+	return its;
+
+out_unmap:
+	iounmap(its_base);
+	return NULL;
+}
+
+static void its_node_destroy(struct its_node *its)
+{
+	iounmap(its->base);
+	kfree(its);
+}
+
 static int __init its_of_probe(struct device_node *node)
 {
 	struct device_node *np;
 	struct resource res;
+	int err;
 
 	/*
 	 * Make sure *all* the ITS are reset before we probe any, as
@@ -5382,8 +5393,6 @@  static int __init its_of_probe(struct device_node *node)
 	 */
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
-		int err;
-
 		if (!of_device_is_available(np) ||
 		    !of_property_read_bool(np, "msi-controller") ||
 		    of_address_to_resource(np, 0, &res))
@@ -5396,6 +5405,8 @@  static int __init its_of_probe(struct device_node *node)
 
 	for (np = of_find_matching_node(node, its_device_id); np;
 	     np = of_find_matching_node(np, its_device_id)) {
+		struct its_node *its;
+
 		if (!of_device_is_available(np))
 			continue;
 		if (!of_property_read_bool(np, "msi-controller")) {
@@ -5409,7 +5420,17 @@  static int __init its_of_probe(struct device_node *node)
 			continue;
 		}
 
-		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
+
+		its = its_node_init(&res, &np->fwnode, of_node_to_nid(np));
+		if (!its)
+			return -ENOMEM;
+
+		its_enable_quirks(its);
+		err = its_probe_one(its);
+		if (err)  {
+			its_node_destroy(its);
+			return err;
+		}
 	}
 	return 0;
 }
@@ -5521,6 +5542,7 @@  static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 {
 	struct acpi_madt_generic_translator *its_entry;
 	struct fwnode_handle *dom_handle;
+	struct its_node *its;
 	struct resource res;
 	int err;
 
@@ -5545,11 +5567,18 @@  static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
 		goto dom_err;
 	}
 
-	err = its_probe_one(&res, dom_handle,
-			acpi_get_its_numa_node(its_entry->translation_id));
+	its = its_node_init(&res, dom_handle,
+			    acpi_get_its_numa_node(its_entry->translation_id));
+	if (!its) {
+		err = -ENOMEM;
+		goto node_err;
+	}
+
+	err = its_probe_one(its);
 	if (!err)
 		return 0;
 
+node_err:
 	iort_deregister_domain_token(its_entry->translation_id);
 dom_err:
 	irq_domain_free_fwnode(dom_handle);