Re: [PATCH 12/12] PCI: xgene-msi: Restructure handler setup/teardown

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Sat, Jun 28, 2025 at 06:30:05PM +0100, Marc Zyngier wrote:
> Another utterly pointless aspect of the xgene-msi driver is that
> it is built around CPU hotplug. Which is quite amusing since this
> is one of the few arm64 platforms that, by construction, cannot
> do CPU hotplug in a supported way (no EL3, no PSCI, no luck).
> 
> Drop the CPU hotplug nonsense and just setup the IRQs and handlers
> in a less overdesigned way, grouping things more logically in the
> process.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/pci/controller/pci-xgene-msi.c | 109 +++++++++----------------
>  1 file changed, 37 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c
> index a22a6df7808c7..9f05c2a12da94 100644
> --- a/drivers/pci/controller/pci-xgene-msi.c
> +++ b/drivers/pci/controller/pci-xgene-msi.c
> @@ -216,12 +216,6 @@ static int xgene_allocate_domains(struct device_node *node,
>  	return msi->inner_domain ? 0 : -ENOMEM;
>  }
>  
> -static void xgene_free_domains(struct xgene_msi *msi)
> -{
> -	if (msi->inner_domain)
> -		irq_domain_remove(msi->inner_domain);
> -}
> -
>  static int xgene_msi_init_allocator(struct device *dev)
>  {
>  	xgene_msi_ctrl->bitmap = devm_bitmap_zalloc(dev, NR_MSI_VEC, GFP_KERNEL);
> @@ -271,26 +265,48 @@ static void xgene_msi_isr(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> -static enum cpuhp_state pci_xgene_online;
> -
>  static void xgene_msi_remove(struct platform_device *pdev)
>  {
> -	struct xgene_msi *msi = platform_get_drvdata(pdev);
> -
> -	if (pci_xgene_online)
> -		cpuhp_remove_state(pci_xgene_online);
> -	cpuhp_remove_state(CPUHP_PCI_XGENE_DEAD);

No question on the patch - just noticed we could remove
CPUHP_PCI_XGENE_DEAD from cpuhp_state since it would become
unused AFAICS.

Thanks,
Lorenzo

> +	for (int i = 0; i < NR_HW_IRQS; i++) {
> +		unsigned int irq = xgene_msi_ctrl->gic_irq[i];
> +		if (!irq)
> +			continue;
> +		irq_set_chained_handler_and_data(irq, NULL, NULL);
> +	}
>  
> -	xgene_free_domains(msi);
> +	if (xgene_msi_ctrl->inner_domain)
> +		irq_domain_remove(xgene_msi_ctrl->inner_domain);
>  }
>  
> -static int xgene_msi_hwirq_alloc(unsigned int cpu)
> +static int xgene_msi_handler_setup(struct platform_device *pdev)
>  {
> +	struct xgene_msi *xgene_msi = xgene_msi_ctrl;
>  	int i;
> -	int err;
>  
> -	for (i = cpu; i < NR_HW_IRQS; i += num_possible_cpus()) {
> -		unsigned int irq = xgene_msi_ctrl->gic_irq[i];
> +	for (i = 0; i < NR_HW_IRQS; i++) {
> +		u32 msi_val;
> +		int irq, err;
> +
> +		/*
> +		 * MSInIRx registers are read-to-clear; before registering
> +		 * interrupt handlers, read all of them to clear spurious
> +		 * interrupts that may occur before the driver is probed.
> +		 */
> +		for (int msi_idx = 0; msi_idx < IDX_PER_GROUP; msi_idx++)
> +			xgene_msi_ir_read(xgene_msi, i, msi_idx);
> +
> +		/* Read MSIINTn to confirm */
> +		msi_val = xgene_msi_int_read(xgene_msi, i);
> +		if (msi_val) {
> +			dev_err(&pdev->dev, "Failed to clear spurious IRQ\n");
> +			return EINVAL;
> +		}
> +
> +		irq = platform_get_irq(pdev, i);
> +		if (irq < 0)
> +			return irq;
> +
> +		xgene_msi->gic_irq[i] = irq;
>  
>  		/*
>  		 * Statically allocate MSI GIC IRQs to each CPU core.
> @@ -298,7 +314,7 @@ static int xgene_msi_hwirq_alloc(unsigned int cpu)
>  		 * to each core.
>  		 */
>  		irq_set_status_flags(irq, IRQ_NO_BALANCING);
> -		err = irq_set_affinity(irq, cpumask_of(cpu));
> +		err = irq_set_affinity(irq, cpumask_of(i % num_possible_cpus()));
>  		if (err) {
>  			pr_err("failed to set affinity for GIC IRQ");
>  			return err;
> @@ -311,19 +327,6 @@ static int xgene_msi_hwirq_alloc(unsigned int cpu)
>  	return 0;
>  }
>  
> -static int xgene_msi_hwirq_free(unsigned int cpu)
> -{
> -	struct xgene_msi *msi = xgene_msi_ctrl;
> -	int i;
> -
> -	for (i = cpu; i < NR_HW_IRQS; i += num_possible_cpus()) {
> -		if (!msi->gic_irq[i])
> -			continue;
> -		irq_set_chained_handler_and_data(msi->gic_irq[i], NULL, NULL);
> -	}
> -	return 0;
> -}
> -
>  static const struct of_device_id xgene_msi_match_table[] = {
>  	{.compatible = "apm,xgene1-msi"},
>  	{},
> @@ -333,7 +336,6 @@ static int xgene_msi_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
>  	struct xgene_msi *xgene_msi;
> -	u32 msi_val, msi_idx;
>  	int rc;
>  
>  	xgene_msi_ctrl = devm_kzalloc(&pdev->dev, sizeof(*xgene_msi_ctrl),
> @@ -343,8 +345,6 @@ static int xgene_msi_probe(struct platform_device *pdev)
>  
>  	xgene_msi = xgene_msi_ctrl;
>  
> -	platform_set_drvdata(pdev, xgene_msi);
> -
>  	xgene_msi->msi_regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>  	if (IS_ERR(xgene_msi->msi_regs)) {
>  		rc = PTR_ERR(xgene_msi->msi_regs);
> @@ -364,48 +364,13 @@ static int xgene_msi_probe(struct platform_device *pdev)
>  		goto error;
>  	}
>  
> -	for (int irq_index = 0; irq_index < NR_HW_IRQS; irq_index++) {
> -		rc = platform_get_irq(pdev, irq_index);
> -		if (rc < 0)
> -			goto error;
> -
> -		xgene_msi->gic_irq[irq_index] = rc;
> -	}
> -
> -	/*
> -	 * MSInIRx registers are read-to-clear; before registering
> -	 * interrupt handlers, read all of them to clear spurious
> -	 * interrupts that may occur before the driver is probed.
> -	 */
> -	for (int irq_index = 0; irq_index < NR_HW_IRQS; irq_index++) {
> -		for (msi_idx = 0; msi_idx < IDX_PER_GROUP; msi_idx++)
> -			xgene_msi_ir_read(xgene_msi, irq_index, msi_idx);
> -
> -		/* Read MSIINTn to confirm */
> -		msi_val = xgene_msi_int_read(xgene_msi, irq_index);
> -		if (msi_val) {
> -			dev_err(&pdev->dev, "Failed to clear spurious IRQ\n");
> -			rc = -EINVAL;
> -			goto error;
> -		}
> -	}
> -
> -	rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "pci/xgene:online",
> -			       xgene_msi_hwirq_alloc, NULL);
> -	if (rc < 0)
> -		goto err_cpuhp;
> -	pci_xgene_online = rc;
> -	rc = cpuhp_setup_state(CPUHP_PCI_XGENE_DEAD, "pci/xgene:dead", NULL,
> -			       xgene_msi_hwirq_free);
> +	rc = xgene_msi_handler_setup(pdev);
>  	if (rc)
> -		goto err_cpuhp;
> +		goto error;
>  
>  	dev_info(&pdev->dev, "APM X-Gene PCIe MSI driver loaded\n");
>  
>  	return 0;
> -
> -err_cpuhp:
> -	dev_err(&pdev->dev, "failed to add CPU MSI notifier\n");
>  error:
>  	xgene_msi_remove(pdev);
>  	return rc;
> -- 
> 2.39.2
>