Subject: Re: [patch net-next 06/11] net: devlink: don't take devlink_mutex for devlink_compat_*

From: Jiri Pirko <[email-masked]>
Date: Fri, 22 Mar 2019 14:44:52 +0100

Thu, Mar 21, 2019 at 08:08:24PM CET, [email-masked] wrote:
>On Thu, 21 Mar 2019 14:20:14 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <[email-masked]>
>> 
>> The netdevice is guaranteed to not disappear so we can rely that
>> devlink_port and devlink won't disappear as well. No need to take
>> devlink_mutex so don't take it here.
>
>I'm not sure the port can't disappear, just looking at this series it
>seems bnxt registers the port after netdev (maybe it unregisters in 
>the other order).  Not that it matters here, we use the main devlink
>instance for the compat helpers, and devlink instance should
>definitely exist.

You are right. Btw, the devlink_port-netdev registration order should be
fixed as well. Niting it down to my todo.

>
>So FWIW the entire series looks good to me.
>
>I've also pushed my port patches out:
>
>https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/log/?h=devlink-pci-ports
>
>if that's of any use to you (e.g. the patch which changes the order of
>port vs netdev registration).

Thanks.

>
>> Signed-off-by: Jiri Pirko <[email-masked]>
>
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 3dc51ddf7451..1e125c3b890c 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -6444,17 +6444,15 @@ void devlink_compat_running_version(struct net_device *dev,
>>  	dev_hold(dev);
>>  	rtnl_unlock();
>>  
>> -	mutex_lock(&devlink_mutex);
>>  	devlink = netdev_to_devlink(dev);
>>  	if (!devlink || !devlink->ops->info_get)
>> -		goto unlock_list;
>> +		goto out;
>>  
>>  	mutex_lock(&devlink->lock);
>>  	__devlink_compat_running_version(devlink, buf, len);
>>  	mutex_unlock(&devlink->lock);
>> -unlock_list:
>> -	mutex_unlock(&devlink_mutex);
>>  
>> +out:
>>  	rtnl_lock();
>>  	dev_put(dev);
>>  }
>> @@ -6462,22 +6460,22 @@ void devlink_compat_running_version(struct net_device *dev,
>>  int devlink_compat_flash_update(struct net_device *dev, const char *file_name)
>>  {
>>  	struct devlink *devlink;
>> -	int ret = -EOPNOTSUPP;
>> +	int ret;
>>  
>>  	dev_hold(dev);
>>  	rtnl_unlock();
>>  
>> -	mutex_lock(&devlink_mutex);
>>  	devlink = netdev_to_devlink(dev);
>> -	if (!devlink || !devlink->ops->flash_update)
>> -		goto unlock_list;
>> +	if (!devlink || !devlink->ops->flash_update) {
>> +		ret = -EOPNOTSUPP;
>> +		goto out;
>> +	}
>>  
>>  	mutex_lock(&devlink->lock);
>>  	ret = devlink->ops->flash_update(devlink, file_name, NULL, NULL);
>>  	mutex_unlock(&devlink->lock);
>> -unlock_list:
>> -	mutex_unlock(&devlink_mutex);
>>  
>> +out:
>>  	rtnl_lock();
>>  	dev_put(dev);
>>  
>