| Message ID | 20240425212624.2703397-1-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] bnxt: fix bnxt_get_avail_msix() returning negative values | expand |
On Thu, Apr 25, 2024 at 2:26 PM David Wei <dw@davidwei.uk> wrote: > > Current net-next/main does not boot for older chipsets e.g. Stratus. > > Sample dmesg: > [ 11.368315] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Able to reserve only 0 out of 9 requested RX rings > [ 11.390181] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Unable to reserve tx rings > [ 11.438780] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): 2nd rings reservation failed. > [ 11.487559] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Not enough rings available. > [ 11.506012] bnxt_en 0000:02:00.0: probe with driver bnxt_en failed with error -12 > > This is caused by bnxt_get_avail_msix() returning a negative value for > these chipsets not using the new resource manager i.e. !BNXT_NEW_RM. > This in turn causes hwr.cp in __bnxt_reserve_rings() to be set to 0. > > In the current call stack, __bnxt_reserve_rings() is called from > bnxt_set_dflt_rings() before bnxt_init_int_mode(). Therefore, > bp->total_irqs is always 0 and for !BNXT_NEW_RM bnxt_get_avail_msix() > always returns a negative number. Thanks for the patch. I'm still trying to understand the flow on this older NIC. If BNXT_NEW_RM() is not true, shouldn't bnxt_need_reserve_ring() return false from the top of __bnxt_reserve_rings()? Ah perhaps this NIC is using hwrm_spec_code >= 0x10601 and !BNXT_NEW_RM(). In that case bnxt_need_reserve_rings() will return true because we have to reserve only the TX rings. Let me review this code path some more. Thanks again.
On 2024-04-25 3:25 pm, Michael Chan wrote: > On Thu, Apr 25, 2024 at 2:26 PM David Wei <dw@davidwei.uk> wrote: >> >> Current net-next/main does not boot for older chipsets e.g. Stratus. >> >> Sample dmesg: >> [ 11.368315] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Able to reserve only 0 out of 9 requested RX rings >> [ 11.390181] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Unable to reserve tx rings >> [ 11.438780] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): 2nd rings reservation failed. >> [ 11.487559] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Not enough rings available. >> [ 11.506012] bnxt_en 0000:02:00.0: probe with driver bnxt_en failed with error -12 >> >> This is caused by bnxt_get_avail_msix() returning a negative value for >> these chipsets not using the new resource manager i.e. !BNXT_NEW_RM. >> This in turn causes hwr.cp in __bnxt_reserve_rings() to be set to 0. >> >> In the current call stack, __bnxt_reserve_rings() is called from >> bnxt_set_dflt_rings() before bnxt_init_int_mode(). Therefore, >> bp->total_irqs is always 0 and for !BNXT_NEW_RM bnxt_get_avail_msix() >> always returns a negative number. > > Thanks for the patch. I'm still trying to understand the flow on this > older NIC. > > If BNXT_NEW_RM() is not true, shouldn't bnxt_need_reserve_ring() > return false from the top of __bnxt_reserve_rings()? > > Ah perhaps this NIC is using hwrm_spec_code >= 0x10601 and > !BNXT_NEW_RM(). In that case bnxt_need_reserve_rings() will return > true because we have to reserve only the TX rings. Let me review this > code path some more. Thanks again. Yes, hwrm_spec_code >= 0x10601 and the first conditional in bnxt_need_reserve_rings() returns true.
On Thu, Apr 25, 2024 at 2:26 PM David Wei <dw@davidwei.uk> wrote: > > Current net-next/main does not boot for older chipsets e.g. Stratus. > > Sample dmesg: > [ 11.368315] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Able to reserve only 0 out of 9 requested RX rings > [ 11.390181] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Unable to reserve tx rings > [ 11.438780] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): 2nd rings reservation failed. > [ 11.487559] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Not enough rings available. > [ 11.506012] bnxt_en 0000:02:00.0: probe with driver bnxt_en failed with error -12 > > This is caused by bnxt_get_avail_msix() returning a negative value for > these chipsets not using the new resource manager i.e. !BNXT_NEW_RM. > This in turn causes hwr.cp in __bnxt_reserve_rings() to be set to 0. > > In the current call stack, __bnxt_reserve_rings() is called from > bnxt_set_dflt_rings() before bnxt_init_int_mode(). Therefore, > bp->total_irqs is always 0 and for !BNXT_NEW_RM bnxt_get_avail_msix() > always returns a negative number. > > Comparing with a newer chipset e.g. Thor and the codepath for > BNXT_NEW_RM, I believe the intent is for bnxt_get_avail_msix() to always > return >= 0. Fix the issue by using max(). Historically, MSIX vectors were requested by the RoCE driver during run-time and we used bnxt_get_avail_msix() for this purpose. Today, RoCE MSIX vectors are statically set aside. bnxt_get_avail_msix() should only be called for the BNXT_NEW_RM() case to reserve the MSIX ahead of time for RoCE use. bnxt_get_avail_msix() should also be simplified to handle the BNXT_NEW_RM() case only. I think that's the most correct fix. Thanks. > > Alternatively, perhaps __bnxt_reserve_rings() should be reverted back. > But there may be paths calling into it where bnxt_get_avail_msix() > returns a positive integer. >
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index be96bb494ae6..06b7a963bbbd 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -10486,14 +10486,11 @@ int bnxt_get_avail_msix(struct bnxt *bp, int num) max_idx = min_t(int, bp->total_irqs, max_cp); avail_msix = max_idx - bp->cp_nr_rings; if (!BNXT_NEW_RM(bp) || avail_msix >= num) - return avail_msix; + return max(avail_msix, 0); - if (max_irq < total_req) { + if (max_irq < total_req) num = max_irq - bp->cp_nr_rings; - if (num <= 0) - return 0; - } - return num; + return max(num, 0); } static int bnxt_get_num_msix(struct bnxt *bp)
Current net-next/main does not boot for older chipsets e.g. Stratus. Sample dmesg: [ 11.368315] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Able to reserve only 0 out of 9 requested RX rings [ 11.390181] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Unable to reserve tx rings [ 11.438780] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): 2nd rings reservation failed. [ 11.487559] bnxt_en 0000:02:00.0 (unnamed net_device) (uninitialized): Not enough rings available. [ 11.506012] bnxt_en 0000:02:00.0: probe with driver bnxt_en failed with error -12 This is caused by bnxt_get_avail_msix() returning a negative value for these chipsets not using the new resource manager i.e. !BNXT_NEW_RM. This in turn causes hwr.cp in __bnxt_reserve_rings() to be set to 0. In the current call stack, __bnxt_reserve_rings() is called from bnxt_set_dflt_rings() before bnxt_init_int_mode(). Therefore, bp->total_irqs is always 0 and for !BNXT_NEW_RM bnxt_get_avail_msix() always returns a negative number. Comparing with a newer chipset e.g. Thor and the codepath for BNXT_NEW_RM, I believe the intent is for bnxt_get_avail_msix() to always return >= 0. Fix the issue by using max(). Alternatively, perhaps __bnxt_reserve_rings() should be reverted back. But there may be paths calling into it where bnxt_get_avail_msix() returns a positive integer. Fixes: d630624ebd70 ("bnxt_en: Utilize ulp client resources if RoCE is not registered") Signed-off-by: David Wei <dw@davidwei.uk> --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)