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

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

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.