Received-SPF: pass (google.com: domain of linux-kernel+bounces-190023-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 27 May 2024 10:09:26 +0530
From: Sunil V L <sunilvl@ventanamicro.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org, linux-acpi@vger.kernel.org,
	linux-pci@vger.kernel.org, linux-serial@vger.kernel.org,
	acpica-devel@lists.linux.dev,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Anup Patel <anup@brainfault.org>,
	Samuel Holland <samuel.holland@sifive.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Robert Moore <robert.moore@intel.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	Andrew Jones <ajones@ventanamicro.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Marc Zyngier <maz@kernel.org>,
	Atish Kumar Patra <atishp@rivosinc.com>,
	Andrei Warkentin <andrei.warkentin@intel.com>,
	Haibo1 Xu <haibo1.xu@intel.com>,
	=?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
Subject: Re: [PATCH v5 13/17] irqchip/riscv-intc: Add ACPI support for AIA
Message-ID: <ZlQOfrMHHaGV0cDg@sunil-laptop>
References: <20240501121742.1215792-1-sunilvl@ventanamicro.com>
 <20240501121742.1215792-14-sunilvl@ventanamicro.com>
 <874jaofbfp.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <874jaofbfp.ffs@tglx>

Hi Thomas,

On Thu, May 23, 2024 at 11:47:06PM +0200, Thomas Gleixner wrote:
> On Wed, May 01 2024 at 17:47, Sunil V L wrote:
> > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> > index 9e71c4428814..af7a2f78f0ee 100644
> > --- a/drivers/irqchip/irq-riscv-intc.c
> > +++ b/drivers/irqchip/irq-riscv-intc.c
> > @@ -249,14 +249,105 @@ IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> >  IRQCHIP_DECLARE(andes, "andestech,cpu-intc", riscv_intc_init);
> >  
> >  #ifdef CONFIG_ACPI
> > +struct rintc_data {
> > +	u32 ext_intc_id;
> > +	unsigned long hart_id;
> > +	u64 imsic_addr;
> > +	u32 imsic_size;
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
>
Sure, thanks!
 
> > +};
> > +
> > +static u32 nr_rintc;
> > +static struct rintc_data *rintc_acpi_data[NR_CPUS];
> > +
> > +int acpi_get_intc_index_hartid(u32 index, unsigned long *hartid)
> 
> Why int? All of these functions have strictly boolean return values:
> success = true, fail = false, no?
> 
> Either bool or get rid of the pointer and let the function return
> either the real hart id or an invalid one.
>
Sure. I just tried to keep it similar to the parent function. But let me
go with your suggestion in the next revision.
 
> > +{
> > +	if (index >= nr_rintc)
> > +		return -1;
> > +
> > +	*hartid = rintc_acpi_data[index]->hart_id;
> > +	return 0;
> 
> I.e.
> 
> 	return index >= nr_rintc ? rintc_acpi_data[index]->hart_id : INVALID_HART_ID;
> 
Sure.

> > +int acpi_get_ext_intc_parent_hartid(u8 id, u32 idx, unsigned long *hartid)
> > +{
> > +	int i, j = 0;
> > +
> > +	for (i = 0; i < nr_rintc; i++) {
> > +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> > +			if (idx == j) {
> > +				*hartid = rintc_acpi_data[i]->hart_id;
> > +				return 0;
> > +			}
> > +			j++;
> > +		}
> > +	}
> > +
> > +	return -1;
> > +}
> > +
> > +void acpi_get_plic_nr_contexts(u8 id, int *nr_contexts)
> > +{
> > +	int i, j = 0;
> > +
> > +	for (i = 0; i < nr_rintc; i++) {
> > +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id)
> > +			j++;
> > +	}
> > +
> > +	*nr_contexts = j;
> > +}
> > +
> > +int acpi_get_plic_context(u8 id, u32 idx, int *context_id)
> > +{
> > +	int i, j = 0;
> > +
> > +	for (i = 0; i < nr_rintc; i++) {
> > +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> > +			if (idx == j) {
> > +				*context_id = IDC_CONTEXT_ID(rintc_acpi_data[i]->ext_intc_id);
> > +				return 0;
> > +			}
> > +
> > +			j++;
> > +		}
> > +	}
> 
> So that's the third incarnation of the same loop with the truly self
> explaining variable and argument names.
> 
>     j is actually the index of the context which is associated to a
>     given PLIC ID.
> 
>     idx is the context index to search for
> 
> Right? So why can't these things be named in a way which makes the
> intent of the code clear?
> 
> Also why are all the arguments u8/u32? There is no hardware
> involved. Simple 'unsigned int' is just fine and the u8/u32 is not bying
> you anything here.
> 
> Aside of that these ugly macros can be completely avoided and the code
> can be written without a copy & pasta orgy.
> 
> struct rintc_data {
> 	union {
> 		u32		ext_intc_id;
>                 struct {
>                 	u32	context_id	: 16,
>                         			:  8,
>                         	aplic_plic_id	:  8;
>                 }
>         };
> 	unsigned long		hart_id;
> 	u64			imsic_addr;
> 	u32			imsic_size;
> };
> 
> #define for_each_matching_plic(_plic, _plic_id)				\
> 	for (_plic = 0; _plic < nr_rintc; _plict++)			\
>         	if (rintc_acpi_data[_plic]->aplic_plic_id != _plic_id)	\
>                 	continue;					\
>                 else
> 
> unsigned int acpi_get_plic_nr_contexts(unsigned int plic_id)
> {
> 	unsigned int nctx = 0;
> 
> 	for_each_matching_plic(plic, plic_id)
> 		nctx++;
> 
> 	return nctx;
> }
> 
> static struct rintc_data *get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
> {
> 	unsigned int ctxt = 0;
> 
> 	for_each_matching_plic(plic, plic_id) {
>         	if (ctxt == ctxt_idx)
>                 	return rintc_acpi_data + plic;
>         }
>         return NULL;
> }
> 
> unsigned long acpi_get_ext_intc_parent_hartid(unsigned int plic_id, unsigned int ctxt_idx)
> {
>         struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);
> 
>         return data ? data->hart_id : INVALID_HART_ID;
> }
> 
> unsigned int acpi_get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
> {
>         struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);
> 
>         return data ? data->context_id : INVALID_CONTEXT;
> }
> 
> Or something like that. Hmm?
>
Nice!. Yes, this is better. Thanks a lot for the suggestion. Let me
update in the next revision.
 
> > +int acpi_get_imsic_mmio_info(u32 index, struct resource *res)
> > +{
> > +	if (index >= nr_rintc)
> > +		return -1;
> > +
> > +	res->start = rintc_acpi_data[index]->imsic_addr;
> > +	res->end = res->start + rintc_acpi_data[index]->imsic_size - 1;
> > +	res->flags = IORESOURCE_MEM;
> > +	return 0;
> > +}
> > +
> > +static struct fwnode_handle *ext_entc_get_gsi_domain_id(u32 gsi)
> > +{
> > +	return riscv_acpi_get_gsi_domain_id(gsi);
> > +}
> 
> This wrapper is required because using riscv_acpi_get_gsi_domain_id()
> directly is too obvious, right?
>
:-). Let me remove it.
 
> >  static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
> >  				       const unsigned long end)
> >  {
> > -	struct fwnode_handle *fn;
> >  	struct acpi_madt_rintc *rintc;
> > +	struct fwnode_handle *fn;
> > +	int rc;
> >  
> >  	rintc = (struct acpi_madt_rintc *)header;
> > +	rintc_acpi_data[nr_rintc] = kzalloc(sizeof(*rintc_acpi_data[0]), GFP_KERNEL);
> > +	if (!rintc_acpi_data[nr_rintc])
> > +		return -ENOMEM;
> > +
> > +	rintc_acpi_data[nr_rintc]->ext_intc_id = rintc->ext_intc_id;
> > +	rintc_acpi_data[nr_rintc]->hart_id = rintc->hart_id;
> > +	rintc_acpi_data[nr_rintc]->imsic_addr = rintc->imsic_addr;
> > +	rintc_acpi_data[nr_rintc]->imsic_size = rintc->imsic_size;
> > +	nr_rintc++;
> >  
> >  	/*
> >  	 * The ACPI MADT will have one INTC for each CPU (or HART)
> > @@ -273,7 +364,14 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
> >  		return -ENOMEM;
> >  	}
> >  
> > -	return riscv_intc_init_common(fn, &riscv_intc_chip);
> > +	rc = riscv_intc_init_common(fn, &riscv_intc_chip);
> > +	if (rc) {
> > +		irq_domain_free_fwnode(fn);
> > +		return rc;
> > +	}
> 
> This looks like a completely unrelated bug fix. Please don't mix functional
> changes and fixes.
> 
Makes sense. Let me create separate patch.

Thanks a lot for the review!
Sunil