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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

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?

> 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) :)