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@xxxxxxxxxx>
> ---
> 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
>
