| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <931bb43b-fbba-4423-9a20-122602b3b630@ti.com>
Date: Wed, 28 Jan 2026 18:32:05 +0530
From: "Malladi, Meghana" <m-malladi@...com>
To: Vladimir Oltean <vladimir.oltean@....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 Vladimir,
On 1/12/2026 11:44 PM, Vladimir Oltean wrote:
> 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.
Ok got it, will fix it in v2. I am aware that ethtool saves state in the
device driver which will be returned in the get() callback but didn't
know that should be the case every time.
>
>> + 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.
>
Sure, I will check and update in the comments.
>> + 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?
Sure, I will store all the values in the driver memory and return from
them. But should that be the case all the time ?
>
>> +
>> + /* 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
Ok, will update it.
>
>> +
>> + 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
>
Ok makes sense, thanks for pointing this out.
>> +
>> + 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.
.set_mm() will return -EBUSY if the interfaces are up. So changes in
verification parameters or ENABLED state can happen only when the
interfaces are down. And when the interface goes up the IET task get
scheduled re-triggering everything based on updated configurations.
>
>> +
>> + 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.
>
Yes I think its high time we did this. Will make this change in the next
version.
>> #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
>>
--
Thanks,
Meghana Malladi
Powered by blists - more mailing lists