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.kicin...@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) :)
