Re: [PATCH net-next v2 07/15] bnxt_en: Allocate crypto structure and backing store

[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: Allocate crypto structure and backing store

If the chip and firmware support crypto (TLS) offload, allocate a
bp->crypto_info software structure and backing store to support the RX
and TX contexts.  Each offloaded TLS connection requires a backing store
context for each direction.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_crypto.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_crypto.c
> new file mode 100644
> index 000000000000..a5fee08eaa67
> --- /dev/null
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_crypto.c

[ ... ]

> +static u32 bnxt_get_max_crypto_key_ctx(struct bnxt *bp, int key_type)
> +{
> +	u32 fw_maj = BNXT_FW_MAJ(bp);
> +
> +	if (key_type == BNXT_TX_CRYPTO_KEY_TYPE)
> +		return (fw_maj < 233) ? BNXT_MAX_TX_CRYPTO_KEYS_PRE_233FW :
> +		       BNXT_MAX_TX_CRYPTO_KEYS;
> +
> +	return (fw_maj < 233) ? BNXT_MAX_RX_CRYPTO_KEYS_PRE_233FW :
> +	       BNXT_MAX_RX_CRYPTO_KEYS;
> +}

[ ... ]

> +void bnxt_alloc_crypto_info(struct bnxt *bp,
> +			    struct hwrm_func_qcaps_output *resp)
> +{
> +	u16 max_keys = le16_to_cpu(resp->max_key_ctxs_alloc);
> +	struct bnxt_crypto_info *crypto = bp->crypto_info;
> +
> +	if (BNXT_VF(bp))
> +		return;
> +	if (!crypto) {
> +		struct bnxt_kctx *kctx;
> +		int i;
> +
> +		crypto = kzalloc_obj(*crypto);
> +		if (!crypto) {
> +			netdev_warn(bp->dev,
> +				    "Unable to allocate crypto info\n");
> +			return;
> +		}
> +		for (i = 0; i < BNXT_MAX_CRYPTO_KEY_TYPE; i++) {
> +			kctx = &crypto->kctx[i];
> +			kctx->type = i;
> +			kctx->max_ctx = bnxt_get_max_crypto_key_ctx(bp, i);
> +		}
> +		bp->crypto_info = crypto;
> +	}
> +	crypto->max_key_ctxs_alloc = max_keys;
> +}

[Medium]
Should the kctx[].max_ctx values be refreshed on every call, rather than
only when bp->crypto_info is first allocated?

bnxt_alloc_crypto_info() can be invoked multiple times for a given bp.
__bnxt_hwrm_func_qcaps() is the caller, and it runs from paths such as
bnxt_resume(), bnxt_restore_pf_fw_resources(), bnxt_io_resume(), and
during SR-IOV enable, in addition to initial probe.

On the second and later calls, the crypto pointer is non-NULL, so the
loop that calls bnxt_get_max_crypto_key_ctx(bp, i) is skipped and only
crypto->max_key_ctxs_alloc is refreshed.

If BNXT_FW_MAJ(bp) crosses the 233 boundary between two qcaps queries on
the same driver instance (for example after a firmware update applied
across suspend/resume, or after a firmware reset that re-reads
fw_ver_code), the cached kctx[].max_ctx retains the pre-233 value of
65535 instead of being recomputed to 204800.

bnxt_backing_store_cfg_v2() then sizes the TCK/RCK backing stores from
those cached values:

	if (crypto) {
		ctxm = &ctx->ctx_arr[BNXT_CTX_TCK];
		rc = bnxt_setup_ctxm_pg_tbls(bp, ctxm,
					     BNXT_TCK(crypto).max_ctx, 1);
		...
		ctxm = &ctx->ctx_arr[BNXT_CTX_RCK];
		rc = bnxt_setup_ctxm_pg_tbls(bp, ctxm,
					     BNXT_RCK(crypto).max_ctx, 1);

and bnxt_setup_ctxm_pg_tbls() applies clamp_t(u32, entries,
ctxm->min_entries, ctxm->max_entries), so the stale 65535 caps the
allocation even if firmware now advertises a larger ctxm->max_entries.

Would moving the kctx[] initialization out of the if (!crypto) block, or
running it unconditionally, be a better fit for the function name and
kerneldoc which describe it as 'Allocate and initialize crypto offload
context' without noting the once-only initialization?