Re: [patch net] devlink: convert occ_get op to separate registration

[David Ahern <dsahern@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On 4/5/18 2:13 PM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> This resolves race during initialization where the resources with
> ops are registered before driver and the structures used by occ_get
> op is initialized. So keep occ_get callbacks registered only when
> all structs are initialized.
> 
> The example flows, as it is in mlxsw:
> 1) driver load/asic probe:
>    mlxsw_core
>       -> mlxsw_sp_resources_register
>         -> mlxsw_sp_kvdl_resources_register
>           -> devlink_resource_register IDX
>    mlxsw_spectrum
>       -> mlxsw_sp_kvdl_init
>         -> mlxsw_sp_kvdl_parts_init
>           -> mlxsw_sp_kvdl_part_init
>             -> devlink_resource_size_get IDX (to get the current setup
>                                               size from devlink)
>         -> devlink_resource_occ_get_register IDX (register current
>                                                   occupancy getter)
> 2) reload triggered by devlink command:
>   -> mlxsw_devlink_core_bus_device_reload
>     -> mlxsw_sp_fini
>       -> mlxsw_sp_kvdl_fini
> 	-> devlink_resource_occ_get_unregister IDX
>     (struct mlxsw_sp *mlxsw_sp is freed at this point, call to occ get
>      which is using mlxsw_sp would cause use-after free)
>     -> mlxsw_sp_init
>       -> mlxsw_sp_kvdl_init
>         -> mlxsw_sp_kvdl_parts_init
>           -> mlxsw_sp_kvdl_part_init
>             -> devlink_resource_size_get IDX (to get the current setup
>                                               size from devlink)
>         -> devlink_resource_occ_get_register IDX (register current
>                                                   occupancy getter)
> 
> Fixes: d9f9b9a4d05f ("devlink: Add support for resource abstraction")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---


Why won't something like the attached work as opposed to playing
register / unregister games?

[-- Attachment #2: mlxsw-reload.patch --]
[-- Type: text/plain, Size: 4748 bytes --]

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 93ea56620a24..dcded613faa6 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -113,6 +113,7 @@ struct mlxsw_core {
 	struct mlxsw_thermal *thermal;
 	struct mlxsw_core_port *ports;
 	unsigned int max_ports;
+	bool reload_in_progress;
 	bool reload_fail;
 	unsigned long driver_priv[0];
 	/* driver_priv has to be always the last item */
@@ -154,6 +155,12 @@ void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core)
 }
 EXPORT_SYMBOL(mlxsw_core_driver_priv);
 
+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core)
+{
+	return mlxsw_core->mlxsw_core_driver_priv;
+}
+EXPORT_SYMBOL(mlxsw_core_reload_in_progress);
+
 struct mlxsw_rx_listener_item {
 	struct list_head list;
 	struct mlxsw_rx_listener rxl;
@@ -972,12 +979,16 @@ static int mlxsw_devlink_core_bus_device_reload(struct devlink *devlink)
 	if (!mlxsw_bus->reset)
 		return -EOPNOTSUPP;
 
+	mlxsw_core->reload_in_progress = true;
+
 	mlxsw_core_bus_device_unregister(mlxsw_core, true);
 	mlxsw_bus->reset(mlxsw_core->bus_priv);
 	err = mlxsw_core_bus_device_register(mlxsw_core->bus_info,
 					     mlxsw_core->bus,
 					     mlxsw_core->bus_priv, true,
 					     devlink);
+	mlxsw_core->reload_in_progress = false;
+
 	if (err)
 		mlxsw_core->reload_fail = true;
 	return err;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index 092d39399f3c..ffa1db154757 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -60,6 +60,7 @@ struct mlxsw_bus_info;
 unsigned int mlxsw_core_max_ports(const struct mlxsw_core *mlxsw_core);
 
 void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core);
+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core);
 
 int mlxsw_core_driver_register(struct mlxsw_driver *mlxsw_driver);
 void mlxsw_core_driver_unregister(struct mlxsw_driver *mlxsw_driver);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 53fffd09d133..09b89af37d8a 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3808,8 +3808,12 @@ static const struct mlxsw_config_profile mlxsw_sp_config_profile = {
 static u64 mlxsw_sp_resource_kvd_linear_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+	struct mlxsw_sp *mlxsw_sp;
+
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
 
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	return mlxsw_sp_kvdl_occ_get(mlxsw_sp);
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
index 8796db44dcc3..dd66285bafb5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
@@ -329,9 +329,13 @@ u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
 static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	struct mlxsw_sp_kvdl_part *part;
+	struct mlxsw_sp *mlxsw_sp;
 
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
+
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_SINGLE];
 	return mlxsw_sp_kvdl_part_occ(part);
 }
@@ -339,8 +343,13 @@ static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
 static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	struct mlxsw_sp_kvdl_part *part;
+	struct mlxsw_sp *mlxsw_sp;
+
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
+
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_CHUNKS];
 	return mlxsw_sp_kvdl_part_occ(part);
@@ -349,9 +358,13 @@ static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
 static u64 mlxsw_sp_kvdl_large_chunks_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	struct mlxsw_sp_kvdl_part *part;
+	struct mlxsw_sp *mlxsw_sp;
+
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
 
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_LARGE_CHUNKS];
 	return mlxsw_sp_kvdl_part_occ(part);
 }