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.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?
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
