Re: [RFC PATCH net-next 0/4] ethtool: Add ability to flash and query transceiver modules' firmware

[Ido Schimmel <idosch@idosch.org>]

On Tue, Nov 30, 2021 at 09:54:26AM +0100, Michal Kubecek wrote:
> On Tue, Nov 30, 2021 at 10:36:02AM +0200, Ido Schimmel wrote:
> > On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:
> > > 
> > > Immediate question I have is why not devlink. We purposefully moved 
> > > FW flashing to devlink because I may take long, so doing it under
> > > rtnl_lock is really bad. Other advantages exist (like flashing
> > > non-Ethernet ports). Ethtool netlink already existed at the time.
> > 
> > Device firmware flashing doesn't belong in devlink because of locking
> > semantics. It belongs in devlink because you are updating the firmware
> > of the device, which can instantiate multiple netdevs. For multi-port
> > devices, it always seemed weird to tell users "choose some random port
> > and use it for 'ethtool -f'". I remember being asked if the command
> > needs to be run for all swp* netdevs (no).
> > 
> > On the other hand, each netdev corresponds to a single transceiver
> > module and each transceiver module corresponds to a single netdev
> > (modulo split which is a user configuration).
> 
> Devlink also has abstraction for ports so it can be used so it is not
> necessarily a problem.
> 
> > In addition, users are already dumping the EEPROM contents of
> > transceiver modules via ethtool and also toggling their settings.
> > 
> > Given the above, it's beyond me why we should tell users to use anything
> > other than ethtool to update transceiver modules' firmware.
> 
> As I already mentioned, we should distinguish between ethtool API and
> ethtool utility. It is possible to implement the flashing in devlink API
> and let both devlink and ethtool utilities use that API.
> 
> I'm not saying ethtool API is a wrong choice, IMHO either option has its
> pros and cons.

What are the cons of implementing it in ethtool? It seems that the only
thing devlink has going for it is the fact that it supports devlink
device firmware update API, but it cannot be used as-is and needs to be
heavily extended (e.g., asynchronicity is a must, per-port as opposed to
per-device). It doesn't support any transceiver module API, as opposed
to ethtool.

> I'm just trying to point out that implementation in devlink API does
> not necessarily mean one cannot use the ethtool to use the feature.

I agree it can be done, but the fact that something can be done doesn't
mean it should be done. If I'm extending devlink with new uAPI, then I
will add support for it in devlink(8) and not ethtool(8) and vice versa.