Re: [PATCH net-next v6 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Meghana Malladi <m-malladi@ti.com>]

On 5/26/26 01:05, Maxime Chevallier wrote:
> 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
> 

I remember getting a comment asking me to add a comment or check to 
validate tx_min_frag_size, I don't remember at this point why I added 
for verify_time as well, lost track in the comments lot. I will remove 
it. Just to confirm, do I need icssg_qos_validate_tx_min_frag_size() or 
can I remove it as well ?

>> +
>> +    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 ?
> 

I don't have any strong reason as such, I didn't want to put any helper 
functions 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.
> 

Will remove this is my next version.

>>   #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

Thanks,
Meghana