Received-SPF: pass (google.com: domain of linux-kernel+bounces-188064-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: Sunil V L <sunilvl@ventanamicro.com>,
 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
Cc: 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>, Sunil V L <sunilvl@ventanamicro.com>
Subject: Re: [PATCH v5 14/17] irqchip/riscv-imsic: Add ACPI support
In-Reply-To: <20240501121742.1215792-15-sunilvl@ventanamicro.com>
References: <20240501121742.1215792-1-sunilvl@ventanamicro.com>
 <20240501121742.1215792-15-sunilvl@ventanamicro.com>
Date: Fri, 24 May 2024 00:00:21 +0200
Message-ID: <871q5sfatm.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Wed, May 01 2024 at 17:47, Sunil V L wrote:

> RISC-V IMSIC interrupt controller provides IPI and MSI support.
> Currently, DT based drivers setup the IPI feature early during boot but
> defer setting up the MSI functionality. However, in ACPI systems, ACPI,
> both IPI and MSI features need to be initialized early itself.

Why?

> +
> +#ifdef CONFIG_ACPI
> +
> +static struct fwnode_handle *imsic_acpi_fwnode;
> +
> +struct fwnode_handle *imsic_acpi_get_fwnode(struct device *dev)

Why is this function global? It's only used in the very same file and
under the same CONFIG_ACPI #ifdef, no?

> +{
> +	return imsic_acpi_fwnode;
> +}
> +
> +static int __init imsic_early_acpi_init(union acpi_subtable_headers *header,
> +					const unsigned long end)
> +{
> +	struct acpi_madt_imsic *imsic = (struct acpi_madt_imsic *)header;
> +	int rc;
> +
> +	imsic_acpi_fwnode = irq_domain_alloc_named_fwnode("imsic");
> +	if (!imsic_acpi_fwnode) {
> +		pr_err("unable to allocate IMSIC FW node\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Setup IMSIC state */
> +	rc = imsic_setup_state(imsic_acpi_fwnode, (void *)imsic);

Pointless (void *) cast.

> +	if (rc) {
> +		pr_err("%pfwP: failed to setup state (error %d)\n", imsic_acpi_fwnode, rc);
> +		return rc;
> +	}
> +
> +	/* Do early setup of IMSIC state and IPIs */
> +	rc = imsic_early_probe(imsic_acpi_fwnode);
> +	if (rc)
> +		return rc;
> +
> +	rc = imsic_platform_acpi_probe(imsic_acpi_fwnode);
> +
> +#ifdef CONFIG_PCI
> +	if (!rc)
> +		pci_msi_register_fwnode_provider(&imsic_acpi_get_fwnode);
> +#endif
> +
> +	return rc;

Any error return in this function leaks the firmware node and probably
some more stuff.

> +}
> +
> +IRQCHIP_ACPI_DECLARE(riscv_imsic, ACPI_MADT_TYPE_IMSIC, NULL,
> +		     1, imsic_early_acpi_init);
> +#endif

..

> -	/* Find number of interrupt identities */
> -	rc = of_property_read_u32(to_of_node(fwnode), "riscv,num-ids",
> -				  &global->nr_ids);
> -	if (rc) {
> -		pr_err("%pfwP: number of interrupt identities not found\n", fwnode);
> -		return rc;
> +		/* Find number of guest interrupt identities */
> +		rc = of_property_read_u32(to_of_node(fwnode), "riscv,num-guest-ids",
> +					  &global->nr_guest_ids);
> +		if (rc)
> +			global->nr_guest_ids = global->nr_ids;
> +	} else {
> +		global->guest_index_bits = imsic->guest_index_bits;
> +		global->hart_index_bits = imsic->hart_index_bits;
> +		global->group_index_bits = imsic->group_index_bits;
> +		global->group_index_shift = imsic->group_index_shift;
> +		global->nr_ids = imsic->num_ids;
> +		global->nr_guest_ids = imsic->num_guest_ids;
>  	}

Seriously?

Why can't you just split out the existing DT code into a separate
function in an initial patch which avoulds all of this unreviewable
churn of making the DT stuff indented ?

> +#ifdef CONFIG_ACPI
> +int imsic_platform_acpi_probe(struct fwnode_handle *fwnode);
> +struct fwnode_handle *imsic_acpi_get_fwnode(struct device *dev);
> +#else
> +static inline struct fwnode_handle *imsic_acpi_get_fwnode(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif

Oh well.

>  #endif

Thanks,

        tglx