Message-ID: <ZR7BV1gmWlEae+gq@lpieralisi>
Date:   Thu, 5 Oct 2023 15:59:51 +0200
From:   Lorenzo Pieralisi <lpieralisi@...nel.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Robin Murphy <robin.murphy@....com>,
        Mark Rutland <mark.rutland@....com>,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Fang Xiang <fangxiang3@...omi.com>
Subject: Re: [PATCH 2/2] irqchip/gic-v3: Enable non-coherent
 redistributors/ITSes probing

On Tue, Oct 03, 2023 at 05:44:27PM +0100, Marc Zyngier wrote:
> On Tue, 03 Oct 2023 15:43:40 +0100,
> Lorenzo Pieralisi <lpieralisi@...nel.org> wrote:
> > 
> > On Tue, Sep 05, 2023 at 12:34:58PM +0100, Marc Zyngier wrote:
> > 
> > [...]
> > 
> > > >  	 * Make sure *all* the ITS are reset before we probe any, as
> > > >  	 * they may be sharing memory. If any of the ITS fails to
> > > > @@ -5396,7 +5405,8 @@ static int __init its_of_probe(struct device_node *node)
> > > >  			continue;
> > > >  		}
> > > >  
> > > > -		its_probe_one(&res, &np->fwnode, of_node_to_nid(np));
> > > > +		its_probe_one(&res, &np->fwnode, of_node_to_nid(np),
> > > > +			      of_property_read_bool(np, "dma-noncoherent"));
> > > >  	}
> > > >  	return 0;
> > > >  }
> > > > @@ -5533,7 +5543,8 @@ static int __init gic_acpi_parse_madt_its(union acpi_subtable_headers *header,
> > > >  	}
> > > >  
> > > >  	err = its_probe_one(&res, dom_handle,
> > > > -			acpi_get_its_numa_node(its_entry->translation_id));
> > > > +			acpi_get_its_numa_node(its_entry->translation_id),
> > > > +			false);
> > > 
> > > I came up with the following alternative approach, which is as usual
> > > completely untested. It is entirely based on the quirk infrastructure,
> > > and doesn't touch the ACPI path at all.
> > 
> > Writing the ACPI bits. We can't use the quirks framework for ACPI (we
> > don't have "properties" and I don't think we want to attach any to the
> > fwnode_handle) that's why I generalized its_probe_one() above with an
> > extra param, that would have simplified ACPI parsing:
> > 
> > - we alloc struct its_node in its_probe_one() but at that stage
> >   ACPI parsing was already done. If we have to parse the MADT(ITS) again
> >   just to scan for non-coherent we then have to match the MADT entries
> >   to the *current* struct its_node* we are handling (MADT parsing
> >   callbacks don't even take a param - we have to resort to global
> >   variables - definitely doable but it is a bit ugly).
> 
> Well, a more acceptable approach would be for its_probe_one() to take
> an allocated and possibly pre-populated its_node structure (crucially,
> with the quirk flags set), which itself results in a bunch of low
> hanging cleanups, see the patch below.
> 
> I have boot tested it in a DT guest, so it is obviously perfect.

If you don't mind I will post it together with the resulting series.

I have just removed:

/* Stick ACPI quirk handling here */

point taken :)

Thanks,
Lorenzo

> 	M.
> 
> From 978f654d4459adf0b8f3f8e896ca37035b3b114c Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@...nel.org>
> Date: Tue, 3 Oct 2023 17:35:27 +0100
> Subject: [PATCH] irqchip/gic-v3-its: Split allocation from initialisation of
>  its_node
> 
> 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@...nel.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 151 +++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index e0c2b10d154d..bf21383b714e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4952,7 +4952,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;
> @@ -4966,7 +4966,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);
> @@ -5017,8 +5017,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;
> @@ -5032,15 +5031,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;
> @@ -5048,75 +5047,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) {
> @@ -5125,12 +5099,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;
> @@ -5174,7 +5145,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;
>  
> @@ -5191,11 +5162,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;
>  }
>  
> @@ -5356,10 +5324,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
> @@ -5369,8 +5380,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))
> @@ -5383,6 +5392,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")) {
> @@ -5396,7 +5407,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;
>  }
> @@ -5508,6 +5529,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;
>  
> @@ -5532,11 +5554,20 @@ 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;
> +	}
> +
> +	/* Stick ACPI quirk handling here */
> +
> +	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);
> -- 
> 2.34.1
> 
> -- 
> Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists