Re: [PATCH net-next v7 4/6] net: mana: Use GIC functions to allocate global EQs

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: Use GIC functions to allocate global EQs

This patch replaces the inline GDMA global interrupt setup in
mana_gd_setup_dyn_irqs(), mana_gd_setup_irqs(), and mana_gd_remove_irqs()
with calls to the new mana_gd_get_gic()/mana_gd_put_gic() helpers
introduced in the preceding patch.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 5930ab817056..850afdbd9c4e 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1888,30 +1888,13 @@ static int mana_gd_setup_dyn_irqs(struct pci_dev *pdev, int nvec)
>  	 * further used in irq_setup()
>  	 */
>  	for (i = 1; i <= nvec; i++) {
> -		gic = kzalloc_obj(*gic);
> +		gic = mana_gd_get_gic(gc, false, &i);
>  		if (!gic) {
>  			err = -ENOMEM;
>  			goto free_irq;
>  		}
> -		gic->handler = mana_gd_process_eq_events;
> -		INIT_LIST_HEAD(&gic->eq_list);
> -		spin_lock_init(&gic->lock);
> -
> -		snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_q%d@pci:%s",
> -			 i - 1, pci_name(pdev));

[Low]
Before this patch the per-queue interrupts in mana_gd_setup_dyn_irqs()
and mana_gd_setup_irqs() were named as:

    "mana_q%d@pci:%s", i - 1, pci_name(pdev)

so they appeared as mana_q0, mana_q1, ... in /proc/interrupts.

After this patch, allocation goes through mana_gd_get_gic(), which
names them using gic->msi (equal to i, not i - 1):

    snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_msi%d@pci:%s",
             gic->msi, pci_name(dev));

so the same vectors now appear as mana_msi1, mana_msi2, ...  The
mana_hwc@pci:%s name is preserved, but every per-queue vector is both
renamed (mana_q -> mana_msi) and renumbered (0-based -> 1-based).

Is this rename intentional?  The commit message describes the change
as replacing "the GDMA global interrupt setup code with the new GIC
allocation and release functions", which reads as a pure refactor.  If
the rename is intended, could it be mentioned in the changelog so that
operators relying on the mana_q prefix in /proc/interrupts are not
surprised?

> -
> -		/* one pci vector is already allocated for HWC */
> -		irqs[i - 1] = pci_irq_vector(pdev, i);
> -		if (irqs[i - 1] < 0) {
> -			err = irqs[i - 1];
> -			goto free_current_gic;
> -		}
> -
> -		err = request_irq(irqs[i - 1], mana_gd_intr, 0, gic->name, gic);
> -		if (err)
> -			goto free_current_gic;
>  
> -		xa_store(&gc->irq_contexts, i, gic, GFP_KERNEL);
> +		irqs[i - 1] = gic->irq;
>  	}
>  
>  	/*
> @@ -1933,19 +1916,11 @@ static int mana_gd_setup_dyn_irqs(struct pci_dev *pdev, int nvec)
>  	kfree(irqs);
>  	return 0;
>  
> -free_current_gic:
> -	kfree(gic);
>  free_irq:
>  	for (i -= 1; i > 0; i--) {
>  		irq = pci_irq_vector(pdev, i);
> -		gic = xa_load(&gc->irq_contexts, i);
> -		if (WARN_ON(!gic))
> -			continue;
> -
>  		irq_update_affinity_hint(irq, NULL);
> -		free_irq(irq, gic);
> -		xa_erase(&gc->irq_contexts, i);
> -		kfree(gic);
> +		mana_gd_put_gic(gc, false, i);
>  	}

[Low]
Is the irq_update_affinity_hint(irq, NULL) call here still needed?

Looking at mana_gd_put_gic():

    if (!refcount_dec_and_test(&gic->refcount))
            goto out;

    irq = gic->irq;

    irq_update_affinity_hint(irq, NULL);
    free_irq(irq, gic);

When the last reference is dropped (which is the case on every caller in
this patch, since the refcount is 1 here), the helper already clears the
affinity hint before calling free_irq().  The explicit call in the caller
duplicates that work.

The duplication is harmless today because irq_update_affinity_hint() is
idempotent, but could the clearing of the affinity hint be left entirely
to mana_gd_put_gic()?  Otherwise, if these helpers are ever reused in a
context where the refcount is greater than one, the caller will clear an
affinity hint that the helper would correctly have left intact.

> @@ -1966,34 +1941,13 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev, int nvec)
>  	start_irqs = irqs;
>  
>  	for (i = 0; i < nvec; i++) {
> -		gic = kzalloc_obj(*gic);
> +		gic = mana_gd_get_gic(gc, false, &i);
>  		if (!gic) {
>  			err = -ENOMEM;
>  			goto free_irq;
>  		}
>  
> -		gic->handler = mana_gd_process_eq_events;
> -		INIT_LIST_HEAD(&gic->eq_list);
> -		spin_lock_init(&gic->lock);
> -
> -		if (!i)
> -			snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_hwc@pci:%s",
> -				 pci_name(pdev));
> -		else
> -			snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_q%d@pci:%s",
> -				 i - 1, pci_name(pdev));

[Low]
The same rename applies here for the non-HWC vectors (mana_q%d with
i - 1 becomes mana_msi%d with i inside mana_gd_get_gic()).  Could the
commit message note this user-visible change in /proc/interrupts?

[ ... ]

> @@ -2022,19 +1976,11 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev, int nvec)
>  	kfree(start_irqs);
>  	return 0;
>  
> -free_current_gic:
> -	kfree(gic);
>  free_irq:
>  	for (i -= 1; i >= 0; i--) {
>  		irq = pci_irq_vector(pdev, i);
> -		gic = xa_load(&gc->irq_contexts, i);
> -		if (WARN_ON(!gic))
> -			continue;
> -
>  		irq_update_affinity_hint(irq, NULL);
> -		free_irq(irq, gic);
> -		xa_erase(&gc->irq_contexts, i);
> -		kfree(gic);
> +		mana_gd_put_gic(gc, false, i);
>  	}

[Low]
Same observation as in mana_gd_setup_dyn_irqs(): the explicit
irq_update_affinity_hint(irq, NULL) duplicates what
mana_gd_put_gic() already does when it drops the last reference.  Could
this call be dropped from the caller?

> @@ -2109,26 +2055,20 @@ static int mana_gd_setup_remaining_irqs(struct pci_dev *pdev)
>  static void mana_gd_remove_irqs(struct pci_dev *pdev)
>  {
>  	struct gdma_context *gc = pci_get_drvdata(pdev);
> -	struct gdma_irq_context *gic;
>  	int irq, i;
>  
>  	if (gc->max_num_msix < 1)
>  		return;
>  
>  	for (i = 0; i < gc->max_num_msix; i++) {
> -		irq = pci_irq_vector(pdev, i);
> -		if (irq < 0)
> -			continue;
> -
> -		gic = xa_load(&gc->irq_contexts, i);
> -		if (WARN_ON(!gic))
> +		if (!xa_load(&gc->irq_contexts, i))
>  			continue;
>  
>  		/* Need to clear the hint before free_irq */
> +		irq = pci_irq_vector(pdev, i);
>  		irq_update_affinity_hint(irq, NULL);
> -		free_irq(irq, gic);
> -		xa_erase(&gc->irq_contexts, i);
> -		kfree(gic);
> +
> +		mana_gd_put_gic(gc, false, i);
>  	}

[Low]
Same pattern in mana_gd_remove_irqs(): the comment says "Need to clear
the hint before free_irq", but the actual free_irq() now happens inside
mana_gd_put_gic(), which itself calls irq_update_affinity_hint(irq, NULL)
right before free_irq().  Could the explicit call (and comment) be
removed so that the affinity-hint clearing lives in one place?