Re: [PATCH net-next v2 10/15] bnxt_en: Add MPC transmit and completion functions

[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.
---
bnxt_en: Add MPC transmit and completion functions

This patch adds transmit, ring selection, and completion handling
helpers for the bnxt midpath rings (MPC), to be used for sending
control data to the crypto engines.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 4f5de41d4e86..a772b86842c1 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.c
> index a121bdcf9686..67ba43b7f168 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.c
> @@ -404,3 +404,141 @@ void bnxt_hwrm_mpc_ring_free(struct bnxt *bp, bool close_path)

[ ... ]

> +int bnxt_start_xmit_mpc(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
> +			void *data, unsigned int len, unsigned long handle)
> +{

[ ... ]

> +	memcpy(txbd, data, len);
> +	prod += bds;
> +	txr->tx_prod = prod;

[Low]
The rest of the bnxt tx fast paths consistently use WRITE_ONCE() when
updating txr->tx_prod and txr->tx_cons, since bnxt_tx_avail() reads both
with READ_ONCE():

    static inline u32 bnxt_tx_avail(struct bnxt *bp,
                                    const struct bnxt_tx_ring_info *txr)
    {
            u32 used = READ_ONCE(txr->tx_prod) - READ_ONCE(txr->tx_cons);
            ...
    }

The new MPC paths use plain stores for txr->tx_prod here in
bnxt_start_xmit_mpc() and for txr->tx_cons in bnxt_mpc_cmp() (below).

Since bnxt_mpc_cmp() runs in NAPI context without tx_lock, while the
producer side (in a follow-up patch) takes tx_lock around
bnxt_start_xmit_mpc(), the producer's READ_ONCE(tx_cons) inside
bnxt_tx_avail() is paired with the consumer's plain store, which would
be flagged by KCSAN.

Should these stores use WRITE_ONCE() to match the convention in the rest
of the driver?

> +
> +	/* Sync BD data before updating doorbell */
> +	wmb();
> +	bnxt_db_write(bp, &txr->tx_db, prod);
> +
> +	return 0;
> +}
> +
> +static bool bnxt_mpc_unsolicit(struct mpc_cmp *mpcmp)
> +{
> +	u32 client = MPC_CMP_CLIENT_TYPE(mpcmp);
> +
> +	if (client != MPC_CMP_CLIENT_TCE && client != MPC_CMP_CLIENT_RCE)
> +		return false;
> +	return MPC_CMP_UNSOLICIT_SUBTYPE(mpcmp);
> +}

[Medium]
The header in this same patch declares three subtypes:

    #define MPC_CMP_SUBTYPE_SOLICITED    (0x0 << 8)
    #define MPC_CMP_SUBTYPE_ERR          (0x1 << 8)
    #define MPC_CMP_SUBTYPE_RESYNC       (0x2 << 8)

but MPC_CMP_UNSOLICIT_SUBTYPE() only matches MPC_CMP_SUBTYPE_ERR:

    #define MPC_CMP_UNSOLICIT_SUBTYPE(mpcmp)                          \
            ((le32_to_cpu((mpcmp)->mpc_cmp_client_subtype_type) &     \
             MPC_CMP_SUBTYPE) == MPC_CMP_SUBTYPE_ERR)

Should a RESYNC completion (which by name is not a response to a host
TX) also be treated as unsolicited? As written, bnxt_mpc_unsolicit()
returns false on RESYNC and bnxt_mpc_cmp() then advances tx_cons and
clears mpc_buf->handle for a slot the host never submitted.

Would a check like != MPC_CMP_SUBTYPE_SOLICITED (or matching both ERR
and RESYNC) be more in line with the macro name and the defined
subtypes?

> +
> +int bnxt_mpc_cmp(struct bnxt *bp, struct bnxt_cp_ring_info *cpr, u32 *raw_cons)
> +{

[ ... ]

> +	if (!bnxt_mpc_unsolicit(mpcmp)) {
> +		struct bnxt_sw_mpc_tx_bd *mpc_buf;
> +		struct bnxt_tx_ring_info *txr;
> +		u16 tx_cons;
> +		u32 opaque;
> +
> +		opaque = mpcmp->mpc_cmp_opaque;
> +		txr = bnapi->tx_mpc_ring[client];
> +		tx_cons = txr->tx_cons;
> +		if (TX_OPAQUE_RING(opaque) != txr->tx_napi_idx)
> +			netdev_warn(bp->dev, "Wrong opaque %x, expected ring %x, idx %x\n",
> +				    opaque, txr->tx_napi_idx, txr->tx_cons);
> +		mpc_buf = &txr->tx_mpc_buf_ring[RING_TX(bp, tx_cons)];
> +		mpc_buf->handle = 0;
> +		tx_cons += mpc_buf->inline_bds;
> +		txr->tx_cons = tx_cons;
> +		txr->tx_hw_cons = RING_TX(bp, tx_cons);
> +	}

[Medium]
When the opaque ring index returned by the device does not match the
locally-resolved txr (TX_OPAQUE_RING(opaque) != txr->tx_napi_idx),
control just falls through after netdev_warn() and the code still
dereferences txr->tx_mpc_buf_ring[RING_TX(bp, tx_cons)], clears
mpc_buf->handle, and advances txr->tx_cons by mpc_buf->inline_bds.

If this check ever fires (HW bug, memory corruption, compromised
firmware), would the driver be corrupting its own ring bookkeeping for
the locally-resolved txr — clearing a handle for a buffer that was not
actually completed and skipping over slots whose handles are then never
released?

Given that handle holds a kernel pointer/cookie used by the crypto
path, should this branch return early (or otherwise avoid touching
tx_cons / tx_mpc_buf_ring) instead of continuing as if the completion
were valid?

[Low]
Same WRITE_ONCE() concern as in bnxt_start_xmit_mpc() — txr->tx_cons is
written here with a plain store while producers read it via
READ_ONCE(txr->tx_cons) in bnxt_tx_avail(). Should this be
WRITE_ONCE(txr->tx_cons, tx_cons) for consistency with the rest of the
driver?

> +
> +cmp_done:
> +	*raw_cons = tmp_raw_cons;
> +	return 0;
> +}
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.h
> index cdc03a074963..9958d1749ffb 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_mpc.h
> @@ -30,11 +30,53 @@ struct bnxt_mpc_info {

[ ... ]

> +struct mpc_cmp {
> +	__le32 mpc_cmp_client_subtype_type;
> +	#define MPC_CMP_TYPE					(0x3f << 0)
> +	 #define MPC_CMP_TYPE_MID_PATH_SHORT			 0x1e
> +	 #define MPC_CMP_TYPE_MID_PATH_LONG			 0x1f
> +	#define MPC_CMP_SUBTYPE					0xf00
> +	#define MPC_CMP_SUBTYPE_SFT				 8
> +	 #define MPC_CMP_SUBTYPE_SOLICITED			 (0x0 << 8)
> +	 #define MPC_CMP_SUBTYPE_ERR				 (0x1 << 8)
> +	 #define MPC_CMP_SUBTYPE_RESYNC				 (0x2 << 8)

[ ... ]

> +#define MPC_CMP_UNSOLICIT_SUBTYPE(mpcmp)				\
> +	((le32_to_cpu((mpcmp)->mpc_cmp_client_subtype_type) &		\
> +	 MPC_CMP_SUBTYPE) == MPC_CMP_SUBTYPE_ERR)

[ ... ]