From: Maxime Chevallier <maxime.chevallier@bootlin•com>
To: Meghana Malladi <m-malladi@ti•com>,
liuhangbin@gmail•com, h-mittal1@ti•com, haokexin@gmail•com,
vadim.fedorenko@linux•dev, devnexen@gmail•com, horms@kernel•org,
jacob.e.keller@intel•com, arnd@arndb•de, afd@ti•com,
basharath@couthit•com, parvathi@couthit•com,
vladimir.oltean@nxp•com, rogerq@kernel•org, danishanwar@ti•com,
pabeni@redhat•com, kuba@kernel•org, edumazet@google•com,
davem@davemloft•net, andrew+netdev@lunn•ch
Cc: linux-arm-kernel@lists•infradead.org, netdev@vger•kernel.org,
linux-kernel@vger•kernel.org, srk@ti•com,
Vignesh Raghavendra <vigneshr@ti•com>
Subject: Re: [PATCH net-next v6 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge
Date: Mon, 25 May 2026 21:35:41 +0200 [thread overview]
Message-ID: <7f194a90-8bf6-4217-b5fa-c003b3dc85b3@bootlin.com> (raw)
In-Reply-To: <20260525182700.3135858-3-m-malladi@ti.com>
Hi,
On 5/25/26 20:27, Meghana Malladi wrote:
> From: MD Danish Anwar <danishanwar@ti•com>
>
> Add driver support for viewing and changing the MAC Merge sublayer
> parameters via ethtool ops: .set_mm(), .get_mm() and .get_mm_stats().
>
> The minimum size of non-final mPacket fragments supported by the firmware
> without leading errors is 64 Bytes (including FCS). Verify time bounded to
> 1-128 ms per 802.3-2018 clause 30.14.1.6. Add a check to ensure
> user passed tx_min_frag_size argument via ethtool, honors this.
> Add pa stats registers to check statistics for preemption, which can be
> dumped using ethtool ops.
>
> Fix emac_get_stat_by_name() to return u64 instead of int and return 0 on
> error instead of -EINVAL. This prevents invalid stat lookups from corrupting
> output stats with signed error codes cast to u64. Error conditions are still
> logged via netdev_err().
>
> Signed-off-by: MD Danish Anwar <danishanwar@ti•com>
> Signed-off-by: Meghana Malladi <m-malladi@ti•com>
> ---
>
> v6-v5:
> - Initialize tx_min_frag_size and verify_time with valid values
> to avoid returning corrupted values during get_mm()
> - Fix rx_min_frag_size to ETH_ZLEN excluding ETH_FCS_LEN
> - Fix return codes for emac_set_mm()
> All the above changes address the comments raised by sashiko
>
> drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 106 ++++++++++++++++++
> drivers/net/ethernet/ti/icssg/icssg_prueth.h | 7 +-
> drivers/net/ethernet/ti/icssg/icssg_qos.h | 46 ++++++++
> drivers/net/ethernet/ti/icssg/icssg_stats.c | 4 +-
> drivers/net/ethernet/ti/icssg/icssg_stats.h | 7 +-
> .../net/ethernet/ti/icssg/icssg_switch_map.h | 5 +
> 6 files changed, 168 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index b715af21d23ac..ee940051644d6 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> @@ -294,6 +294,109 @@ 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;
> + enum icssg_ietfpe_verify_states verify_status;
> +
> + if (emac->is_sr1)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&iet->fpe_lock);
> + state->tx_enabled = iet->fpe_enabled;
> + state->tx_min_frag_size = iet->tx_min_frag_size;
> + state->tx_active = iet->fpe_active;
> + state->verify_enabled = iet->mac_verify_configure;
> + state->verify_time = iet->verify_time_ms;
> + verify_status = iet->verify_status;
> + mutex_unlock(&iet->fpe_lock);
> +
> + state->rx_min_frag_size = ETH_ZLEN;
> + state->pmac_enabled = true;
> +
> + switch (verify_status) {
> + case ICSSG_IETFPE_STATE_DISABLED:
> + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> + break;
> + case ICSSG_IETFPE_STATE_INITIAL:
> + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;
> + break;
> + case ICSSG_IETFPE_STATE_VERIFYING:
> + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING;
> + break;
> + case ICSSG_IETFPE_STATE_SUCCEEDED:
> + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
> + break;
> + case ICSSG_IETFPE_STATE_FAILED:
> + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> + break;
> + default:
> + state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
> + break;
> + }
> +
> + /* 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 = 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;
> + int err;
> +
> + if (emac->is_sr1)
> + return -EOPNOTSUPP;
> +
> + if (!cfg->pmac_enabled) {
> + NL_SET_ERR_MSG_MOD(extack, "preemptible MAC is always enabled");
> + return -EOPNOTSUPP;
> + }
> +
> + err = icssg_qos_validate_tx_min_frag_size(cfg->tx_min_frag_size, extack);
> + if (err)
> + return err;
> +
> + err = icssg_qos_validate_verify_time(cfg->verify_time, extack);
> + if (err)
> + return err;
The ethnl code already validates the verify_time :
https://elixir.bootlin.com/linux/v7.0.9/source/net/ethtool/mm.c#L153
> +
> + mutex_lock(&iet->fpe_lock);
> + iet->verify_time_ms = cfg->verify_time;
> + iet->tx_min_frag_size = cfg->tx_min_frag_size;
> + iet->fpe_enabled = cfg->tx_enabled;
> + iet->mac_verify_configure = cfg->verify_enabled;
> + err = icssg_config_ietfpe(ndev, cfg->tx_enabled);
> + mutex_unlock(&iet->fpe_lock);
> +
> + return err;
> +}
> +
> +static void emac_get_mm_stats(struct net_device *ndev,
> + struct ethtool_mm_stats *s)
> +{
> + struct prueth_emac *emac = netdev_priv(ndev);
> +
> + if (emac->is_sr1)
> + return;
> +
> + if (!emac->prueth->pa_stats)
> + return;
> +
> + /* MACMergeHoldCount stats is not tracked by the firmware */
> + 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 +420,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 85f7017d2c8e7..61320c252bec2 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -45,6 +45,7 @@
> #include "icss_iep.h"
> #include "icssg_switch_map.h"
> #include "icssg_qos.h"
> +#include "icssg_stats.h"
>
> #define PRUETH_MAX_MTU (2000 - ETH_HLEN - ETH_FCS_LEN)
> #define PRUETH_MIN_PKT_SIZE (VLAN_ETH_ZLEN)
> @@ -58,8 +59,8 @@
>
> #define ICSSG_MAX_RFLOWS 8 /* per slice */
>
> -#define ICSSG_NUM_PA_STATS 32
> -#define ICSSG_NUM_MIIG_STATS 60
> +#define ICSSG_NUM_PA_STATS ARRAY_SIZE(icssg_all_pa_stats)
> +#define ICSSG_NUM_MIIG_STATS ARRAY_SIZE(icssg_all_miig_stats)
> /* Number of ICSSG related stats */
> #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
> #define ICSSG_NUM_STANDARD_STATS 31
> @@ -460,7 +461,7 @@ int emac_fdb_flow_id_updated(struct prueth_emac *emac);
>
> void icssg_stats_work_handler(struct work_struct *work);
> void emac_update_hardware_stats(struct prueth_emac *emac);
> -int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name);
> +u64 emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name);
>
> /* Common functions */
> void prueth_cleanup_rx_chns(struct prueth_emac *emac,
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.h b/drivers/net/ethernet/ti/icssg/icssg_qos.h
> index 9355e96bbcda8..87ca031afcaa4 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_qos.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.h
> @@ -13,6 +13,7 @@
> #define ICSSG_EXPRESS_Q_MASK_ALL 0xFF
> #define ICSSG_IET_MAX_VERIFY_TIME 128
> #define ICSSG_IET_MIN_VERIFY_TIME 1
> +#define ICSSG_IET_MAX_TX_MIN_FRAG_SIZE 252
>
> /**
> * enum icssg_ietfpe_verify_states - status of MAC Merge Verify returned by firmware
> @@ -63,4 +64,49 @@ void icssg_qos_link_state_update(struct net_device *ndev);
> int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> void *type_data);
> int icssg_config_ietfpe(struct net_device *ndev, bool enable);
> +static inline int icssg_qos_validate_tx_min_frag_size(u32 min_frag_size,
> + struct netlink_ext_ack *extack)
> +{
> + /* Firmware takes min_frag_size including FCS length.
> + * The firmware requires the fragment size (including FCS) to be
> + * a multiple of 64 bytes. Since 64 bytes = ETH_ZLEN + ETH_FCS_LEN,
> + * valid user-facing values are: 60, 124, 188, 252.
> + */
> +
> + if (min_frag_size < ETH_ZLEN) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "tx_min_frag_size must be at least 60 bytes");
> + return -EINVAL;
> + }
> +
> + if (min_frag_size > ICSSG_IET_MAX_TX_MIN_FRAG_SIZE) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "tx_min_frag_size must not exceed 252 bytes");
> + return -EINVAL;
> + }
> +
> + if ((min_frag_size + ETH_FCS_LEN) % (ETH_ZLEN + ETH_FCS_LEN)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "tx_min_frag_size must be a multiple of 64 bytes minus 4");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Is there any reason this helper is in a header file instead
of being directly in icssg_ethtool.c ?
> +
> +static inline int icssg_qos_validate_verify_time(u32 verify_time_ms,
> + struct netlink_ext_ack *extack)
> +{
> + /* 802.3-2018 clause 30.14.1.6: aMACMergeVerifyTime must be
> + * between 1 and 128 ms inclusive
> + */
> + if (verify_time_ms < ICSSG_IET_MIN_VERIFY_TIME ||
> + verify_time_ms > ICSSG_IET_MAX_VERIFY_TIME) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "verify_time must be between 1 and 128 ms");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
And this shouldn't be needed as the ethnl mm code already validates
these boundaries, as they are straight from the standard.
> #endif /* __NET_TI_ICSSG_QOS_H */
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.c b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> index 7159baa0155cf..cfdb6f5dc5da1 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.c
> @@ -74,7 +74,7 @@ void icssg_stats_work_handler(struct work_struct *work)
> }
> EXPORT_SYMBOL_GPL(icssg_stats_work_handler);
>
> -int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
> +u64 emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
> {
> int i;
>
> @@ -91,5 +91,5 @@ int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name)
> }
>
> netdev_err(emac->ndev, "Invalid stats %s\n", stat_name);
> - return -EINVAL;
> + return 0;
> }
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> index 5ec0b38e0c67d..8073deac35c3e 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> @@ -8,8 +8,6 @@
> #ifndef __NET_TI_ICSSG_STATS_H
> #define __NET_TI_ICSSG_STATS_H
>
> -#include "icssg_prueth.h"
> -
> #define STATS_TIME_LIMIT_1G_MS 25000 /* 25 seconds @ 1G */
>
> struct miig_stats_regs {
> @@ -189,6 +187,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 7e053b8af3ece..855fd4ed0b3f6 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
Maxime
next prev parent reply other threads:[~2026-05-25 19:36 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-25 18:26 [PATCH net-next v6 0/2] Add Frame Preemption MAC Merge support for ICSSG Meghana Malladi
2026-05-25 18:26 ` [PATCH net-next v6 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support Meghana Malladi
2026-05-25 19:18 ` Maxime Chevallier
2026-05-25 19:46 ` Andrew Lunn
2026-06-04 13:37 ` Meghana Malladi
2026-06-04 13:37 ` Meghana Malladi
2026-05-25 18:27 ` [PATCH net-next v6 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge Meghana Malladi
2026-05-25 19:35 ` Maxime Chevallier [this message]
2026-06-04 13:45 ` Meghana Malladi
2026-05-27 11:57 ` [PATCH net-next v6 0/2] Add Frame Preemption MAC Merge support for ICSSG Simon Horman
2026-06-04 13:39 ` Meghana Malladi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7f194a90-8bf6-4217-b5fa-c003b3dc85b3@bootlin.com \
--to=maxime.chevallier@bootlin$(echo .)com \
--cc=afd@ti$(echo .)com \
--cc=andrew+netdev@lunn$(echo .)ch \
--cc=arnd@arndb$(echo .)de \
--cc=basharath@couthit$(echo .)com \
--cc=danishanwar@ti$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=devnexen@gmail$(echo .)com \
--cc=edumazet@google$(echo .)com \
--cc=h-mittal1@ti$(echo .)com \
--cc=haokexin@gmail$(echo .)com \
--cc=horms@kernel$(echo .)org \
--cc=jacob.e.keller@intel$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=liuhangbin@gmail$(echo .)com \
--cc=m-malladi@ti$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=parvathi@couthit$(echo .)com \
--cc=rogerq@kernel$(echo .)org \
--cc=srk@ti$(echo .)com \
--cc=vadim.fedorenko@linux$(echo .)dev \
--cc=vigneshr@ti$(echo .)com \
--cc=vladimir.oltean@nxp$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox