Re: [PATCH 35/46] common/sfc_efx/base: support MAC statistics on Medford4 NICs

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 4/16/25 17:00, Ivan Malov wrote:
> Supply Medford4-specific methods to clear, upload and update
> MAC statistics, as well as the method to toggle periodic DMA
> updates. All of these leverage the same netport MCDI command.
> 
> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am>
> Reviewed-by: Andy Moreton <andy.moreton@amd.com>
> Reviewed-by: Pieter Jansen Van Vuuren <pieter.jansen-van-vuuren@amd.com>

[snip]

> diff --git a/drivers/common/sfc_efx/base/ef10_nic.c b/drivers/common/sfc_efx/base/ef10_nic.c
> index eb1b68b17e..31fcb361f2 100644
> --- a/drivers/common/sfc_efx/base/ef10_nic.c
> +++ b/drivers/common/sfc_efx/base/ef10_nic.c
> @@ -2523,7 +2523,9 @@ ef10_nic_probe(
>   
>   #if EFSYS_OPT_MAC_STATS
>   	/* Wipe the MAC statistics */
> -	if ((rc = efx_mcdi_mac_stats_clear(enp)) != 0)
> +
> +	rc = efx_mcdi_mac_stats_clear(enp);
> +	if (rc != 0)

it looks like unrelated style change.

>   		goto fail6;
>   #endif
>   

[snip]

> diff --git a/drivers/common/sfc_efx/base/efx_np.c b/drivers/common/sfc_efx/base/efx_np.c
> index d4ee17ffb4..df836f09a6 100644
> --- a/drivers/common/sfc_efx/base/efx_np.c
> +++ b/drivers/common/sfc_efx/base/efx_np.c
> @@ -1291,3 +1291,92 @@ efx_np_mac_ctrl(
>   	EFSYS_PROBE1(fail1, efx_rc_t, rc);
>   	return (rc);
>   }
> +
> +#if EFSYS_OPT_MAC_STATS
> +	__checkReturn	efx_rc_t
> +efx_np_mac_stats(
> +	__in		efx_nic_t *enp,
> +	__in		efx_np_handle_t nph,
> +	__in		efx_stats_action_t action,
> +	__in_opt	const efsys_mem_t *esmp,
> +	__in		uint16_t period_ms)
> +{
> +	EFX_MCDI_DECLARE_BUF(payload,
> +	    MC_CMD_GET_NETPORT_STATISTICS_IN_LEN,
> +	    MC_CMD_GET_NETPORT_STATISTICS_OUT_LENMIN);
> +	int enable = (action == EFX_STATS_ENABLE_NOEVENTS);
> +	int events = (action == EFX_STATS_ENABLE_EVENTS);
> +	int disable = (action == EFX_STATS_DISABLE);
> +	int upload = (action == EFX_STATS_UPLOAD);
> +	int clear = (action == EFX_STATS_CLEAR);

IMHO boolean_t should be used for 5 above variables.

> +	efx_mcdi_req_t req;
> +	efx_rc_t rc;

[snip]

> +
> +		/* TODO: validate encp->enc_mac_stats_nstats */

TODO again. The new code is full of TODO and FIXME. It looks like it has
huge backlog. What's the problem to fix the TODO? Is the driver really
ready?

> +		sz = encp->enc_mac_stats_nstats * sizeof (efx_qword_t);
> +
[snip]

> +	__checkReturn		efx_rc_t
> +medford4_mac_stats_periodic(
> +	__in			efx_nic_t *enp,
> +	__in			efsys_mem_t *esmp,
> +	__in			uint16_t period_ms,
> +	__in			boolean_t events)
> +{
> +	efx_port_t *epp = &(enp->en_port);
> +	efx_rc_t rc;
> +
> +	if (period_ms == 0) {
> +		rc = efx_np_mac_stats(enp, epp->ep_np_handle,
> +			    EFX_STATS_DISABLE, NULL, 0);
> +	} else if (events != B_FALSE) {
> +		rc = efx_np_mac_stats(enp, epp->ep_np_handle,
> +			    EFX_STATS_ENABLE_EVENTS, esmp, period_ms);
> +	} else {
> +		rc = efx_np_mac_stats(enp, epp->ep_np_handle,
> +			    EFX_STATS_ENABLE_NOEVENTS, esmp, period_ms);
> +	}
> +
> +	if (rc != 0)
> +		goto fail1;
> +
> +	return (0);
> +
> +fail1:
> +	EFSYS_PROBE1(fail1, efx_rc_t, rc);
> +	return (rc);
> +}
> +
> +#define	MEDFORD4_MAC_STAT_READ(_esmp, _field, _eqp)			\
> +	EFSYS_MEM_READQ((_esmp), (_field) * sizeof (efx_qword_t), _eqp)
> +
> +	__checkReturn			efx_rc_t
> +medford4_mac_stats_update(
> +	__in				efx_nic_t *enp,
> +	__in				efsys_mem_t *esmp,
> +	__inout_ecount(EFX_MAC_NSTATS)	efsys_stat_t *stats,
> +	__inout_opt			uint32_t *generationp)
> +{
> +	const efx_nic_cfg_t *encp = &enp->en_nic_cfg;
> +	efx_port_t *epp = &(enp->en_port);
> +	efx_qword_t generation_start;
> +	efx_qword_t generation_end;
> +	unsigned int i;
> +	efx_rc_t rc;
> +
> +	/* TODO: validate encp->enc_mac_stats_nstats */
> +	if (EFSYS_MEM_SIZE(esmp) <
> +	    (encp->enc_mac_stats_nstats * sizeof (efx_qword_t))) {
> +		/* DMA buffer too small */
> +		rc = ENOSPC;
> +		goto fail1;
> +	}
> +
> +	/* Read END first so we don't race with the MC */
> +	EFSYS_DMA_SYNC_FOR_KERNEL(esmp, 0, EFSYS_MEM_SIZE(esmp));
> +	MEDFORD4_MAC_STAT_READ(esmp, (encp->enc_mac_stats_nstats - 1),
> +	    &generation_end);
> +	EFSYS_MEM_READ_BARRIER();
> +
> +	for (i = 0; i < EFX_ARRAY_SIZE(epp->ep_np_mac_stat_lut); ++i) {
> +		efx_qword_t value;
> +
> +		if (epp->ep_np_mac_stat_lut[i].ens_valid == B_FALSE)
> +			continue;
> +
> +		MEDFORD4_MAC_STAT_READ(esmp,
> +		    epp->ep_np_mac_stat_lut[i].ens_dma_fld, &value);
> +
> +		EFSYS_STAT_SET_QWORD(&(stats[i]), &value);
> +	}
> +
> +	/* TODO: care about VADAPTOR statistics */

TODO again

> +
> +	/* Read START generation counter */
> +	EFSYS_DMA_SYNC_FOR_KERNEL(esmp, 0, EFSYS_MEM_SIZE(esmp));
> +	EFSYS_MEM_READ_BARRIER();
> +
> +	/* FIXME: we never parse marker descriptors; assume start is 0 offset */

FIXME again

> +	MEDFORD4_MAC_STAT_READ(esmp, 0, &generation_start);
> +
> +	/* Check that we didn't read the stats in the middle of a DMA */
> +	if (memcmp(&generation_start, &generation_end,
> +		    sizeof (generation_start)) != 0)
> +		return (EAGAIN);
> +
> +	if (generationp != NULL)
> +		*generationp = EFX_QWORD_FIELD(generation_start, EFX_DWORD_0);
> +
> +	return (0);
> +
> +fail1:
> +	EFSYS_PROBE1(fail1, efx_rc_t, rc);
> +	return (rc);
> +}
> +
> +#undef MEDFORD4_MAC_STAT_READ
>   #endif /* EFSYS_OPT_MAC_STATS */
>   #endif /* EFSYS_OPT_MEDFORD4 */