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
