| Message ID | 20240823195657.31588-10-michael.chan@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: Update for net-next | expand |
On Fri, Aug 23, 2024 at 12:56:57PM -0700, Michael Chan wrote: > A range of MSIX vectors are allocated at initialization for the number > needed for RocE and L2. During run-time, if the user increases or > decreases the number of L2 rings, all the MSIX vectors have to be > freed and a new range has to be allocated. This is not optimal and > causes disruptions to RoCE traffic every time there is a change in L2 > MSIX. > > If the system supports dynamic MSIX allocations, use dynamic > allocation to add new L2 MSIX vectors or free unneeded L2 MSIX > vectors. RoCE traffic is not affected using this scheme. > > Reviewed-by: Hongguang Gao <hongguang.gao@broadcom.com> > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> > Reviewed-by: Simon Horman <horms@kernel.org> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > --- > v2: Fix typo in changelog > --- > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 57 +++++++++++++++++++++-- > 1 file changed, 54 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index fa4115f6dafe..39dc67dbe9b2 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -10622,6 +10622,43 @@ static void bnxt_setup_msix(struct bnxt *bp) > > static int bnxt_init_int_mode(struct bnxt *bp); > > +static int bnxt_add_msix(struct bnxt *bp, int total) > +{ > + int i; > + > + if (bp->total_irqs >= total) > + return total; > + > + for (i = bp->total_irqs; i < total; i++) { > + struct msi_map map; > + > + map = pci_msix_alloc_irq_at(bp->pdev, i, NULL); > + if (map.index < 0) > + break; > + bp->irq_tbl[i].vector = map.virq; > + bp->total_irqs++; > + } > + return bp->total_irqs; > +} > + > +static int bnxt_trim_msix(struct bnxt *bp, int total) > +{ > + int i; > + > + if (bp->total_irqs <= total) > + return total; > + > + for (i = bp->total_irqs; i > total; i--) { > + struct msi_map map; > + > + map.index = i - 1; > + map.virq = bp->irq_tbl[i - 1].vector; > + pci_msix_free_irq(bp->pdev, map); > + bp->total_irqs--; > + } > + return bp->total_irqs; > +} Patch looks fine, treat it only as suggestion: You can save some lines of code by merging this two function. static int bnxt_change_msix(struct bnxt *bp, int total) { int i; /* add MSI-Xs if needed */ for (i = bp->total_irqs; i < total; i++) { ... } /* remove MSI-Xs if needed */ for (i = bp->total_irqs; i > total; i--) { ... } return bp->total_irqs; } > + > static int bnxt_setup_int_mode(struct bnxt *bp) > { > int rc; > @@ -10788,6 +10825,7 @@ static void bnxt_clear_int_mode(struct bnxt *bp) > int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init) > { > bool irq_cleared = false; > + bool irq_change = false; > int tcs = bp->num_tc; > int irqs_required; > int rc; > @@ -10806,15 +10844,28 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init) > } > > if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) { > - bnxt_ulp_irq_stop(bp); > - bnxt_clear_int_mode(bp); > - irq_cleared = true; > + irq_change = true; > + if (!pci_msix_can_alloc_dyn(bp->pdev)) { > + bnxt_ulp_irq_stop(bp); > + bnxt_clear_int_mode(bp); > + irq_cleared = true; > + } > } > rc = __bnxt_reserve_rings(bp); > if (irq_cleared) { > if (!rc) > rc = bnxt_init_int_mode(bp); > bnxt_ulp_irq_restart(bp, rc); > + } else if (irq_change && !rc) { > + int total; > + > + if (irqs_required > bp->total_irqs) > + total = bnxt_add_msix(bp, irqs_required); > + else > + total = bnxt_trim_msix(bp, irqs_required); > + > + if (total != irqs_required) > + rc = -ENOSPC; and here if (bnxt_change_msix(bp, irqs_required) != irqs_required) rc = -ENOSPC; Thanks, Michal > } > if (rc) { > netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc); > -- > 2.30.1 >
On Tue, 27 Aug 2024 09:32:03 +0200 Michal Swiatkowski wrote: > > +static int bnxt_add_msix(struct bnxt *bp, int total) > > +{ > > + int i; > > + > > + if (bp->total_irqs >= total) > > + return total; > > + > > + for (i = bp->total_irqs; i < total; i++) { > > + struct msi_map map; > > + > > + map = pci_msix_alloc_irq_at(bp->pdev, i, NULL); > > + if (map.index < 0) > > + break; > > + bp->irq_tbl[i].vector = map.virq; > > + bp->total_irqs++; > > + } > > + return bp->total_irqs; > > +} > > + > > +static int bnxt_trim_msix(struct bnxt *bp, int total) > > +{ > > + int i; > > + > > + if (bp->total_irqs <= total) > > + return total; > > + > > + for (i = bp->total_irqs; i > total; i--) { > > + struct msi_map map; > > + > > + map.index = i - 1; > > + map.virq = bp->irq_tbl[i - 1].vector; > > + pci_msix_free_irq(bp->pdev, map); > > + bp->total_irqs--; > > + } > > + return bp->total_irqs; > > +} > > Patch looks fine, treat it only as suggestion: > > You can save some lines of code by merging this two function. > > static int bnxt_change_msix(struct bnxt *bp, int total) > { > int i; > > /* add MSI-Xs if needed */ > for (i = bp->total_irqs; i < total; i++) { > ... > } > > /* remove MSI-Xs if needed */ > for (i = bp->total_irqs; i > total; i--) { > ... > } > > return bp->total_irqs; > } > > + } else if (irq_change && !rc) { > > + int total; > > + > > + if (irqs_required > bp->total_irqs) > > + total = bnxt_add_msix(bp, irqs_required); > > + else > > + total = bnxt_trim_msix(bp, irqs_required); > > + > > + if (total != irqs_required) > > + rc = -ENOSPC; > > and here > > if (bnxt_change_msix(bp, irqs_required) != irqs_required) > rc = -ENOSPC;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index fa4115f6dafe..39dc67dbe9b2 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -10622,6 +10622,43 @@ static void bnxt_setup_msix(struct bnxt *bp) static int bnxt_init_int_mode(struct bnxt *bp); +static int bnxt_add_msix(struct bnxt *bp, int total) +{ + int i; + + if (bp->total_irqs >= total) + return total; + + for (i = bp->total_irqs; i < total; i++) { + struct msi_map map; + + map = pci_msix_alloc_irq_at(bp->pdev, i, NULL); + if (map.index < 0) + break; + bp->irq_tbl[i].vector = map.virq; + bp->total_irqs++; + } + return bp->total_irqs; +} + +static int bnxt_trim_msix(struct bnxt *bp, int total) +{ + int i; + + if (bp->total_irqs <= total) + return total; + + for (i = bp->total_irqs; i > total; i--) { + struct msi_map map; + + map.index = i - 1; + map.virq = bp->irq_tbl[i - 1].vector; + pci_msix_free_irq(bp->pdev, map); + bp->total_irqs--; + } + return bp->total_irqs; +} + static int bnxt_setup_int_mode(struct bnxt *bp) { int rc; @@ -10788,6 +10825,7 @@ static void bnxt_clear_int_mode(struct bnxt *bp) int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init) { bool irq_cleared = false; + bool irq_change = false; int tcs = bp->num_tc; int irqs_required; int rc; @@ -10806,15 +10844,28 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init) } if (irq_re_init && BNXT_NEW_RM(bp) && irqs_required != bp->total_irqs) { - bnxt_ulp_irq_stop(bp); - bnxt_clear_int_mode(bp); - irq_cleared = true; + irq_change = true; + if (!pci_msix_can_alloc_dyn(bp->pdev)) { + bnxt_ulp_irq_stop(bp); + bnxt_clear_int_mode(bp); + irq_cleared = true; + } } rc = __bnxt_reserve_rings(bp); if (irq_cleared) { if (!rc) rc = bnxt_init_int_mode(bp); bnxt_ulp_irq_restart(bp, rc); + } else if (irq_change && !rc) { + int total; + + if (irqs_required > bp->total_irqs) + total = bnxt_add_msix(bp, irqs_required); + else + total = bnxt_trim_msix(bp, irqs_required); + + if (total != irqs_required) + rc = -ENOSPC; } if (rc) { netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);