Message-ID: <20260112181436.4s5ceywwembn674r@skbuf>
Date: Mon, 12 Jan 2026 20:14:36 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Meghana Malladi <m-malladi@...com>
Cc: vadim.fedorenko@...ux.dev, horms@...nel.org, jacob.e.keller@...el.com,
	afd@...com, pmohan@...thit.com, basharath@...thit.com,
	rogerq@...nel.org, danishanwar@...com, pabeni@...hat.com,
	kuba@...nel.org, edumazet@...gle.com, davem@...emloft.net,
	andrew+netdev@...n.ch, linux-arm-kernel@...ts.infradead.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, srk@...com,
	Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [PATCH net-next 2/2] net: ti: icssg-prueth: Add ethtool ops for
 Frame Preemption MAC Merge

Hi Meghana,

On Wed, Jan 07, 2026 at 06:21:11PM +0530, Meghana Malladi wrote:
> Add ethtool ops .get_mm, .set_mm and .get_mm_stats to enable / disable
> Mac merge frame preemption and dump Preemption related statistics for
> ICSSG driver. Add pa stats registers for mac merge related statistics,
> which can be dumped using the ethtool ops.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@...com>
> Signed-off-by: Meghana Malladi <m-malladi@...com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 58 +++++++++++++++++++
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  2 +-
>  drivers/net/ethernet/ti/icssg/icssg_stats.h   |  5 ++
>  .../net/ethernet/ti/icssg/icssg_switch_map.h  |  5 ++
>  4 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index b715af21d23a..ceca6d6ec0f4 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> @@ -294,6 +294,61 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
>  	return 0;
>  }
>  
> +static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	void __iomem *config;
> +
> +	config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> +
> +	state->tx_enabled = iet->fpe_enabled;

I would expect state->tx_enabled to be returned from
iet->fpe_configured, aka from the same variable in which tx_enabled was
saved in emac_set_mm(). In case it's not clear, ethtool saves state in
the device driver and expects that state to be later returned in the
get() callback.

> +	state->pmac_enabled = true;
> +	state->verify_status = readb(config + PRE_EMPTION_VERIFY_STATUS);
> +	state->tx_min_frag_size = iet->tx_min_frag_size;

What is the range of acceptable values for PRE_EMPTION_ADD_FRAG_SIZE_LOCAL?
Is it safe to accept this value with no sanitization, just the ethtool
netlink policy which limits it between 60 and 252? Even if that is the
case, please add a comment specifying what the valid range is.

> +	state->rx_min_frag_size = 124;

Please add a comment justifying this non-standard value.

> +	state->tx_active = readb(config + PRE_EMPTION_ACTIVE_TX) ? true : false;
> +	state->verify_enabled = readb(config + PRE_EMPTION_ENABLE_VERIFY) ? true : false;
> +	state->verify_time = iet->verify_time_ms;

Why are some values returned from firmware and others from driver memory?

> +
> +	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
> +	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
> +	 */
> +	state->max_verify_time = 128;

ETHTOOL_MM_MAX_VERIFY_TIME_MS

> +
> +	return 0;
> +}
> +
> +static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> +		       struct netlink_ext_ack *extack)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +
> +	if (!cfg->pmac_enabled)
> +		netdev_err(ndev, "preemptible MAC is always enabled");

missing \n, OR use NL_SET_ERR_MSG_MOD(extack) which doesn't need \n.
Doing the latter is preferable, because the driver still accepts the
command while not modifying its internal state, and ethtool prints the
extack as warning if the return code was 0.
The catch is that openlldp sets pmac_enabled=false on exit, and that
would otherwise generate a noisy netdev_err() in your proposal (but
would be silent with the extack):
https://github.com/intel/openlldp/blob/master/lldp_8023.c#L443-L444

> +
> +	iet->verify_time_ms = cfg->verify_time;
> +	iet->tx_min_frag_size = cfg->tx_min_frag_size;
> +
> +	iet->fpe_configured = cfg->tx_enabled;
> +	iet->mac_verify_configured = cfg->verify_enabled;

Changes to the verification parameters should retrigger the verification
state machine, even if the link did not flap. Also, changes to the
ENABLED state should similarly be applied right away.

> +
> +	return 0;
> +}
> +
> +static void emac_get_mm_stats(struct net_device *ndev,
> +			      struct ethtool_mm_stats *s)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +
> +	s->MACMergeFrameAssOkCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_OK");
> +	s->MACMergeFrameAssErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_ERR");
> +	s->MACMergeFragCountRx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_RX");
> +	s->MACMergeFragCountTx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_TX");
> +	s->MACMergeFrameSmdErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_BAD_FRAG");
> +}
> +
>  const struct ethtool_ops icssg_ethtool_ops = {
>  	.get_drvinfo = emac_get_drvinfo,
>  	.get_msglevel = emac_get_msglevel,
> @@ -317,5 +372,8 @@ const struct ethtool_ops icssg_ethtool_ops = {
>  	.set_eee = emac_set_eee,
>  	.nway_reset = emac_nway_reset,
>  	.get_rmon_stats = emac_get_rmon_stats,
> +	.get_mm = emac_get_mm,
> +	.set_mm = emac_set_mm,
> +	.get_mm_stats = emac_get_mm_stats,
>  };
>  EXPORT_SYMBOL_GPL(icssg_ethtool_ops);
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index 7a586038adf8..9c31574cc7f6 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -58,7 +58,7 @@
>  
>  #define ICSSG_MAX_RFLOWS	8	/* per slice */
>  
> -#define ICSSG_NUM_PA_STATS	32
> +#define ICSSG_NUM_PA_STATS	37

Can't this be expressed as ARRAY_SIZE(icssg_all_pa_stats)? It is very
fragile to have to count and update this manually.

>  #define ICSSG_NUM_MIIG_STATS	60
>  /* Number of ICSSG related stats */
>  #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> index 5ec0b38e0c67..f35ae1b4f846 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> @@ -189,6 +189,11 @@ static const struct icssg_pa_stats icssg_all_pa_stats[] = {
>  	ICSSG_PA_STATS(FW_INF_DROP_PRIOTAGGED),
>  	ICSSG_PA_STATS(FW_INF_DROP_NOTAG),
>  	ICSSG_PA_STATS(FW_INF_DROP_NOTMEMBER),
> +	ICSSG_PA_STATS(FW_PREEMPT_BAD_FRAG),
> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_ERR),
> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_TX),
> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_OK),
> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_RX),
>  	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR),
>  	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF),
>  	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF),
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
> index 7e053b8af3ec..855fd4ed0b3f 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
> @@ -256,6 +256,11 @@
>  #define FW_INF_DROP_PRIOTAGGED		0x0148
>  #define FW_INF_DROP_NOTAG		0x0150
>  #define FW_INF_DROP_NOTMEMBER		0x0158
> +#define FW_PREEMPT_BAD_FRAG		0x0160
> +#define FW_PREEMPT_ASSEMBLY_ERR		0x0168
> +#define FW_PREEMPT_FRAG_CNT_TX		0x0170
> +#define FW_PREEMPT_ASSEMBLY_OK		0x0178
> +#define FW_PREEMPT_FRAG_CNT_RX		0x0180
>  #define FW_RX_EOF_SHORT_FRMERR		0x0188
>  #define FW_RX_B0_DROP_EARLY_EOF		0x0190
>  #define FW_TX_JUMBO_FRM_CUTOFF		0x0198
> -- 
> 2.43.0
>

Powered by blists - more mailing lists