On 3/29/18 2:33 PM, Ido Schimmel wrote: > From: Jiri Pirko <j...@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.
Why can't the occ_get handler look at some flag in an mlxsw struct to know if the system has initialized? Separate registration here is awkward. You register a resource and then register its op later. Also, this should be a standalone patch rather than embedded in a 'mlxsw: Various cleanups' set. > > Signed-off-by: Jiri Pirko <j...@mellanox.com> > Signed-off-by: Ido Schimmel <ido...@mellanox.com> > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 24 ++----- > drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 1 - > .../net/ethernet/mellanox/mlxsw/spectrum_kvdl.c | 67 ++++++++++++-------- > include/net/devlink.h | 40 ++++++++---- > net/core/devlink.c | 74 > +++++++++++++++++++--- > 5 files changed, 134 insertions(+), 72 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > index b831af38e0a1..0d95d2cb73e3 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c > @@ -3805,18 +3805,6 @@ 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); > - > - return mlxsw_sp_kvdl_occ_get(mlxsw_sp); > -} > - > -static const struct devlink_resource_ops mlxsw_sp_resource_kvd_linear_ops = { > - .occ_get = mlxsw_sp_resource_kvd_linear_occ_get, > -}; > - > static void > mlxsw_sp_resource_size_params_prepare(struct mlxsw_core *mlxsw_core, > struct devlink_resource_size_params > *kvd_size_params, > @@ -3877,8 +3865,7 @@ static int mlxsw_sp_resources_register(struct > mlxsw_core *mlxsw_core) > err = devlink_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD, > kvd_size, MLXSW_SP_RESOURCE_KVD, > DEVLINK_RESOURCE_ID_PARENT_TOP, > - &kvd_size_params, > - NULL); > + &kvd_size_params); > if (err) > return err; > > @@ -3887,8 +3874,7 @@ static int mlxsw_sp_resources_register(struct > mlxsw_core *mlxsw_core) > linear_size, > MLXSW_SP_RESOURCE_KVD_LINEAR, > MLXSW_SP_RESOURCE_KVD, > - &linear_size_params, > - &mlxsw_sp_resource_kvd_linear_ops); > + &linear_size_params); > if (err) > return err; > > @@ -3905,8 +3891,7 @@ static int mlxsw_sp_resources_register(struct > mlxsw_core *mlxsw_core) > double_size, > MLXSW_SP_RESOURCE_KVD_HASH_DOUBLE, > MLXSW_SP_RESOURCE_KVD, > - &hash_double_size_params, > - NULL); > + &hash_double_size_params); > if (err) > return err; > > @@ -3915,8 +3900,7 @@ static int mlxsw_sp_resources_register(struct > mlxsw_core *mlxsw_core) > single_size, > MLXSW_SP_RESOURCE_KVD_HASH_SINGLE, > MLXSW_SP_RESOURCE_KVD, > - &hash_single_size_params, > - NULL); > + &hash_single_size_params); > if (err) > return err; > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > index 21bee8f19894..c59a0d7d81d5 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > @@ -442,7 +442,6 @@ void mlxsw_sp_kvdl_free(struct mlxsw_sp *mlxsw_sp, int > entry_index); > int mlxsw_sp_kvdl_alloc_size_query(struct mlxsw_sp *mlxsw_sp, > unsigned int entry_count, > unsigned int *p_alloc_size); > -u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp); > int mlxsw_sp_kvdl_resources_register(struct devlink *devlink); > > struct mlxsw_sp_acl_rule_info { > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c > b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c > index 7b28f65d6407..1b7280168e6b 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c > @@ -315,8 +315,9 @@ static u64 mlxsw_sp_kvdl_part_occ(struct > mlxsw_sp_kvdl_part *part) > return occ; > } > > -u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp) > +static u64 mlxsw_sp_kvdl_occ_get(void *priv) > { > + const struct mlxsw_sp *mlxsw_sp = priv; > u64 occ = 0; > int i; > > @@ -326,48 +327,33 @@ u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp > *mlxsw_sp) > return occ; > } > > -static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink) > +static u64 mlxsw_sp_kvdl_single_occ_get(void *priv) > { > - struct mlxsw_core *mlxsw_core = devlink_priv(devlink); > - struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core); > + const struct mlxsw_sp *mlxsw_sp = priv; > struct mlxsw_sp_kvdl_part *part; > > part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_SINGLE]; > return mlxsw_sp_kvdl_part_occ(part); > } > > -static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink) > +static u64 mlxsw_sp_kvdl_chunks_occ_get(void *priv) > { > - struct mlxsw_core *mlxsw_core = devlink_priv(devlink); > - struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core); > + const struct mlxsw_sp *mlxsw_sp = priv; > struct mlxsw_sp_kvdl_part *part; > > part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_CHUNKS]; > return mlxsw_sp_kvdl_part_occ(part); > } > > -static u64 mlxsw_sp_kvdl_large_chunks_occ_get(struct devlink *devlink) > +static u64 mlxsw_sp_kvdl_large_chunks_occ_get(void *priv) > { > - struct mlxsw_core *mlxsw_core = devlink_priv(devlink); > - struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core); > + const struct mlxsw_sp *mlxsw_sp = priv; > struct mlxsw_sp_kvdl_part *part; > > part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_LARGE_CHUNKS]; > return mlxsw_sp_kvdl_part_occ(part); > } > > -static const struct devlink_resource_ops mlxsw_sp_kvdl_single_ops = { > - .occ_get = mlxsw_sp_kvdl_single_occ_get, > -}; > - > -static const struct devlink_resource_ops mlxsw_sp_kvdl_chunks_ops = { > - .occ_get = mlxsw_sp_kvdl_chunks_occ_get, > -}; > - > -static const struct devlink_resource_ops mlxsw_sp_kvdl_chunks_large_ops = { > - .occ_get = mlxsw_sp_kvdl_large_chunks_occ_get, > -}; > - > int mlxsw_sp_kvdl_resources_register(struct devlink *devlink) > { > struct mlxsw_core *mlxsw_core = devlink_priv(devlink); > @@ -386,8 +372,7 @@ int mlxsw_sp_kvdl_resources_register(struct devlink > *devlink) > MLXSW_SP_KVDL_SINGLE_SIZE, > MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE, > MLXSW_SP_RESOURCE_KVD_LINEAR, > - &size_params, > - &mlxsw_sp_kvdl_single_ops); > + &size_params); > if (err) > return err; > > @@ -398,8 +383,7 @@ int mlxsw_sp_kvdl_resources_register(struct devlink > *devlink) > MLXSW_SP_KVDL_CHUNKS_SIZE, > MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS, > MLXSW_SP_RESOURCE_KVD_LINEAR, > - &size_params, > - &mlxsw_sp_kvdl_chunks_ops); > + &size_params); > if (err) > return err; > > @@ -410,13 +394,13 @@ int mlxsw_sp_kvdl_resources_register(struct devlink > *devlink) > MLXSW_SP_KVDL_LARGE_CHUNKS_SIZE, > > MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS, > MLXSW_SP_RESOURCE_KVD_LINEAR, > - &size_params, > - &mlxsw_sp_kvdl_chunks_large_ops); > + &size_params); > return err; > } > > int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp) > { > + struct devlink *devlink = priv_to_devlink(mlxsw_sp->core); > struct mlxsw_sp_kvdl *kvdl; > int err; > > @@ -429,6 +413,23 @@ int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp) > if (err) > goto err_kvdl_parts_init; > > + devlink_resource_occ_get_register(devlink, > + MLXSW_SP_RESOURCE_KVD_LINEAR, > + mlxsw_sp_kvdl_occ_get, > + mlxsw_sp); > + devlink_resource_occ_get_register(devlink, > + MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE, > + mlxsw_sp_kvdl_single_occ_get, > + mlxsw_sp); > + devlink_resource_occ_get_register(devlink, > + MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS, > + mlxsw_sp_kvdl_chunks_occ_get, > + mlxsw_sp); > + devlink_resource_occ_get_register(devlink, > + > MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS, > + mlxsw_sp_kvdl_large_chunks_occ_get, > + mlxsw_sp); > + > return 0; > > err_kvdl_parts_init: > @@ -438,6 +439,16 @@ int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp) > > void mlxsw_sp_kvdl_fini(struct mlxsw_sp *mlxsw_sp) > { > + struct devlink *devlink = priv_to_devlink(mlxsw_sp->core); > + > + devlink_resource_occ_get_unregister(devlink, > + > MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS); > + devlink_resource_occ_get_unregister(devlink, > + > MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS); > + devlink_resource_occ_get_unregister(devlink, > + > MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE); > + devlink_resource_occ_get_unregister(devlink, > + MLXSW_SP_RESOURCE_KVD_LINEAR); > mlxsw_sp_kvdl_parts_fini(mlxsw_sp); > kfree(mlxsw_sp->kvdl); > } > diff --git a/include/net/devlink.h b/include/net/devlink.h > index e21d8cadd480..2e4f71e16e95 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -231,14 +231,6 @@ struct devlink_dpipe_headers { > unsigned int headers_count; > }; > > -/** > - * struct devlink_resource_ops - resource ops > - * @occ_get: get the occupied size > - */ > -struct devlink_resource_ops { > - u64 (*occ_get)(struct devlink *devlink); > -}; > - > /** > * struct devlink_resource_size_params - resource's size parameters > * @size_min: minimum size which can be set > @@ -265,6 +257,8 @@ devlink_resource_size_params_init(struct > devlink_resource_size_params *size_para > size_params->unit = unit; > } > > +typedef u64 devlink_resource_occ_get_t(void *priv); > + > /** > * struct devlink_resource - devlink resource > * @name: name of the resource > @@ -277,7 +271,6 @@ devlink_resource_size_params_init(struct > devlink_resource_size_params *size_para > * @size_params: size parameters > * @list: parent list > * @resource_list: list of child resources > - * @resource_ops: resource ops > */ > struct devlink_resource { > const char *name; > @@ -289,7 +282,8 @@ struct devlink_resource { > struct devlink_resource_size_params size_params; > struct list_head list; > struct list_head resource_list; > - const struct devlink_resource_ops *resource_ops; > + devlink_resource_occ_get_t *occ_get; > + void *occ_get_priv; > }; > > #define DEVLINK_RESOURCE_ID_PARENT_TOP 0 > @@ -409,8 +403,7 @@ int devlink_resource_register(struct devlink *devlink, > u64 resource_size, > u64 resource_id, > u64 parent_resource_id, > - const struct devlink_resource_size_params > *size_params, > - const struct devlink_resource_ops *resource_ops); > + const struct devlink_resource_size_params > *size_params); > void devlink_resources_unregister(struct devlink *devlink, > struct devlink_resource *resource); > int devlink_resource_size_get(struct devlink *devlink, > @@ -419,6 +412,12 @@ int devlink_resource_size_get(struct devlink *devlink, > int devlink_dpipe_table_resource_set(struct devlink *devlink, > const char *table_name, u64 resource_id, > u64 resource_units); > +void devlink_resource_occ_get_register(struct devlink *devlink, > + u64 resource_id, > + devlink_resource_occ_get_t *occ_get, > + void *occ_get_priv); > +void devlink_resource_occ_get_unregister(struct devlink *devlink, > + u64 resource_id); > > #else > > @@ -562,8 +561,7 @@ devlink_resource_register(struct devlink *devlink, > u64 resource_size, > u64 resource_id, > u64 parent_resource_id, > - const struct devlink_resource_size_params > *size_params, > - const struct devlink_resource_ops *resource_ops) > + const struct devlink_resource_size_params > *size_params) > { > return 0; > } > @@ -589,6 +587,20 @@ devlink_dpipe_table_resource_set(struct devlink *devlink, > return -EOPNOTSUPP; > } > > +static inline void > +devlink_resource_occ_get_register(struct devlink *devlink, > + u64 resource_id, > + devlink_resource_occ_get_t *occ_get, > + void *occ_get_priv) > +{ > +} > + > +static inline void > +devlink_resource_occ_get_unregister(struct devlink *devlink, > + u64 resource_id) > +{ > +} > + > #endif > > #endif /* _NET_DEVLINK_H_ */ > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 9236e421bd62..ad1317376798 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -2405,6 +2405,16 @@ devlink_resource_size_params_put(struct > devlink_resource *resource, > return 0; > } > > +static int devlink_resource_occ_put(struct devlink_resource *resource, > + struct sk_buff *skb) > +{ > + if (!resource->occ_get) > + return 0; > + return nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC, > + resource->occ_get(resource->occ_get_priv), > + DEVLINK_ATTR_PAD); > +} > + > static int devlink_resource_put(struct devlink *devlink, struct sk_buff *skb, > struct devlink_resource *resource) > { > @@ -2425,11 +2435,8 @@ static int devlink_resource_put(struct devlink > *devlink, struct sk_buff *skb, > if (resource->size != resource->size_new) > nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_SIZE_NEW, > resource->size_new, DEVLINK_ATTR_PAD); > - if (resource->resource_ops && resource->resource_ops->occ_get) > - if (nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC, > - resource->resource_ops->occ_get(devlink), > - DEVLINK_ATTR_PAD)) > - goto nla_put_failure; > + if (devlink_resource_occ_put(resource, skb)) > + goto nla_put_failure; > if (devlink_resource_size_params_put(resource, skb)) > goto nla_put_failure; > if (list_empty(&resource->resource_list)) > @@ -3162,15 +3169,13 @@ EXPORT_SYMBOL_GPL(devlink_dpipe_table_unregister); > * @resource_id: resource's id > * @parent_reosurce_id: resource's parent id > * @size params: size parameters > - * @resource_ops: resource ops > */ > int devlink_resource_register(struct devlink *devlink, > const char *resource_name, > u64 resource_size, > u64 resource_id, > u64 parent_resource_id, > - const struct devlink_resource_size_params > *size_params, > - const struct devlink_resource_ops *resource_ops) > + const struct devlink_resource_size_params > *size_params) > { > struct devlink_resource *resource; > struct list_head *resource_list; > @@ -3213,7 +3218,6 @@ int devlink_resource_register(struct devlink *devlink, > resource->size = resource_size; > resource->size_new = resource_size; > resource->id = resource_id; > - resource->resource_ops = resource_ops; > resource->size_valid = true; > memcpy(&resource->size_params, size_params, > sizeof(resource->size_params)); > @@ -3315,6 +3319,58 @@ int devlink_dpipe_table_resource_set(struct devlink > *devlink, > } > EXPORT_SYMBOL_GPL(devlink_dpipe_table_resource_set); > > +/** > + * devlink_resource_occ_get_register - register occupancy getter > + * > + * @devlink: devlink > + * @resource_id: resource id > + * @occ_get: occupancy getter callback > + * @occ_get_priv: occupancy getter callback priv > + */ > +void devlink_resource_occ_get_register(struct devlink *devlink, > + u64 resource_id, > + devlink_resource_occ_get_t *occ_get, > + void *occ_get_priv) > +{ > + struct devlink_resource *resource; > + > + mutex_lock(&devlink->lock); > + resource = devlink_resource_find(devlink, NULL, resource_id); > + if (WARN_ON(!resource)) > + goto out; > + WARN_ON(resource->occ_get); > + > + resource->occ_get = occ_get; > + resource->occ_get_priv = occ_get_priv; > +out: > + mutex_unlock(&devlink->lock); > +} > +EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register); > + > +/** > + * devlink_resource_occ_get_unregister - unregister occupancy getter > + * > + * @devlink: devlink > + * @resource_id: resource id > + */ > +void devlink_resource_occ_get_unregister(struct devlink *devlink, > + u64 resource_id) > +{ > + struct devlink_resource *resource; > + > + mutex_lock(&devlink->lock); > + resource = devlink_resource_find(devlink, NULL, resource_id); > + if (WARN_ON(!resource)) > + goto out; > + WARN_ON(!resource->occ_get); > + > + resource->occ_get = NULL; > + resource->occ_get_priv = NULL; > +out: > + mutex_unlock(&devlink->lock); > +} > +EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister); > + > static int __init devlink_module_init(void) > { > return genl_register_family(&devlink_nl_family); >