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:

quoted

On Mon, Nov 29, 2021 at 09:37:24AM -0800, Jakub Kicinski wrote:

quoted

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.

quoted

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.