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

[sumit.saxena_broadcom.com]

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.

[gallatin]

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

296

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

648

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

816

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.

819

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) ?

1014

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

94

Doesn't this already exist in bnxt.h?

95

Space around the &

sys/dev/bnxt/bnxt_en/if_bnxt.c

451–453

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.

682

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

[sumit.saxena_broadcom.com]

sys/dev/bnxt/bnxt_en/bnxt_mpc.c

296

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'.

648

Ack, will fix up this.

816

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);

1014

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 ?

1014

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

94

Yes, it exists. Will fix up.

95

Ack.

sys/dev/bnxt/bnxt_en/if_bnxt.c

451–453

Ack, I will fix up this as below:

Thor: BNXT_ROCE_IRQ_COUNT
Thor2: BNXT_ROCE_IRQ_COUNT + BNXT_MAX_MPC

[sumit.saxena_broadcom.com]

Handled the review bugs/comments.

[gallatin]

You never seem to use txr->tx_lock. Eg, you init it, and destroy it, but its not used to serialize access to the txr. And you seem to update txr->prod without any protection. Seems like you need to take the lock in bnxt_start_xmit_mpc, or get rid of it and assert whatever mutex protects things.

You also seem to access the consumer with atomics somtimes, and without others. What protects that?

sys/dev/bnxt/bnxt_en/bnxt_mpc.c

375–380

I think you need to check for null ring mem, which can happen during partial alloc failures. Eg, make sure you don't pass null to iflib_dma_free. A zeroed ring_mem is fine, but a null ring_mem is not.

667

I think you need to check for an EBUSY here.

674

maybe limit this to a few prints, or panic? unlimited console prints can wedge a machine sometimes

sys/dev/bnxt/bnxt_en/bnxt_mpc.h

36

Shouldn't this be BNXT_MPC_RE_CFA_TYPE? Else the mpc_rings (declared below) could be written to out of bounds when used for something other than crypto. I don't think you're doing this (yet) but it could be problem later.

sys/dev/bnxt/bnxt_en/if_bnxt.c

3049

Where are partially allocated resources cleaned up?