Page MenuHomeFreeBSD

if_bnxt: add MPC (Mid-Path Channel) ring infrastructure
Needs ReviewPublic

Authored by sumit.saxena_broadcom.com on Mon, Aug 3, 12:13 PM.

Details

Summary

Add MPC ring support: a pair of extra, driver-private
TX/completion/notification rings the firmware uses as a side-channel
for offload commands. The only current consumer is kTLS TX crypto
command submission; RCE/CFA channel types are not yet implemented by
any consumer.

Adds MPC ring allocation/teardown, HWRM ring alloc/free, a private
interrupt path for the MPC NQ ring, and the TX submit/completion
path. The RoCE-only IRQ table builder is split into a generic,
exported bnxt_populate_irq(softc, irq_count) that both MPC and RoCE
use, plus an incremental bnxt_populate_irq_roce() that grows the
table on top of whatever is already there, so IRQ rids stay
consistent regardless of attach order.

This patch does not link standalone: bnxt_mpc_cmp() calls into
kTLS's completion handler, added by a later patch.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

I think every mpc allocation is leaked on detach. Eg, frees are done from bnxt_queues_free(). iflib runs bnxt_detach(), which sets mpc_info = NULL. And then runs bnxt_queues_free(), which gates freeing on if (!mpc) return;

Can you make the new code style (9) compliant please?

sys/dev/bnxt/bnxt_en/bnxt_mpc.c
295

Shouldn't this be txr->id * 0x80 ?

647

Return value seems to be ignored.. should it be a void?

815

I think this is off by one. Consider ring size 4, cons = 3, prod = 0. So the true number of used entries is 1. But this gives (4-3 + 0 + 1) = 2.

818

I think this is off by 1 as well. Since you can't have prod==cons, then in the case where ring size = 4 and we've used 3, the avail is 0. Eg, shouldn't this be txr->ring_size - 1 - used) ?

1013

What's the point of this? you don't seem to write to *raw_cons, so no need to restore it.

sys/dev/bnxt/bnxt_en/bnxt_mpc.h
93

Doesn't this already exist in bnxt.h?

94

Space around the &

sys/dev/bnxt/bnxt_en/if_bnxt.c
466

Maybe a dumb question, but shouldn't this depend on whether or not the NIC supports ROCE and crypto? Eg, it seems like we're stealing msi-x vectors from low-end chips that do not support either.

690

I think you need to check return values from then alloc functions above..

sys/dev/bnxt/bnxt_en/bnxt_mpc.c
295

MPC infrastructure is only applies to Thor2(P7) chips. Older NICs will exit from check at line#240. I think we can just call 'txr->doorbell = softc->legacy_db_size'.

647

Ack, will fix up this.

815

yes, it's off by one. I think below change should fix this:

@@ -816,7 +816,7 @@ static inline uint32_t bnxt_tx_avail(struct bnxt_softc* softc,

if (prod >= cons)
        used = prod - cons;
else {
  • used = (txr->ring_size - cons) + prod + 1;

+ used = (txr->ring_size - cons) + prod;

}

return (txr->ring_size - used);
1013

bnxt_mpc_cmp() initializes 'tmp_raw_cons' with 'raw_cons' (passed from the caller of the function), then operates on 'tmp_raw_cons' and finally updates 'raw_cmp' with 'tmp_raw_cons' before return. So 'raw_cons' can be changed inside this function, so this write should be required. Am I missing something ?

1013

I think it's not really restore of raw_cons. raw_cons value can be changed in this function. In this function, tmp_raw_cons initializes with raw_cons and then we operate on tmp_raw_cons and finally update the raw_cons with tmp_raw_cons before returning from the function. Am I missing anything ?

sys/dev/bnxt/bnxt_en/bnxt_mpc.h
93

Yes, it exists. Will fix up.

94

Ack.

sys/dev/bnxt/bnxt_en/if_bnxt.c
466

Ack, I will fix up this as below:

Thor: BNXT_ROCE_IRQ_COUNT
Thor2: BNXT_ROCE_IRQ_COUNT + BNXT_MAX_MPC