| Message ID | 20190221174620.12144-4-jakub.kicinski@netronome.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | devlink: make ethtool compat reliable | expand |
On Thu, Feb 21, 2019 at 09:46:18AM -0800, Jakub Kicinski wrote: > Instead of iterating over all devlink ports add a NDO which > will return the devlink instance from the driver. > > Suggested-by: Jiri Pirko <jiri@resnulli.us> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > include/linux/netdevice.h | 6 ++++ > net/core/devlink.c | 62 +++++++++++++-------------------------- > 2 files changed, 26 insertions(+), 42 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index aab4d9f6613d..eebcef6b8191 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -940,6 +940,8 @@ struct dev_ifalias { > char ifalias[]; > }; > > +struct devlink; > + > /* > * This structure defines the management hooks for network devices. > * The following hooks can be defined; unless noted otherwise, they are > @@ -1248,6 +1250,9 @@ struct dev_ifalias { > * that got dropped are freed/returned via xdp_return_frame(). > * Returns negative number, means general error invoking ndo, meaning > * no frames were xmit'ed and core-caller will free all frames. > + * struct devlink *(*ndo_get_devlink)(struct net_device *dev); > + * Get devlink instance associated with a given netdev. > + * Called with a reference on the device only, rtnl_lock is not held. The formulation is a bit ambiguous. Do I understand correctly that what it says is that device reference is sufficient and rtnl_lock is not necessary (but there is no harm if caller holds rtnl_lock)? Michal
On Fri, 22 Feb 2019 10:51:31 +0100, Michal Kubecek wrote: > On Thu, Feb 21, 2019 at 09:46:18AM -0800, Jakub Kicinski wrote: > > Instead of iterating over all devlink ports add a NDO which > > will return the devlink instance from the driver. > > > > Suggested-by: Jiri Pirko <jiri@resnulli.us> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > --- > > include/linux/netdevice.h | 6 ++++ > > net/core/devlink.c | 62 +++++++++++++-------------------------- > > 2 files changed, 26 insertions(+), 42 deletions(-) > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index aab4d9f6613d..eebcef6b8191 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -940,6 +940,8 @@ struct dev_ifalias { > > char ifalias[]; > > }; > > > > +struct devlink; > > + > > /* > > * This structure defines the management hooks for network devices. > > * The following hooks can be defined; unless noted otherwise, they are > > @@ -1248,6 +1250,9 @@ struct dev_ifalias { > > * that got dropped are freed/returned via xdp_return_frame(). > > * Returns negative number, means general error invoking ndo, meaning > > * no frames were xmit'ed and core-caller will free all frames. > > + * struct devlink *(*ndo_get_devlink)(struct net_device *dev); > > + * Get devlink instance associated with a given netdev. > > + * Called with a reference on the device only, rtnl_lock is not held. > > The formulation is a bit ambiguous. Do I understand correctly that what > it says is that device reference is sufficient and rtnl_lock is not > necessary (but there is no harm if caller holds rtnl_lock)? Yes, only reference is held, the expectation is that drivers free netdevs before they unregister devlink. The devlink vs rtnl ordering gets a bit tricky, drivers should definitely not expect rtnl to be held in this NDO. If the caller comes from rtnl path (like ethtool) it will likely have to release rtnl before taking devlink->lock.
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index aab4d9f6613d..eebcef6b8191 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -940,6 +940,8 @@ struct dev_ifalias { char ifalias[]; }; +struct devlink; + /* * This structure defines the management hooks for network devices. * The following hooks can be defined; unless noted otherwise, they are @@ -1248,6 +1250,9 @@ struct dev_ifalias { * that got dropped are freed/returned via xdp_return_frame(). * Returns negative number, means general error invoking ndo, meaning * no frames were xmit'ed and core-caller will free all frames. + * struct devlink *(*ndo_get_devlink)(struct net_device *dev); + * Get devlink instance associated with a given netdev. + * Called with a reference on the device only, rtnl_lock is not held. */ struct net_device_ops { int (*ndo_init)(struct net_device *dev); @@ -1446,6 +1451,7 @@ struct net_device_ops { u32 flags); int (*ndo_xsk_async_xmit)(struct net_device *dev, u32 queue_id); + struct devlink * (*ndo_get_devlink)(struct net_device *dev); }; /** diff --git a/net/core/devlink.c b/net/core/devlink.c index fcf09e02f4c6..050999a68c35 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6395,9 +6395,6 @@ static void __devlink_compat_running_version(struct devlink *devlink, struct sk_buff *msg; int rem, err; - if (!devlink->ops->info_get) - return; - msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); if (!msg) return; @@ -6429,55 +6426,36 @@ static void __devlink_compat_running_version(struct devlink *devlink, void devlink_compat_running_version(struct net_device *dev, char *buf, size_t len) { - struct devlink_port *devlink_port; struct devlink *devlink; - mutex_lock(&devlink_mutex); - list_for_each_entry(devlink, &devlink_list, list) { - mutex_lock(&devlink->lock); - list_for_each_entry(devlink_port, &devlink->port_list, list) { - if (devlink_port->type == DEVLINK_PORT_TYPE_ETH && - devlink_port->type_dev == dev) { - __devlink_compat_running_version(devlink, - buf, len); - mutex_unlock(&devlink->lock); - goto out; - } - } - mutex_unlock(&devlink->lock); - } -out: - mutex_unlock(&devlink_mutex); + if (!dev->netdev_ops->ndo_get_devlink) + return; + + devlink = dev->netdev_ops->ndo_get_devlink(dev); + if (!devlink || !devlink->ops->info_get) + return; + + mutex_lock(&devlink->lock); + __devlink_compat_running_version(devlink, buf, len); + mutex_unlock(&devlink->lock); } int devlink_compat_flash_update(struct net_device *dev, const char *file_name) { - struct devlink_port *devlink_port; struct devlink *devlink; + int ret; - mutex_lock(&devlink_mutex); - list_for_each_entry(devlink, &devlink_list, list) { - mutex_lock(&devlink->lock); - list_for_each_entry(devlink_port, &devlink->port_list, list) { - int ret = -EOPNOTSUPP; - - if (devlink_port->type != DEVLINK_PORT_TYPE_ETH || - devlink_port->type_dev != dev) - continue; + if (!dev->netdev_ops->ndo_get_devlink) + return -EOPNOTSUPP; - mutex_unlock(&devlink_mutex); - if (devlink->ops->flash_update) - ret = devlink->ops->flash_update(devlink, - file_name, - NULL, NULL); - mutex_unlock(&devlink->lock); - return ret; - } - mutex_unlock(&devlink->lock); - } - mutex_unlock(&devlink_mutex); + devlink = dev->netdev_ops->ndo_get_devlink(dev); + if (!devlink || !devlink->ops->flash_update) + return -EOPNOTSUPP; - return -EOPNOTSUPP; + mutex_lock(&devlink->lock); + ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL); + mutex_unlock(&devlink->lock); + return ret; } static int __init devlink_init(void)
Instead of iterating over all devlink ports add a NDO which will return the devlink instance from the driver. Suggested-by: Jiri Pirko <jiri@resnulli.us> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- include/linux/netdevice.h | 6 ++++ net/core/devlink.c | 62 +++++++++++++-------------------------- 2 files changed, 26 insertions(+), 42 deletions(-)