Re: [patch net-next 2/5] net: devlink: introduce devlink_compat_phys_port_name_get()

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

On Fri,  1 Mar 2019 09:13:59 +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Introduce devlink_compat_phys_port_name_get() helper that
> gets the physical port name for specified netdevice
> according to devlink port attributes.
> Call this helper from dev_get_phys_port_name()
> in case ndo_get_phys_port_name is not defined.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/net/devlink.h |  9 +++++++++
>  net/core/dev.c        |  3 ++-
>  net/core/devlink.c    | 30 ++++++++++++++++++++++++++++--
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index f34107def48e..ae2cd34da99e 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -729,6 +729,8 @@ int devlink_health_report(struct devlink_health_reporter *reporter,
>  void devlink_compat_running_version(struct net_device *dev,
>  				    char *buf, size_t len);
>  int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
> +int devlink_compat_phys_port_name_get(struct net_device *dev,
> +				      char *name, size_t len);
>  
>  #else
>  
> @@ -1224,6 +1226,13 @@ devlink_compat_flash_update(struct net_device *dev, const char *file_name)
>  {
>  	return -EOPNOTSUPP;
>  }
> +
> +static inline int
> +devlink_compat_phys_port_name_get(struct net_device *dev,
> +				  char *name, size_t len)
> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif
>  
>  #endif /* _NET_DEVLINK_H_ */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2b67f2aa59dd..a614b26b0842 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -146,6 +146,7 @@
>  #include <net/udp_tunnel.h>
>  #include <linux/net_namespace.h>
>  #include <linux/indirect_call_wrapper.h>
> +#include <net/devlink.h>
>  
>  #include "net-sysfs.h"
>  
> @@ -7872,7 +7873,7 @@ int dev_get_phys_port_name(struct net_device *dev,
>  	const struct net_device_ops *ops = dev->netdev_ops;
>  
>  	if (!ops->ndo_get_phys_port_name)
> -		return -EOPNOTSUPP;
> +		return devlink_compat_phys_port_name_get(dev, name, len);
>  	return ops->ndo_get_phys_port_name(dev, name, len);
>  }
>  EXPORT_SYMBOL(dev_get_phys_port_name);
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index a49dee67e66f..4fd45d4a4818 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -5419,8 +5419,8 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
>  }
>  EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
>  
> -int devlink_port_get_phys_port_name(struct devlink_port *devlink_port,
> -				    char *name, size_t len)
> +static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
> +					     char *name, size_t len)
>  {
>  	struct devlink_port_attrs *attrs = &devlink_port->attrs;
>  	int n = 0;
> @@ -5450,6 +5450,12 @@ int devlink_port_get_phys_port_name(struct devlink_port *devlink_port,
>  
>  	return 0;
>  }
> +
> +int devlink_port_get_phys_port_name(struct devlink_port *devlink_port,
> +				    char *name, size_t len)
> +{
> +	return __devlink_port_phys_port_name_get(devlink_port, name, len);
> +}
>  EXPORT_SYMBOL_GPL(devlink_port_get_phys_port_name);
>  
>  int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
> @@ -6469,6 +6475,26 @@ int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
>  	return ret;
>  }
>  
> +int devlink_compat_phys_port_name_get(struct net_device *dev,
> +				      char *name, size_t len)
> +{
> +	struct devlink_port *devlink_port;
> +	int err = -EOPNOTSUPP;
> +
> +	mutex_lock(&devlink_mutex);
> +	devlink_port = netdev_to_devlink_port(dev);
> +	if (!devlink_port)
> +		goto unlock_list;
> +
> +	mutex_lock(&devlink_port->devlink->lock);

I'm moderately confident you won't be able to take devlink locks from
RTNL code :(  I think the locking order is:

  devlink_mutex -> devlink->lock -> rtnl_lock

No? :(  At least NFP assumes that, but I think port splitting does, too.

> +	err = __devlink_port_phys_port_name_get(devlink_port, name, len);
> +	mutex_unlock(&devlink_port->devlink->lock);
> +unlock_list:
> +	mutex_unlock(&devlink_mutex);
> +
> +	return err;
> +}
> +
>  static int __init devlink_init(void)
>  {
>  	return genl_register_family(&devlink_nl_family);