[patch net-next 00/11] devlink: small spring cleanup

[patch net-next 00/11] devlink: small spring cleanup

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Mostly cosmetics and janitor work.

Jiri Pirko (11):
  net: devlink: add couple of missing mutex_destroy() calls
  bnxt: add missing net/devlink.h include
  dsa: add missing net/devlink.h include
  bnxt: set devlink port attrs properly
  bnxt: call devlink_port_type_eth_set() before port register
  net: devlink: don't take devlink_mutex for devlink_compat_*
  net: devlink: don't pass return value of __devlink_port_type_set()
  mlxsw: Move devlink_port_attrs_set() call before register
  dsa: move devlink_port_attrs_set() call before register
  net: devlink: disallow port_attrs_set() to be called before register
  net: devlink: add port type spinlock

 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  5 +-
 drivers/net/ethernet/mellanox/mlxsw/core.c    | 12 ++---
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  8 +--
 drivers/net/ethernet/mellanox/mlxsw/minimal.c |  5 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  6 +--
 .../net/ethernet/mellanox/mlxsw/switchib.c    |  3 +-
 .../net/ethernet/mellanox/mlxsw/switchx2.c    |  5 +-
 include/net/devlink.h                         |  4 ++
 net/core/devlink.c                            | 49 +++++++++++--------
 net/dsa/dsa2.c                                | 48 ++++++++++--------
 10 files changed, 84 insertions(+), 61 deletions(-)

-- 
2.17.2

[patch net-next 01/11] net: devlink: add couple of missing mutex_destroy() calls

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Add missing called to mutex_destroy() for two mutexes used
in devlink code.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/core/devlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 78e22cea4cc7..3dc51ddf7451 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4486,6 +4486,7 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
 {
 	mutex_lock(&reporter->devlink->lock);
 	list_del(&reporter->list);
+	mutex_destroy(&reporter->dump_lock);
 	mutex_unlock(&reporter->devlink->lock);
 	if (reporter->dump_fmsg)
 		devlink_fmsg_free(reporter->dump_fmsg);
@@ -5298,6 +5299,7 @@ EXPORT_SYMBOL_GPL(devlink_unregister);
  */
 void devlink_free(struct devlink *devlink)
 {
+	mutex_destroy(&devlink->lock);
 	WARN_ON(!list_empty(&devlink->reporter_list));
 	WARN_ON(!list_empty(&devlink->region_list));
 	WARN_ON(!list_empty(&devlink->param_list));
-- 
2.17.2

[patch net-next 02/11] bnxt: add missing net/devlink.h include

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

devlink functions are in use, so include the related header file.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index e1feb97bcd81..ecb328ee8f5c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -9,6 +9,7 @@
 
 #include <linux/pci.h>
 #include <linux/netdevice.h>
+#include <net/devlink.h>
 #include "bnxt_hsi.h"
 #include "bnxt.h"
 #include "bnxt_vfr.h"
-- 
2.17.2

Re: [patch net-next 02/11] bnxt: add missing net/devlink.h include

[Vasundhara Volam]

On Thu, Mar 21, 2019 at 6:50 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> From: Jiri Pirko <jiri@mellanox.com>
>
> devlink functions are in use, so include the related header file.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index e1feb97bcd81..ecb328ee8f5c 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -9,6 +9,7 @@
>
>  #include <linux/pci.h>
>  #include <linux/netdevice.h>
> +#include <net/devlink.h>
Jiri,
This net/devlink.h file is already included in bnxt.h file which is
included in bnxt_devlink.c.
Do you face any issues with existing code while compiling or testing?

Thanks,
Vasundhara


>  #include "bnxt_hsi.h"
>  #include "bnxt.h"
>  #include "bnxt_vfr.h"
> --
> 2.17.2
>

Re: [patch net-next 02/11] bnxt: add missing net/devlink.h include

[Jiri Pirko]

Fri, Mar 22, 2019 at 10:16:34AM CET, vasundhara-v.volam@broadcom.com wrote:
>On Thu, Mar 21, 2019 at 6:50 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> devlink functions are in use, so include the related header file.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> index e1feb97bcd81..ecb328ee8f5c 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> @@ -9,6 +9,7 @@
>>
>>  #include <linux/pci.h>
>>  #include <linux/netdevice.h>
>> +#include <net/devlink.h>
>Jiri,
>This net/devlink.h file is already included in bnxt.h file which is
>included in bnxt_devlink.c.
>Do you face any issues with existing code while compiling or testing?

It is better to include directly, not to rely on some header to include
it.

>
>Thanks,
>Vasundhara
>
>
>>  #include "bnxt_hsi.h"
>>  #include "bnxt.h"
>>  #include "bnxt_vfr.h"
>> --
>> 2.17.2
>>

[patch net-next 03/11] dsa: add missing net/devlink.h include

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

devlink functions are in use, so include the related header file.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/dsa/dsa2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index c00ee464afc7..4558de672b4f 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -18,6 +18,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
+#include <net/devlink.h>
 
 #include "dsa_priv.h"
 
-- 
2.17.2

Re: [patch net-next 03/11] dsa: add missing net/devlink.h include

[Andrew Lunn]

On Thu, Mar 21, 2019 at 02:20:11PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> devlink functions are in use, so include the related header file.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[patch net-next 04/11] bnxt: set devlink port attrs properly

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Set the attrs properly so delink has enough info to generate physical
port names.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index ecb328ee8f5c..ab6fd05c462b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -229,6 +229,8 @@ int bnxt_dl_register(struct bnxt *bp)
 		goto err_dl_unreg;
 	}
 
+	devlink_port_attrs_set(&bp->dl_port, DEVLINK_PORT_FLAVOUR_PHYSICAL,
+			       bp->pf.port_id, false, 0);
 	rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
 	if (rc) {
 		netdev_err(bp->dev, "devlink_port_register failed");
-- 
2.17.2

[patch net-next 05/11] bnxt: call devlink_port_type_eth_set() before port register

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Call devlink_port_type_eth_set() before devlink_port_register(). Bnxt
instances won't change type during lifetime. This avoids one extra
userspace devlink notification.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index ab6fd05c462b..a266bff559dc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -229,6 +229,7 @@ int bnxt_dl_register(struct bnxt *bp)
 		goto err_dl_unreg;
 	}
 
+	devlink_port_type_eth_set(&bp->dl_port, bp->dev);
 	devlink_port_attrs_set(&bp->dl_port, DEVLINK_PORT_FLAVOUR_PHYSICAL,
 			       bp->pf.port_id, false, 0);
 	rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
@@ -236,7 +237,6 @@ int bnxt_dl_register(struct bnxt *bp)
 		netdev_err(bp->dev, "devlink_port_register failed");
 		goto err_dl_param_unreg;
 	}
-	devlink_port_type_eth_set(&bp->dl_port, bp->dev);
 
 	rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
 					  ARRAY_SIZE(bnxt_dl_port_params));
-- 
2.17.2

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

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

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.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/core/devlink.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

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

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

[Jakub Kicinski]

On Thu, 21 Mar 2019 14:20:14 +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> 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.

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

> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

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

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

[Jiri Pirko]

Thu, Mar 21, 2019 at 08:08:24PM CET, jakub.kicinski@netronome.com wrote:
>On Thu, 21 Mar 2019 14:20:14 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> 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 <jiri@mellanox.com>
>
>> 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);
>>  
>

[patch net-next 07/11] net: devlink: don't pass return value of __devlink_port_type_set()

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

__devlink_port_type_set() returns void, it makes no sense to pass it on,
so don't do that.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/core/devlink.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 1e125c3b890c..98ca788ffd97 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5379,8 +5379,7 @@ static void __devlink_port_type_set(struct devlink_port *devlink_port,
 void devlink_port_type_eth_set(struct devlink_port *devlink_port,
 			       struct net_device *netdev)
 {
-	return __devlink_port_type_set(devlink_port,
-				       DEVLINK_PORT_TYPE_ETH, netdev);
+	__devlink_port_type_set(devlink_port, DEVLINK_PORT_TYPE_ETH, netdev);
 }
 EXPORT_SYMBOL_GPL(devlink_port_type_eth_set);
 
@@ -5393,8 +5392,7 @@ EXPORT_SYMBOL_GPL(devlink_port_type_eth_set);
 void devlink_port_type_ib_set(struct devlink_port *devlink_port,
 			      struct ib_device *ibdev)
 {
-	return __devlink_port_type_set(devlink_port,
-				       DEVLINK_PORT_TYPE_IB, ibdev);
+	__devlink_port_type_set(devlink_port, DEVLINK_PORT_TYPE_IB, ibdev);
 }
 EXPORT_SYMBOL_GPL(devlink_port_type_ib_set);
 
@@ -5405,8 +5403,7 @@ EXPORT_SYMBOL_GPL(devlink_port_type_ib_set);
  */
 void devlink_port_type_clear(struct devlink_port *devlink_port)
 {
-	return __devlink_port_type_set(devlink_port,
-				       DEVLINK_PORT_TYPE_NOTSET, NULL);
+	__devlink_port_type_set(devlink_port, DEVLINK_PORT_TYPE_NOTSET, NULL);
 }
 EXPORT_SYMBOL_GPL(devlink_port_type_clear);
 
-- 
2.17.2

[patch net-next 08/11] mlxsw: Move devlink_port_attrs_set() call before register

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Since attrs are static during the existence of devlink port, set the
before registration of the port.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c     | 12 ++++++------
 drivers/net/ethernet/mellanox/mlxsw/core.h     |  8 ++++----
 drivers/net/ethernet/mellanox/mlxsw/minimal.c  |  5 +++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c |  6 +++---
 drivers/net/ethernet/mellanox/mlxsw/switchib.c |  3 ++-
 drivers/net/ethernet/mellanox/mlxsw/switchx2.c |  5 +++--
 6 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index d23d53c0e284..e70bb673eeec 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -1718,7 +1718,9 @@ u64 mlxsw_core_res_get(struct mlxsw_core *mlxsw_core,
 }
 EXPORT_SYMBOL(mlxsw_core_res_get);
 
-int mlxsw_core_port_init(struct mlxsw_core *mlxsw_core, u8 local_port)
+int mlxsw_core_port_init(struct mlxsw_core *mlxsw_core, u8 local_port,
+			 u32 port_number, bool split,
+			 u32 split_port_subnumber)
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_core);
 	struct mlxsw_core_port *mlxsw_core_port =
@@ -1727,6 +1729,8 @@ int mlxsw_core_port_init(struct mlxsw_core *mlxsw_core, u8 local_port)
 	int err;
 
 	mlxsw_core_port->local_port = local_port;
+	devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PHYSICAL,
+			       port_number, split, split_port_subnumber);
 	err = devlink_port_register(devlink, devlink_port, local_port);
 	if (err)
 		memset(mlxsw_core_port, 0, sizeof(*mlxsw_core_port));
@@ -1746,17 +1750,13 @@ void mlxsw_core_port_fini(struct mlxsw_core *mlxsw_core, u8 local_port)
 EXPORT_SYMBOL(mlxsw_core_port_fini);
 
 void mlxsw_core_port_eth_set(struct mlxsw_core *mlxsw_core, u8 local_port,
-			     void *port_driver_priv, struct net_device *dev,
-			     u32 port_number, bool split,
-			     u32 split_port_subnumber)
+			     void *port_driver_priv, struct net_device *dev)
 {
 	struct mlxsw_core_port *mlxsw_core_port =
 					&mlxsw_core->ports[local_port];
 	struct devlink_port *devlink_port = &mlxsw_core_port->devlink_port;
 
 	mlxsw_core_port->port_driver_priv = port_driver_priv;
-	devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PHYSICAL,
-			       port_number, split, split_port_subnumber);
 	devlink_port_type_eth_set(devlink_port, dev);
 }
 EXPORT_SYMBOL(mlxsw_core_port_eth_set);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index 8ec53f027575..74e95e943b24 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -164,12 +164,12 @@ void mlxsw_core_lag_mapping_clear(struct mlxsw_core *mlxsw_core,
 				  u16 lag_id, u8 local_port);
 
 void *mlxsw_core_port_driver_priv(struct mlxsw_core_port *mlxsw_core_port);
-int mlxsw_core_port_init(struct mlxsw_core *mlxsw_core, u8 local_port);
+int mlxsw_core_port_init(struct mlxsw_core *mlxsw_core, u8 local_port,
+			 u32 port_number, bool split,
+			 u32 split_port_subnumber);
 void mlxsw_core_port_fini(struct mlxsw_core *mlxsw_core, u8 local_port);
 void mlxsw_core_port_eth_set(struct mlxsw_core *mlxsw_core, u8 local_port,
-			     void *port_driver_priv, struct net_device *dev,
-			     u32 port_number, bool split,
-			     u32 split_port_subnumber);
+			     void *port_driver_priv, struct net_device *dev);
 void mlxsw_core_port_ib_set(struct mlxsw_core *mlxsw_core, u8 local_port,
 			    void *port_driver_priv);
 void mlxsw_core_port_clear(struct mlxsw_core *mlxsw_core, u8 local_port,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/minimal.c b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
index 00c390024350..0ee1656609f5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/minimal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/minimal.c
@@ -150,7 +150,8 @@ mlxsw_m_port_create(struct mlxsw_m *mlxsw_m, u8 local_port, u8 module)
 	struct net_device *dev;
 	int err;
 
-	err = mlxsw_core_port_init(mlxsw_m->core, local_port);
+	err = mlxsw_core_port_init(mlxsw_m->core, local_port,
+				   module + 1, false, 0);
 	if (err) {
 		dev_err(mlxsw_m->bus_info->dev, "Port %d: Failed to init core port\n",
 			local_port);
@@ -190,7 +191,7 @@ mlxsw_m_port_create(struct mlxsw_m *mlxsw_m, u8 local_port, u8 module)
 	}
 
 	mlxsw_core_port_eth_set(mlxsw_m->core, mlxsw_m_port->local_port,
-				mlxsw_m_port, dev, module + 1, false, 0);
+				mlxsw_m_port, dev);
 
 	return 0;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 9eb63300c1d3..eaf86c4c2f6c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3391,7 +3391,8 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
 	struct net_device *dev;
 	int err;
 
-	err = mlxsw_core_port_init(mlxsw_sp->core, local_port);
+	err = mlxsw_core_port_init(mlxsw_sp->core, local_port,
+				   module + 1, split, lane / width);
 	if (err) {
 		dev_err(mlxsw_sp->bus_info->dev, "Port %d: Failed to init core port\n",
 			local_port);
@@ -3573,8 +3574,7 @@ static int mlxsw_sp_port_create(struct mlxsw_sp *mlxsw_sp, u8 local_port,
 	}
 
 	mlxsw_core_port_eth_set(mlxsw_sp->core, mlxsw_sp_port->local_port,
-				mlxsw_sp_port, dev, module + 1,
-				mlxsw_sp_port->split, lane / width);
+				mlxsw_sp_port, dev);
 	mlxsw_core_schedule_dw(&mlxsw_sp_port->periodic_hw_stats.update_dw, 0);
 	return 0;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/switchib.c b/drivers/net/ethernet/mellanox/mlxsw/switchib.c
index bcf2e79a21c8..e1e7e0dd808d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/switchib.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/switchib.c
@@ -267,7 +267,8 @@ static int mlxsw_sib_port_create(struct mlxsw_sib *mlxsw_sib, u8 local_port,
 {
 	int err;
 
-	err = mlxsw_core_port_init(mlxsw_sib->core, local_port);
+	err = mlxsw_core_port_init(mlxsw_sib->core, local_port,
+				   module + 1, false, 0);
 	if (err) {
 		dev_err(mlxsw_sib->bus_info->dev, "Port %d: Failed to init core port\n",
 			local_port);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
index 533fe6235b7c..568883fc40df 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/switchx2.c
@@ -1102,7 +1102,7 @@ static int __mlxsw_sx_port_eth_create(struct mlxsw_sx *mlxsw_sx, u8 local_port,
 	}
 
 	mlxsw_core_port_eth_set(mlxsw_sx->core, mlxsw_sx_port->local_port,
-				mlxsw_sx_port, dev, module + 1, false, 0);
+				mlxsw_sx_port, dev);
 	mlxsw_sx->ports[local_port] = mlxsw_sx_port;
 	return 0;
 
@@ -1127,7 +1127,8 @@ static int mlxsw_sx_port_eth_create(struct mlxsw_sx *mlxsw_sx, u8 local_port,
 {
 	int err;
 
-	err = mlxsw_core_port_init(mlxsw_sx->core, local_port);
+	err = mlxsw_core_port_init(mlxsw_sx->core, local_port,
+				   module + 1, false, 0);
 	if (err) {
 		dev_err(mlxsw_sx->bus_info->dev, "Port %d: Failed to init core port\n",
 			local_port);
-- 
2.17.2

[patch net-next 09/11] dsa: move devlink_port_attrs_set() call before register

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Since attrs are static during the existence of devlink port, set the
before registration of the port.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/dsa/dsa2.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 4558de672b4f..6d71569afd88 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -258,14 +258,36 @@ static void dsa_tree_teardown_default_cpu(struct dsa_switch_tree *dst)
 
 static int dsa_port_setup(struct dsa_port *dp)
 {
+	enum devlink_port_flavour flavour;
 	struct dsa_switch *ds = dp->ds;
-	int err = 0;
+	int err;
+
+	if (dp->type == DSA_PORT_TYPE_UNUSED)
+		return 0;
 
 	memset(&dp->devlink_port, 0, sizeof(dp->devlink_port));
 
-	if (dp->type != DSA_PORT_TYPE_UNUSED)
-		err = devlink_port_register(ds->devlink, &dp->devlink_port,
-					    dp->index);
+	switch (dp->type) {
+	case DSA_PORT_TYPE_CPU:
+		flavour = DEVLINK_PORT_FLAVOUR_CPU;
+		break;
+	case DSA_PORT_TYPE_DSA:
+		flavour = DEVLINK_PORT_FLAVOUR_DSA;
+		break;
+	case DSA_PORT_TYPE_USER: /* fall-through */
+	default:
+		flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
+		break;
+	}
+
+	/* dp->index is used now as port_number. However
+	 * CPU ports should have separate numbering
+	 * independent from front panel port numbers.
+	 */
+	devlink_port_attrs_set(&dp->devlink_port, flavour,
+			       dp->index, false, 0);
+	err = devlink_port_register(ds->devlink, &dp->devlink_port,
+				    dp->index);
 	if (err)
 		return err;
 
@@ -273,13 +295,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 	case DSA_PORT_TYPE_UNUSED:
 		break;
 	case DSA_PORT_TYPE_CPU:
-		/* dp->index is used now as port_number. However
-		 * CPU ports should have separate numbering
-		 * independent from front panel port numbers.
-		 */
-		devlink_port_attrs_set(&dp->devlink_port,
-				       DEVLINK_PORT_FLAVOUR_CPU,
-				       dp->index, false, 0);
 		err = dsa_port_link_register_of(dp);
 		if (err) {
 			dev_err(ds->dev, "failed to setup link for port %d.%d\n",
@@ -288,13 +303,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 		}
 		break;
 	case DSA_PORT_TYPE_DSA:
-		/* dp->index is used now as port_number. However
-		 * DSA ports should have separate numbering
-		 * independent from front panel port numbers.
-		 */
-		devlink_port_attrs_set(&dp->devlink_port,
-				       DEVLINK_PORT_FLAVOUR_DSA,
-				       dp->index, false, 0);
 		err = dsa_port_link_register_of(dp);
 		if (err) {
 			dev_err(ds->dev, "failed to setup link for port %d.%d\n",
@@ -303,9 +311,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 		}
 		break;
 	case DSA_PORT_TYPE_USER:
-		devlink_port_attrs_set(&dp->devlink_port,
-				       DEVLINK_PORT_FLAVOUR_PHYSICAL,
-				       dp->index, false, 0);
 		err = dsa_slave_create(dp);
 		if (err)
 			dev_err(ds->dev, "failed to create slave for port %d.%d\n",
-- 
2.17.2

Re: [patch net-next 09/11] dsa: move devlink_port_attrs_set() call before register

[Andrew Lunn]

On Thu, Mar 21, 2019 at 02:20:17PM +0100, Jiri Pirko wrote:
> +	switch (dp->type) {
> +	case DSA_PORT_TYPE_CPU:
> +		flavour = DEVLINK_PORT_FLAVOUR_CPU;
> +		break;
> +	case DSA_PORT_TYPE_DSA:
> +		flavour = DEVLINK_PORT_FLAVOUR_DSA;
> +		break;
> +	case DSA_PORT_TYPE_USER: /* fall-through */
> +	default:
> +		flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
> +		break;
> +	}
> +
> +	/* dp->index is used now as port_number. However
> +	 * CPU ports should have separate numbering
> +	 * independent from front panel port numbers.
> +	 */

Hi Jiri

Given the way you have rearranged this code, i think the text above
should now be:

However CPU ports and DSA ports should have separate numbering.

	Andrew

Re: [patch net-next 09/11] dsa: move devlink_port_attrs_set() call before register

[Jiri Pirko]

Thu, Mar 21, 2019 at 02:41:01PM CET, andrew@lunn.ch wrote:
>On Thu, Mar 21, 2019 at 02:20:17PM +0100, Jiri Pirko wrote:
>> +	switch (dp->type) {
>> +	case DSA_PORT_TYPE_CPU:
>> +		flavour = DEVLINK_PORT_FLAVOUR_CPU;
>> +		break;
>> +	case DSA_PORT_TYPE_DSA:
>> +		flavour = DEVLINK_PORT_FLAVOUR_DSA;
>> +		break;
>> +	case DSA_PORT_TYPE_USER: /* fall-through */
>> +	default:
>> +		flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>> +		break;
>> +	}
>> +
>> +	/* dp->index is used now as port_number. However
>> +	 * CPU ports should have separate numbering
>> +	 * independent from front panel port numbers.
>> +	 */
>
>Hi Jiri
>
>Given the way you have rearranged this code, i think the text above
>should now be:
>
>However CPU ports and DSA ports should have separate numbering.

You are right. Will fix in v2.

>
>	Andrew

[patch net-next 10/11] net: devlink: disallow port_attrs_set() to be called before register

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Since the port attributes are static and cannot change during the port
lifetime, WARN_ON if some driver calls it after registration. Also, no
need to call notifications as it is noop anyway due to check of
devlink_port->registered there.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/core/devlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 98ca788ffd97..f77a68f72d14 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5425,12 +5425,13 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
 {
 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
 
+	if (WARN_ON(devlink_port->registered))
+		return;
 	attrs->set = true;
 	attrs->flavour = flavour;
 	attrs->port_number = port_number;
 	attrs->split = split;
 	attrs->split_subport_number = split_subport_number;
-	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
 }
 EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
 
-- 
2.17.2

[patch net-next 11/11] net: devlink: add port type spinlock

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Add spinlock to protect port type and type_dev pointer consistency.
Without that, userspace may see inconsistent type and type_dev
combinations.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/devlink.h |  4 ++++
 net/core/devlink.c    | 17 +++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 63de99e09f04..cb9b060033e1 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -16,6 +16,7 @@
 #include <linux/gfp.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
+#include <linux/spinlock.h>
 #include <net/net_namespace.h>
 #include <uapi/linux/devlink.h>
 
@@ -53,6 +54,9 @@ struct devlink_port {
 	struct devlink *devlink;
 	unsigned index;
 	bool registered;
+	spinlock_t type_lock; /* Protects type and type_dev
+			       * pointer consistency.
+			       */
 	enum devlink_port_type type;
 	enum devlink_port_type desired_type;
 	void *type_dev;
diff --git a/net/core/devlink.c b/net/core/devlink.c
index f77a68f72d14..953e009c9162 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
+#include <linux/spinlock.h>
 #include <rdma/ib_verbs.h>
 #include <net/netlink.h>
 #include <net/genetlink.h>
@@ -543,12 +544,14 @@ static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
 		goto nla_put_failure;
 	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, devlink_port->index))
 		goto nla_put_failure;
+
+	spin_lock(&devlink_port->type_lock);
 	if (nla_put_u16(msg, DEVLINK_ATTR_PORT_TYPE, devlink_port->type))
-		goto nla_put_failure;
+		goto nla_put_failure_type_locked;
 	if (devlink_port->desired_type != DEVLINK_PORT_TYPE_NOTSET &&
 	    nla_put_u16(msg, DEVLINK_ATTR_PORT_DESIRED_TYPE,
 			devlink_port->desired_type))
-		goto nla_put_failure;
+		goto nla_put_failure_type_locked;
 	if (devlink_port->type == DEVLINK_PORT_TYPE_ETH) {
 		struct net_device *netdev = devlink_port->type_dev;
 
@@ -557,7 +560,7 @@ static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
 				 netdev->ifindex) ||
 		     nla_put_string(msg, DEVLINK_ATTR_PORT_NETDEV_NAME,
 				    netdev->name)))
-			goto nla_put_failure;
+			goto nla_put_failure_type_locked;
 	}
 	if (devlink_port->type == DEVLINK_PORT_TYPE_IB) {
 		struct ib_device *ibdev = devlink_port->type_dev;
@@ -565,14 +568,17 @@ static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
 		if (ibdev &&
 		    nla_put_string(msg, DEVLINK_ATTR_PORT_IBDEV_NAME,
 				   ibdev->name))
-			goto nla_put_failure;
+			goto nla_put_failure_type_locked;
 	}
+	spin_unlock(&devlink_port->type_lock);
 	if (devlink_nl_port_attrs_put(msg, devlink_port))
 		goto nla_put_failure;
 
 	genlmsg_end(msg, hdr);
 	return 0;
 
+nla_put_failure_type_locked:
+	spin_unlock(&devlink_port->type_lock);
 nla_put_failure:
 	genlmsg_cancel(msg, hdr);
 	return -EMSGSIZE;
@@ -5337,6 +5343,7 @@ int devlink_port_register(struct devlink *devlink,
 	devlink_port->devlink = devlink;
 	devlink_port->index = port_index;
 	devlink_port->registered = true;
+	spin_lock_init(&devlink_port->type_lock);
 	list_add_tail(&devlink_port->list, &devlink->port_list);
 	INIT_LIST_HEAD(&devlink_port->param_list);
 	mutex_unlock(&devlink->lock);
@@ -5365,8 +5372,10 @@ static void __devlink_port_type_set(struct devlink_port *devlink_port,
 				    enum devlink_port_type type,
 				    void *type_dev)
 {
+	spin_lock(&devlink_port->type_lock);
 	devlink_port->type = type;
 	devlink_port->type_dev = type_dev;
+	spin_unlock(&devlink_port->type_lock);
 	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
 }
 
-- 
2.17.2