On Mon, 07 Jul 2025 16:22:43 +0100,
Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx> wrote:
>
> On Sat, Jun 28, 2025 at 06:30:01PM +0100, Marc Zyngier wrote:
> > The xgene-msi driver uses an odd construct in the form of an
> > intermediate tracking structure, evidently designed to deal with
> > multiple instances of the MSI widget. However, the existing HW
> > only has one set, and it is obvious that there won't be new HW
> > coming down that particular line.
> >
> > Simplify the driver by using a bit of pointer arithmetic instead,
> > directly tracking the interrupt and avoiding extra memory allocation.
>
> A couple of nits, nothing else.
>
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> > drivers/pci/controller/pci-xgene-msi.c | 58 ++++++++------------------
> > 1 file changed, 17 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-xgene-msi.c b/drivers/pci/controller/pci-xgene-msi.c
> > index b3ac0125b3b40..4be79b9ff80df 100644
> > --- a/drivers/pci/controller/pci-xgene-msi.c
> > +++ b/drivers/pci/controller/pci-xgene-msi.c
> > @@ -24,19 +24,13 @@
> > #define NR_HW_IRQS 16
> > #define NR_MSI_VEC (IDX_PER_GROUP * IRQS_PER_IDX * NR_HW_IRQS)
> >
> > -struct xgene_msi_group {
> > - struct xgene_msi *msi;
> > - int gic_irq;
> > - u32 msi_grp;
> > -};
> > -
> > struct xgene_msi {
> > struct irq_domain *inner_domain;
> > u64 msi_addr;
> > void __iomem *msi_regs;
> > unsigned long *bitmap;
> > struct mutex bitmap_lock;
> > - struct xgene_msi_group *msi_groups;
> > + unsigned int gic_irq[NR_HW_IRQS];
> > };
> >
> > /* Global data */
> > @@ -261,27 +255,20 @@ static int xgene_msi_init_allocator(struct device *dev)
> >
> > mutex_init(&xgene_msi_ctrl->bitmap_lock);
> >
> > - xgene_msi_ctrl->msi_groups = devm_kcalloc(dev, NR_HW_IRQS,
> > - sizeof(struct xgene_msi_group),
> > - GFP_KERNEL);
> > - if (!xgene_msi_ctrl->msi_groups)
> > - return -ENOMEM;
> > -
> > return 0;
> > }
> >
> > static void xgene_msi_isr(struct irq_desc *desc)
> > {
> > + unsigned int *irqp = irq_desc_get_handler_data(desc);
> > struct irq_chip *chip = irq_desc_get_chip(desc);
> > struct xgene_msi *xgene_msi = xgene_msi_ctrl;
> > - struct xgene_msi_group *msi_groups;
> > int msir_index, msir_val, hw_irq, ret;
> > u32 intr_index, grp_select, msi_grp;
> >
> > chained_irq_enter(chip, desc);
> >
> > - msi_groups = irq_desc_get_handler_data(desc);
> > - msi_grp = msi_groups->msi_grp;
> > + msi_grp = irqp - xgene_msi->gic_irq;
> >
> > /*
> > * MSIINTn (n is 0..F) indicates if there is a pending MSI interrupt
> > @@ -341,35 +328,31 @@ static void xgene_msi_remove(struct platform_device *pdev)
> > cpuhp_remove_state(pci_xgene_online);
> > cpuhp_remove_state(CPUHP_PCI_XGENE_DEAD);
> >
> > - kfree(msi->msi_groups);
> > -
> > xgene_free_domains(msi);
> > }
> >
> > static int xgene_msi_hwirq_alloc(unsigned int cpu)
> > {
> > - struct xgene_msi *msi = xgene_msi_ctrl;
> > - struct xgene_msi_group *msi_group;
> > int i;
> > int err;
> >
> > for (i = cpu; i < NR_HW_IRQS; i += num_possible_cpus()) {
> > - msi_group = &msi->msi_groups[i];
> > + unsigned int irq = xgene_msi_ctrl->gic_irq[i];
> >
> > /*
> > * Statically allocate MSI GIC IRQs to each CPU core.
> > * With 8-core X-Gene v1, 2 MSI GIC IRQs are allocated
> > * to each core.
> > */
> > - irq_set_status_flags(msi_group->gic_irq, IRQ_NO_BALANCING);
> > - err = irq_set_affinity(msi_group->gic_irq, cpumask_of(cpu));
> > + irq_set_status_flags(irq, IRQ_NO_BALANCING);
> > + err = irq_set_affinity(irq, cpumask_of(cpu));
> > if (err) {
> > pr_err("failed to set affinity for GIC IRQ");
> > return err;
> > }
> >
> > - irq_set_chained_handler_and_data(msi_group->gic_irq,
> > - xgene_msi_isr, msi_group);
> > + irq_set_chained_handler_and_data(irq, xgene_msi_isr,
> > + &xgene_msi_ctrl->gic_irq[i]);
> > }
> >
> > return 0;
> > @@ -378,15 +361,12 @@ static int xgene_msi_hwirq_alloc(unsigned int cpu)
> > static int xgene_msi_hwirq_free(unsigned int cpu)
> > {
> > struct xgene_msi *msi = xgene_msi_ctrl;
> > - struct xgene_msi_group *msi_group;
> > int i;
> >
> > for (i = cpu; i < NR_HW_IRQS; i += num_possible_cpus()) {
> > - msi_group = &msi->msi_groups[i];
> > - if (!msi_group->gic_irq)
> > + if (!msi->gic_irq[i])
>
> In patch 5 we removed this check in xgene_msi_hwirq_alloc(), if it
> superfluous there it should be here too.
Hmmm, good point. I'll get rid of that one too.
>
> > continue;
> > - irq_set_chained_handler_and_data(msi_group->gic_irq, NULL,
> > - NULL);
> > + irq_set_chained_handler_and_data(msi->gic_irq[i], NULL, NULL);
> > }
> > return 0;
> > }
> > @@ -399,10 +379,9 @@ static const struct of_device_id xgene_msi_match_table[] = {
> > static int xgene_msi_probe(struct platform_device *pdev)
> > {
> > struct resource *res;
> > - int rc, irq_index;
>
> Just noticed, insignificant nit: don't see why moving irq_index to a
> local loop variable is required in this patch - fine to leave the
> code in the patch as-is - reporting it to make sure I have not
> missed anything.
Not required, just my own obsession with scope reduction of local
variables. I thought that given the magnitude of the changes, I might
as well give myself some artistic license! ;-)
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
