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

[Meghana Malladi]

Hi Vladimir,

On 1/28/26 18:44, Vladimir Oltean wrote:
> 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.

Ok got it. Will fix this in v2. Thanks.