Re: [PATCH net-next 3/3] nfp: devlink: allow flashing the device via devlink

[Michal Kubecek <mkubecek@suse.cz>]

On Wed, Feb 20, 2019 at 06:59:05PM -0800, Florian Fainelli wrote:
> On 2/19/2019 4:49 PM, Jakub Kicinski wrote:
> > On Tue, 19 Feb 2019 10:19:42 +0100, Jiri Pirko wrote:
> >> Fri, Feb 15, 2019 at 04:44:29PM CET, jakub.kicinski@netronome.com wrote:
> >>> On Fri, 15 Feb 2019 11:15:14 +0100, Jiri Pirko wrote:  
> >>>>> static const struct ethtool_ops nfp_net_ethtool_ops = {    
> >>>>
> >>>> Why don't you use the compat fallback? I think you should.  
> >>>
> >>> You and Michal both asked the same so let me answer the first to ask :)
> >>> - if devlink is built as a module the fallback is not reachable.  
> >>
> >> So the fallback is not really good as you can't use it for real drivers
> >> anyway. Odd. Maybe we should compile devlink in without possibility to
> >> have it as module.
> > 
> > Ack, I'll make devlink a bool.
> 
> Meh how about those poor and memory constrained embedded systems?
> Ideally ethtool should/could have been modular as well, but that ship
> has now sailed.

I would certainly like to make the ioctl interface optional once we
reach the end of "phase one", i.e. make ioctl-less ethtool possible.
Looking at the code, I don't see an obvious reason why it couldn't be
modular.

There seem to be only few functions in net/core/ethtool.c which are
called from outside and all seem to be simple helpers not really tied to
the rest of the code, except for netdev_rss_key variable (needed for
/proc/sys/net/core/netdev_rss_key). Some of them could even be inline.
We could always put these into some net/ethtool/stub.c (ethtool-stub.c)
and build unconditionally.

So if keeping the option to have devlink (and ethtool) as a module is
really desirable, I believe it can be done even now (unless I missed
something important).

> We have had similar issues with PHYLIB before where we wanted
> net/core/ethtool.c to be able to call into generic PHYLIB functions to
> obtain PHY statistics, an inline helper that de-references the PHY
> device's driver function pointers solved that (look for
> phy_ethtool_get_{strings,sset,stats}) while letting PHYLIB remain modular.

There is also something similar in netfilter - nf_ct_hook, nfnl_ct_hook
or nf_ipv6_ops.

> devlink_compat_flash_update() is a bit big to be inlined, but why not?

Most of it seems to be the lookup which I'm planning to factor out as
a separate helper. But that would also need to be available to external
code, of course.

Michal