Re: [PATCH net-next 4/5] nfp: add .ndo_get_devlink

[Michal Kubecek <mkubecek@suse.cz>]

On Fri, Feb 22, 2019 at 09:02:04AM -0800, Jakub Kicinski wrote:
> On Fri, 22 Feb 2019 11:04:50 +0100, Michal Kubecek wrote:
> > On Thu, Feb 21, 2019 at 09:46:19AM -0800, Jakub Kicinski wrote:
> > > Support getting devlink instance from a new NDO.
> > > 
> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > ---
> > >  drivers/net/ethernet/netronome/nfp/nfp_app.h        |  2 ++
> > >  drivers/net/ethernet/netronome/nfp/nfp_devlink.c    | 11 +++++++++++
> > >  drivers/net/ethernet/netronome/nfp/nfp_net_common.c |  1 +
> > >  drivers/net/ethernet/netronome/nfp/nfp_net_repr.c   |  1 +
> > >  4 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app.h b/drivers/net/ethernet/netronome/nfp/nfp_app.h
> > > index d578d856a009..f8d422713705 100644
> > > --- a/drivers/net/ethernet/netronome/nfp/nfp_app.h
> > > +++ b/drivers/net/ethernet/netronome/nfp/nfp_app.h
> > > @@ -433,4 +433,6 @@ int nfp_app_nic_vnic_alloc(struct nfp_app *app, struct nfp_net *nn,
> > >  int nfp_app_nic_vnic_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
> > >  				   struct nfp_net *nn, unsigned int id);
> > >  
> > > +struct devlink *nfp_devlink_get_devlink(struct net_device *netdev);
> > > +
> > >  #endif
> > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > > index db2da99f6aa7..e9eca99cf493 100644
> > > --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > > +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > > @@ -376,3 +376,14 @@ void nfp_devlink_port_unregister(struct nfp_port *port)
> > >  {
> > >  	devlink_port_unregister(&port->dl_port);
> > >  }
> > > +
> > > +struct devlink *nfp_devlink_get_devlink(struct net_device *netdev)
> > > +{
> > > +	struct nfp_app *app;
> > > +
> > > +	app = nfp_app_from_netdev(netdev);
> > > +	if (!app)
> > > +		return NULL;
> > > +
> > > +	return priv_to_devlink(app->pf);
> > > +}  
> > 
> > AFAICS this would return a pointer to zero initialized struct devlink
> > when built with CONFIG_DEVLINK=n. Then devlink_compat_running_version()
> > would execute
> > 
> > 	if (!dev->netdev_ops->ndo_get_devlink)
> > 		return;
> > 
> > 	devlink = dev->netdev_ops->ndo_get_devlink(dev);
> > 	if (!devlink || !devlink->ops->info_get)
> > 		return;
> > 
> > with non-null devlink but null devlink->ops so that it dereferences null
> > pointer (and so does devlink_compat_flash_update()).
> 
> devlink_compat_flash_update() is this if CONFIG_DEVLINK=n:
> 
> static inline int
> devlink_compat_flash_update(struct net_device *dev, const char *file_name)
> {
> 	return -EOPNOTSUPP;
> }
> 
> No?

You are right, I missed that devlink_compat_running_version() and
devlink_compat_flash_update() also have alternative versions when
devlink is disabled. So there is no problem.

> > Maybe it would be safer not to call ndo_get_devlink directly and have
> > an inline wrapper like
> > 
> > #if IS_ENABLED(CONFIG_NET_DEVLINK)
> > static inline struct devlink *dev_get_devlink(struct net_device *dev)
> > {
> > 	if (dev->netdev_ops->ndo_get_devlink)
> > 		return dev->netdev_ops->ndo_get_devlink();
> > 	else
> > 		retrurn NULL;
> > }
> > #else
> > static inline struct devlink *dev_get_devlink(struct net_device *dev)
> > {
> > 	return NULL;
> > }
> > #endif
> > 
> > so that one can simply call the wrapper and check return value for NULL.
> 
> Only devlink code can call this ndo, and it doesn't exist with
> DEVLINK=n.  I don't dislike wrappers for NDOs, but I'll defer to Jiri
> to decide if we want a wrapper here (without the #if/#else, just the
> first part for code clarity) :)

If the NDO is only supposed to be called from devlink code (or, more
precisely, code built only with CONFIG_DEVLINK=y), it should be IMHO
mentioned in its description. Another option would be enforcing it by
adding #ifdef around the ndo_get_devlink entry in struct net_device_ops
but that would require using ifdefs also in each driver providing the
NDO which seems inconvenient.

Michal