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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Wed, Jan 28, 2026 at 06:32:05PM +0530, Malladi, Meghana wrote:
> > > 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.

Driver, hardware or firmware. But certainly the state returned in get()
must have some relationship with the state previously set in set().
iet->fpe_enabled has no relationship with state->tx_enabled; it
represents more or less the "tx_active" state.

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

Not necessarily, this implementation is just very inconsistent overall
and that raised the question whether there's any reason behind 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 it won't work with the openlldp stack?
tools/testing/selftests/drivers/net/hw/ethtool_mm.sh won't pass either?
(see h1_create() and h2_create())

I think adding such limitation will put the icssg in a world of its own,
technically implementing the same API as other network controllers but
practically not usable with the same scripts and tools.