Re: [patch net-next RFC 07/10] mlxsw: convert driver to use unlocked devlink API during init/fini

[Ido Schimmel <idosch@nvidia.com>]

On Tue, Jul 12, 2022 at 01:05:08PM +0200, Jiri Pirko wrote:
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
> index ab1cebf227fb..b0267e4dca27 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
> @@ -127,11 +127,11 @@ static int mlxsw_core_resources_ports_register(struct mlxsw_core *mlxsw_core)
>  					  max_ports, 1,
>  					  DEVLINK_RESOURCE_UNIT_ENTRY);
>  
> -	return devlink_resource_register(devlink,
> -					 DEVLINK_RESOURCE_GENERIC_NAME_PORTS,
> -					 max_ports, MLXSW_CORE_RESOURCE_PORTS,
> -					 DEVLINK_RESOURCE_ID_PARENT_TOP,
> -					 &ports_num_params);
> +	return devl_resource_register(devlink,
> +				      DEVLINK_RESOURCE_GENERIC_NAME_PORTS,
> +				      max_ports, MLXSW_CORE_RESOURCE_PORTS,
> +				      DEVLINK_RESOURCE_ID_PARENT_TOP,
> +				      &ports_num_params);
>  }
>  
>  static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
> @@ -157,8 +157,8 @@ static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
>  			goto err_resources_ports_register;
>  	}
>  	atomic_set(&mlxsw_core->active_ports_count, 0);
> -	devlink_resource_occ_get_register(devlink, MLXSW_CORE_RESOURCE_PORTS,
> -					  mlxsw_ports_occ_get, mlxsw_core);
> +	devl_resource_occ_get_register(devlink, MLXSW_CORE_RESOURCE_PORTS,
> +				       mlxsw_ports_occ_get, mlxsw_core);
>  
>  	return 0;
>  
> @@ -171,9 +171,9 @@ static void mlxsw_ports_fini(struct mlxsw_core *mlxsw_core, bool reload)
>  {
>  	struct devlink *devlink = priv_to_devlink(mlxsw_core);
>  
> -	devlink_resource_occ_get_unregister(devlink, MLXSW_CORE_RESOURCE_PORTS);
> +	devl_resource_occ_get_unregister(devlink, MLXSW_CORE_RESOURCE_PORTS);
>  	if (!reload)
> -		devlink_resources_unregister(priv_to_devlink(mlxsw_core));
> +		devl_resources_unregister(priv_to_devlink(mlxsw_core));
>  
>  	kfree(mlxsw_core->ports);
>  }
> @@ -1485,10 +1485,12 @@ mlxsw_devlink_core_bus_device_reload_down(struct devlink *devlink,
>  {
>  	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>  
> +	devl_lock(devlink);
>  	if (!(mlxsw_core->bus->features & MLXSW_BUS_F_RESET))
>  		return -EOPNOTSUPP;

Not releasing the lock. You can take it after this check as these
features do not change

>  
>  	mlxsw_core_bus_device_unregister(mlxsw_core, true);
> +	devl_unlock(devlink);
>  	return 0;
>  }
>  
> @@ -1498,13 +1500,17 @@ mlxsw_devlink_core_bus_device_reload_up(struct devlink *devlink, enum devlink_re
>  					struct netlink_ext_ack *extack)
>  {
>  	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
> +	int err;
>  
> +	devl_lock(devlink);
>  	*actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT) |
>  			     BIT(DEVLINK_RELOAD_ACTION_FW_ACTIVATE);
> -	return mlxsw_core_bus_device_register(mlxsw_core->bus_info,
> -					      mlxsw_core->bus,
> -					      mlxsw_core->bus_priv, true,
> -					      devlink, extack);
> +	err = mlxsw_core_bus_device_register(mlxsw_core->bus_info,
> +					     mlxsw_core->bus,
> +					     mlxsw_core->bus_priv, true,
> +					     devlink, extack);
> +	devl_unlock(devlink);
> +	return err;
>  }
>  
>  static int mlxsw_devlink_flash_update(struct devlink *devlink,
> @@ -2102,6 +2108,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
>  			err = -ENOMEM;
>  			goto err_devlink_alloc;
>  		}
> +		devl_lock(devlink);

Why not just take it in mlxsw_core_bus_device_register() if '!reload' ?
Easier to read and also consistent with the change in
mlxsw_core_bus_device_unregister()


>  	}
>  
>  	mlxsw_core = devlink_priv(devlink);
> @@ -2188,6 +2195,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
>  	if (!reload) {
>  		devlink_set_features(devlink, DEVLINK_F_RELOAD);
>  		devlink_register(devlink);
> +		devl_unlock(devlink);

Did you check this with lockdep? devlink_register() now acquires the
global devlink mutex under the per-instance lock, but devlink core uses
the reverse order.

>  	}
>  	return 0;
>  
> @@ -2214,12 +2222,14 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
>  	mlxsw_ports_fini(mlxsw_core, reload);
>  err_ports_init:
>  	if (!reload)
> -		devlink_resources_unregister(devlink);
> +		devl_resources_unregister(devlink);
>  err_register_resources:
>  	mlxsw_bus->fini(bus_priv);
>  err_bus_init:
> -	if (!reload)
> +	if (!reload) {
> +		devl_unlock(devlink);
>  		devlink_free(devlink);
> +	}
>  err_devlink_alloc:
>  	return err;
>  }
> @@ -2255,8 +2265,10 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
>  {
>  	struct devlink *devlink = priv_to_devlink(mlxsw_core);
>  
> -	if (!reload)
> +	if (!reload) {
> +		devl_lock(devlink);
>  		devlink_unregister(devlink);
> +	}
>  
>  	if (devlink_is_reload_failed(devlink)) {
>  		if (!reload)
> @@ -2281,17 +2293,20 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
>  	kfree(mlxsw_core->lag.mapping);
>  	mlxsw_ports_fini(mlxsw_core, reload);
>  	if (!reload)
> -		devlink_resources_unregister(devlink);
> +		devl_resources_unregister(devlink);
>  	mlxsw_core->bus->fini(mlxsw_core->bus_priv);
> -	if (!reload)
> +	if (!reload) {
>  		devlink_free(devlink);
> +		devl_unlock(devlink);
> +	}
>  
>  	return;
>  
>  reload_fail_deinit:
>  	mlxsw_core_params_unregister(mlxsw_core);
> -	devlink_resources_unregister(devlink);
> +	devl_resources_unregister(devlink);
>  	devlink_free(devlink);
> +	devl_unlock(devlink);
>  }
>  EXPORT_SYMBOL(mlxsw_core_bus_device_unregister);