On 02/09/24 6:32 pm, Andrew Lunn wrote:

quoted

Yes, and I have already added this in this series based on your feedback
on v2.

I have one question though, in emac_ndo_set_features() should I change
these HSR related features irrespective of the current mode?

AFAIK, if NETIF_F_HW_HSR_FWD is set, the forwarding is offloaded to HW.
If NETIF_F_HW_HSR_FWD is not set the forwarding is not offloaded to HW
and is done in SW.

So, I don't see any need to enable this features if we are currently in
switch mode. Let me know what do you think. Should I still enable this
feature irrespective of current mode and later handle this in
prueth_hsr_port_link / unlink()?

The user should not need to know about the different firmwares. So i
would allow NETIF_F_HW_HSR_FWD at any time.

The exception would be, if you look at all the other drivers which
implement HSR offload, if they all return an error if the offloading
cannot be enabled, then you should do the same.

Andrew, I looked at xrs700x dsa and microchip ksz dsa driver as an
example. The drivers return -EOPNOTSUPP whenever offloading is not
possible in the xrs700x_hsr_join / ksz_hsr_join api. I think the same
should be okay for ICSSG driver as well. ICSSG drivers equivalent API is
prueth_hsr_port_link() where I will return -EOPNOTSUPP whenever
offloading is not possible.

So in the .ndo_set_features() I will not add any check so that the user
wouldn't know about the current firmware and whether offloading is
supported or not. In the prueth_hsr_port_link() based on firmware
limitation we will error out.

Andrew, Roger, Let me know if this sounds good.

	Andrew

-- 
Thanks and Regards,
Danish