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

[Jiri Pirko <jiri@resnulli.us>]

Fri, Mar 01, 2019 at 05:14:58PM CET, jakub.kicinski@netronome.com wrote:
>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.

Oh right, thanks for spotting this. Will fix.


>
>> +	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);
>