Re: [PATCH net-next v2 08/15] bnxt_en: Reserve crypto RX and TX key contexts on a PF

[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: Reserve crypto RX and TX key contexts on a PF

This patch teaches the bnxt_en PF reservation path to populate
num_ktls_tx_key_ctxs / num_ktls_rx_key_ctxs in the HWRM_FUNC_CFG request
and to record the reserved counts returned by HWRM_FUNC_QCFG, so kTLS
keys can later be allocated at run-time.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index e2b5f81b36a6..926665c8bb79 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

[ ... ]

> @@ -7915,8 +7920,9 @@ __bnxt_hwrm_reserve_pf_rings(struct bnxt *bp, struct bnxt_hw_rings *hwr)
>  		}
>  		req->num_stat_ctxs = cpu_to_le16(hwr->stat);
>  		req->num_vnics = cpu_to_le16(hwr->vnic);
> +		bnxt_hwrm_reserve_pf_key_ctxs(bp, req);
>  	}
> -	req->enables = cpu_to_le32(enables);
> +	req->enables |= cpu_to_le32(enables);
>  	return req;
>  }

[Medium]
After this change, every caller of __bnxt_hwrm_reserve_pf_rings() that
takes the BNXT_NEW_RM(bp) path picks up a kTLS key reservation.  One of
those callers is bnxt_hwrm_check_pf_rings(), which is the feasibility
check used by bnxt_check_rings() (ethtool -L, ethtool -G, channel
resize, etc.):

    static int bnxt_hwrm_check_pf_rings(struct bnxt *bp,
                                        struct bnxt_hw_rings *hwr)
    {
            struct hwrm_func_cfg_input *req;
            u32 flags;

            req = __bnxt_hwrm_reserve_pf_rings(bp, hwr);
            flags = FUNC_CFG_REQ_FLAGS_TX_ASSETS_TEST;
            if (BNXT_NEW_RM(bp)) {
                    flags |= FUNC_CFG_REQ_FLAGS_RX_ASSETS_TEST |
                             FUNC_CFG_REQ_FLAGS_CMPL_ASSETS_TEST |
                             FUNC_CFG_REQ_FLAGS_STAT_CTX_ASSETS_TEST |
                             FUNC_CFG_REQ_FLAGS_VNIC_ASSETS_TEST;
                    ...
            }

            req->flags = cpu_to_le32(flags);
            return hwrm_req_send_silent(bp, req);
    }

This sets the *_ASSETS_TEST bits in req->flags, but it never sets
FUNC_CFG_REQ_FLAGS2_KTLS_KEY_CTX_ASSETS_TEST in req->flags2 (a grep of
drivers/net/ethernet/broadcom/bnxt confirms that flag is not set
anywhere in the driver).

Does that mean firmware will treat the tx/rx/cmpl/stat/vnic resources as
test-only while still committing the kTLS key reservation as a side
effect of every bnxt_check_rings() call?

If so, would this also cause an ethtool ring/channel resize to be
rejected on a chip whose kTLS key pool is currently exhausted, even
though the user only intended to probe the ring resources?

Should bnxt_hwrm_check_pf_rings() either skip
bnxt_hwrm_reserve_pf_key_ctxs(), or set
FUNC_CFG_REQ_FLAGS2_KTLS_KEY_CTX_ASSETS_TEST in req->flags2 so the kTLS
portion of the request is also evaluated as test-only?

[Low]
On a kTLS-capable PF (bp->crypto_info != NULL),
bnxt_hwrm_reserve_pf_key_ctxs() unconditionally sets
FUNC_CFG_REQ_ENABLES_KTLS_TX_KEY_CTXS |
FUNC_CFG_REQ_ENABLES_KTLS_RX_KEY_CTXS in req->enables, and the change
from "=" to "|=" on the final assignment preserves them.

That makes the early-return short-circuit in bnxt_hwrm_reserve_pf_rings()
unreachable on such PFs:

    req = __bnxt_hwrm_reserve_pf_rings(bp, hwr);
    if (!req)
            return -ENOMEM;

    if (!req->enables) {
            hwrm_req_drop(bp, req);
            return 0;
    }

    rc = hwrm_req_send(bp, req);
    ...
    return bnxt_hwrm_get_rings(bp);

Is it intended that every previously-no-op call now performs an HWRM
round-trip plus a follow-up bnxt_hwrm_get_rings() on kTLS-capable PFs?

If not, would it be appropriate to also gate the kTLS enables/values on
whether any of the original "enables" bits were going to be set, so the
no-op path is preserved?

[ ... ]