Message ID | 20200326183718.2384349-2-jacob.e.keller@intel.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | implement DEVLINK_CMD_REGION_NEW | expand |
On 2020/3/27 2:37, Jacob Keller wrote: > Modify the devlink region code in preparation for adding new operations > on regions. > > Create a devlink_region_ops structure, and move the name pointer from > within the devlink_region structure into the ops structure (similar to > the devlink_health_reporter_ops). > > This prepares the regions to enable support of additional operations in > the future such as requesting snapshots, or accessing the region > directly without a snapshot. > > In order to re-use the constant strings in the mlx4 driver their > declaration must be changed to 'const char * const' to ensure the > compiler realizes that both the data and the pointer cannot change. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Reviewed-by: Jakub Kicinski <kuba@kernel.org> > Reviewed-by: Jiri Pirko <jiri@mellanox.com> > --- > Changes since RFC > * Picked up Jiri's Reviewed-by > > drivers/net/ethernet/mellanox/mlx4/crdump.c | 16 +++++++++++---- > drivers/net/netdevsim/dev.c | 6 +++++- > include/net/devlink.h | 16 +++++++++++---- > net/core/devlink.c | 22 ++++++++++----------- > 4 files changed, 40 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c > index 64ed725aec28..cc2bf596c74b 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/crdump.c > +++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c > @@ -38,8 +38,16 @@ > #define CR_ENABLE_BIT_OFFSET 0xF3F04 > #define MAX_NUM_OF_DUMPS_TO_STORE (8) > > -static const char *region_cr_space_str = "cr-space"; > -static const char *region_fw_health_str = "fw-health"; > +static const char * const region_cr_space_str = "cr-space"; > +static const char * const region_fw_health_str = "fw-health"; > + > +static const struct devlink_region_ops region_cr_space_ops = { > + .name = region_cr_space_str, > +}; > + > +static const struct devlink_region_ops region_fw_health_ops = { > + .name = region_fw_health_str, > +}; > Hi, Jacob. After pull net-next, I get below compiler errors: drivers/net/ethernet/mellanox//mlx4/crdump.c:45:10: error: initializer element is not constant .name = region_cr_space_str, ^ drivers/net/ethernet/mellanox//mlx4/crdump.c:45:10: note: (near initialization for ‘region_cr_space_ops.name’) drivers/net/ethernet/mellanox//mlx4/crdump.c:50:10: error: initializer element is not constant .name = region_fw_health_str, It seems the value of variables region_cr_space_str and region_cr_space_str is unknown during compiling phase. Huazhong. > /* Set to true in case cr enable bit was set to true before crdump */ > static bool crdump_enbale_bit_set; > @@ -205,7 +213,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev) > /* Create cr-space region */ > crdump->region_crspace = > devlink_region_create(devlink, > - region_cr_space_str, > + ®ion_cr_space_ops, > MAX_NUM_OF_DUMPS_TO_STORE, > pci_resource_len(pdev, 0)); > if (IS_ERR(crdump->region_crspace)) > @@ -216,7 +224,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev) > /* Create fw-health region */ > crdump->region_fw_health = > devlink_region_create(devlink, > - region_fw_health_str, > + ®ion_fw_health_ops, > MAX_NUM_OF_DUMPS_TO_STORE, > HEALTH_BUFFER_SIZE); > if (IS_ERR(crdump->region_fw_health)) > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > index 7bfd0622cef1..47a8f8c570c4 100644 > --- a/drivers/net/netdevsim/dev.c > +++ b/drivers/net/netdevsim/dev.c > @@ -340,11 +340,15 @@ static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink) > > #define NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX 16 > > +static const struct devlink_region_ops dummy_region_ops = { > + .name = "dummy", > +}; > + > static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev, > struct devlink *devlink) > { > nsim_dev->dummy_region = > - devlink_region_create(devlink, "dummy", > + devlink_region_create(devlink, &dummy_region_ops, > NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX, > NSIM_DEV_DUMMY_REGION_SIZE); > return PTR_ERR_OR_ZERO(nsim_dev->dummy_region); > diff --git a/include/net/devlink.h b/include/net/devlink.h > index 37230e23b5b0..85db5dd5184d 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -498,6 +498,14 @@ struct devlink_info_req; > > typedef void devlink_snapshot_data_dest_t(const void *data); > > +/** > + * struct devlink_region_ops - Region operations > + * @name: region name > + */ > +struct devlink_region_ops { > + const char *name; > +}; > + > struct devlink_fmsg; > struct devlink_health_reporter; > > @@ -963,10 +971,10 @@ void devlink_port_param_value_changed(struct devlink_port *devlink_port, > u32 param_id); > void devlink_param_value_str_fill(union devlink_param_value *dst_val, > const char *src); > -struct devlink_region *devlink_region_create(struct devlink *devlink, > - const char *region_name, > - u32 region_max_snapshots, > - u64 region_size); > +struct devlink_region * > +devlink_region_create(struct devlink *devlink, > + const struct devlink_region_ops *ops, > + u32 region_max_snapshots, u64 region_size); > void devlink_region_destroy(struct devlink_region *region); > u32 devlink_region_snapshot_id_get(struct devlink *devlink); > int devlink_region_snapshot_create(struct devlink_region *region, > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 73bb8fbe3393..ca5362530567 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -344,7 +344,7 @@ devlink_sb_tc_index_get_from_info(struct devlink_sb *devlink_sb, > struct devlink_region { > struct devlink *devlink; > struct list_head list; > - const char *name; > + const struct devlink_region_ops *ops; > struct list_head snapshot_list; > u32 max_snapshots; > u32 cur_snapshots; > @@ -365,7 +365,7 @@ devlink_region_get_by_name(struct devlink *devlink, const char *region_name) > struct devlink_region *region; > > list_for_each_entry(region, &devlink->region_list, list) > - if (!strcmp(region->name, region_name)) > + if (!strcmp(region->ops->name, region_name)) > return region; > > return NULL; > @@ -3695,7 +3695,7 @@ static int devlink_nl_region_fill(struct sk_buff *msg, struct devlink *devlink, > if (err) > goto nla_put_failure; > > - err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->name); > + err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops->name); > if (err) > goto nla_put_failure; > > @@ -3741,7 +3741,7 @@ static void devlink_nl_region_notify(struct devlink_region *region, > goto out_cancel_msg; > > err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, > - region->name); > + region->ops->name); > if (err) > goto out_cancel_msg; > > @@ -7647,21 +7647,21 @@ EXPORT_SYMBOL_GPL(devlink_param_value_str_fill); > * devlink_region_create - create a new address region > * > * @devlink: devlink > - * @region_name: region name > + * @ops: region operations and name > * @region_max_snapshots: Maximum supported number of snapshots for region > * @region_size: size of region > */ > -struct devlink_region *devlink_region_create(struct devlink *devlink, > - const char *region_name, > - u32 region_max_snapshots, > - u64 region_size) > +struct devlink_region * > +devlink_region_create(struct devlink *devlink, > + const struct devlink_region_ops *ops, > + u32 region_max_snapshots, u64 region_size) > { > struct devlink_region *region; > int err = 0; > > mutex_lock(&devlink->lock); > > - if (devlink_region_get_by_name(devlink, region_name)) { > + if (devlink_region_get_by_name(devlink, ops->name)) { > err = -EEXIST; > goto unlock; > } > @@ -7674,7 +7674,7 @@ struct devlink_region *devlink_region_create(struct devlink *devlink, > > region->devlink = devlink; > region->max_snapshots = region_max_snapshots; > - region->name = region_name; > + region->ops = ops; > region->size = region_size; > INIT_LIST_HEAD(®ion->snapshot_list); > list_add_tail(®ion->list, &devlink->region_list); >
On 3/27/2020 1:16 AM, tanhuazhong wrote: > > > On 2020/3/27 2:37, Jacob Keller wrote: >> Modify the devlink region code in preparation for adding new operations >> on regions. >> >> Create a devlink_region_ops structure, and move the name pointer from >> within the devlink_region structure into the ops structure (similar to >> the devlink_health_reporter_ops). >> >> This prepares the regions to enable support of additional operations in >> the future such as requesting snapshots, or accessing the region >> directly without a snapshot. >> >> In order to re-use the constant strings in the mlx4 driver their >> declaration must be changed to 'const char * const' to ensure the >> compiler realizes that both the data and the pointer cannot change. >> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> Reviewed-by: Jakub Kicinski <kuba@kernel.org> >> Reviewed-by: Jiri Pirko <jiri@mellanox.com> >> --- >> Changes since RFC >> * Picked up Jiri's Reviewed-by >> >> drivers/net/ethernet/mellanox/mlx4/crdump.c | 16 +++++++++++---- >> drivers/net/netdevsim/dev.c | 6 +++++- >> include/net/devlink.h | 16 +++++++++++---- >> net/core/devlink.c | 22 ++++++++++----------- >> 4 files changed, 40 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c >> index 64ed725aec28..cc2bf596c74b 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/crdump.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c >> @@ -38,8 +38,16 @@ >> #define CR_ENABLE_BIT_OFFSET 0xF3F04 >> #define MAX_NUM_OF_DUMPS_TO_STORE (8) >> >> -static const char *region_cr_space_str = "cr-space"; >> -static const char *region_fw_health_str = "fw-health"; >> +static const char * const region_cr_space_str = "cr-space"; >> +static const char * const region_fw_health_str = "fw-health"; >> + >> +static const struct devlink_region_ops region_cr_space_ops = { >> + .name = region_cr_space_str, >> +}; >> + >> +static const struct devlink_region_ops region_fw_health_ops = { >> + .name = region_fw_health_str, >> +}; >> > > > Hi, Jacob. > > After pull net-next, I get below compiler errors: > drivers/net/ethernet/mellanox//mlx4/crdump.c:45:10: error: initializer > element is not constant > .name = region_cr_space_str, > ^ > drivers/net/ethernet/mellanox//mlx4/crdump.c:45:10: note: (near > initialization for ‘region_cr_space_ops.name’) > drivers/net/ethernet/mellanox//mlx4/crdump.c:50:10: error: initializer > element is not constant > .name = region_fw_health_str, > > It seems the value of variables region_cr_space_str and > region_cr_space_str is unknown during compiling phase. > > Huazhong. > What compiler are you using? I had gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) and switching from "const *" to "const * const" it worked. I'm wondering if this is only true in a more recent GCC. We can fix it by using a macro instead, or maybe there's some other trick we can use to get the compiler to realize the variable is truly constant. Thanks, Jake
On 3/27/2020 1:16 AM, tanhuazhong wrote: > > > On 2020/3/27 2:37, Jacob Keller wrote: >> Modify the devlink region code in preparation for adding new operations >> on regions. >> >> Create a devlink_region_ops structure, and move the name pointer from >> within the devlink_region structure into the ops structure (similar to >> the devlink_health_reporter_ops). >> >> This prepares the regions to enable support of additional operations in >> the future such as requesting snapshots, or accessing the region >> directly without a snapshot. >> >> In order to re-use the constant strings in the mlx4 driver their >> declaration must be changed to 'const char * const' to ensure the >> compiler realizes that both the data and the pointer cannot change. >> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> Reviewed-by: Jakub Kicinski <kuba@kernel.org> >> Reviewed-by: Jiri Pirko <jiri@mellanox.com> >> --- >> Changes since RFC >> * Picked up Jiri's Reviewed-by >> >> drivers/net/ethernet/mellanox/mlx4/crdump.c | 16 +++++++++++---- >> drivers/net/netdevsim/dev.c | 6 +++++- >> include/net/devlink.h | 16 +++++++++++---- >> net/core/devlink.c | 22 ++++++++++----------- >> 4 files changed, 40 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c >> index 64ed725aec28..cc2bf596c74b 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/crdump.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c >> @@ -38,8 +38,16 @@ >> #define CR_ENABLE_BIT_OFFSET 0xF3F04 >> #define MAX_NUM_OF_DUMPS_TO_STORE (8) >> >> -static const char *region_cr_space_str = "cr-space"; >> -static const char *region_fw_health_str = "fw-health"; >> +static const char * const region_cr_space_str = "cr-space"; >> +static const char * const region_fw_health_str = "fw-health"; >> + >> +static const struct devlink_region_ops region_cr_space_ops = { >> + .name = region_cr_space_str, >> +}; >> + >> +static const struct devlink_region_ops region_fw_health_ops = { >> + .name = region_fw_health_str, >> +}; >> > > > Hi, Jacob. > > After pull net-next, I get below compiler errors: > drivers/net/ethernet/mellanox//mlx4/crdump.c:45:10: error: initializer > element is not constant > .name = region_cr_space_str, > ^ > drivers/net/ethernet/mellanox//mlx4/crdump.c:45:10: note: (near > initialization for ‘region_cr_space_ops.name’) > drivers/net/ethernet/mellanox//mlx4/crdump.c:50:10: error: initializer > element is not constant > .name = region_fw_health_str, > > It seems the value of variables region_cr_space_str and > region_cr_space_str is unknown during compiling phase. > > Huazhong. > Based on some searching, this appears to be something where newer versions of GCC and clang can recognize that "const char * const" is safe to use as a constant expression, but older compilers cannot. I'm going to submit a fix that uses a macro so that we avoid this breakage on slightly older compilers. Thanks, Jake
diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c index 64ed725aec28..cc2bf596c74b 100644 --- a/drivers/net/ethernet/mellanox/mlx4/crdump.c +++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c @@ -38,8 +38,16 @@ #define CR_ENABLE_BIT_OFFSET 0xF3F04 #define MAX_NUM_OF_DUMPS_TO_STORE (8) -static const char *region_cr_space_str = "cr-space"; -static const char *region_fw_health_str = "fw-health"; +static const char * const region_cr_space_str = "cr-space"; +static const char * const region_fw_health_str = "fw-health"; + +static const struct devlink_region_ops region_cr_space_ops = { + .name = region_cr_space_str, +}; + +static const struct devlink_region_ops region_fw_health_ops = { + .name = region_fw_health_str, +}; /* Set to true in case cr enable bit was set to true before crdump */ static bool crdump_enbale_bit_set; @@ -205,7 +213,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev) /* Create cr-space region */ crdump->region_crspace = devlink_region_create(devlink, - region_cr_space_str, + ®ion_cr_space_ops, MAX_NUM_OF_DUMPS_TO_STORE, pci_resource_len(pdev, 0)); if (IS_ERR(crdump->region_crspace)) @@ -216,7 +224,7 @@ int mlx4_crdump_init(struct mlx4_dev *dev) /* Create fw-health region */ crdump->region_fw_health = devlink_region_create(devlink, - region_fw_health_str, + ®ion_fw_health_ops, MAX_NUM_OF_DUMPS_TO_STORE, HEALTH_BUFFER_SIZE); if (IS_ERR(crdump->region_fw_health)) diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 7bfd0622cef1..47a8f8c570c4 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -340,11 +340,15 @@ static void nsim_devlink_param_load_driverinit_values(struct devlink *devlink) #define NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX 16 +static const struct devlink_region_ops dummy_region_ops = { + .name = "dummy", +}; + static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev, struct devlink *devlink) { nsim_dev->dummy_region = - devlink_region_create(devlink, "dummy", + devlink_region_create(devlink, &dummy_region_ops, NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX, NSIM_DEV_DUMMY_REGION_SIZE); return PTR_ERR_OR_ZERO(nsim_dev->dummy_region); diff --git a/include/net/devlink.h b/include/net/devlink.h index 37230e23b5b0..85db5dd5184d 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -498,6 +498,14 @@ struct devlink_info_req; typedef void devlink_snapshot_data_dest_t(const void *data); +/** + * struct devlink_region_ops - Region operations + * @name: region name + */ +struct devlink_region_ops { + const char *name; +}; + struct devlink_fmsg; struct devlink_health_reporter; @@ -963,10 +971,10 @@ void devlink_port_param_value_changed(struct devlink_port *devlink_port, u32 param_id); void devlink_param_value_str_fill(union devlink_param_value *dst_val, const char *src); -struct devlink_region *devlink_region_create(struct devlink *devlink, - const char *region_name, - u32 region_max_snapshots, - u64 region_size); +struct devlink_region * +devlink_region_create(struct devlink *devlink, + const struct devlink_region_ops *ops, + u32 region_max_snapshots, u64 region_size); void devlink_region_destroy(struct devlink_region *region); u32 devlink_region_snapshot_id_get(struct devlink *devlink); int devlink_region_snapshot_create(struct devlink_region *region, diff --git a/net/core/devlink.c b/net/core/devlink.c index 73bb8fbe3393..ca5362530567 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -344,7 +344,7 @@ devlink_sb_tc_index_get_from_info(struct devlink_sb *devlink_sb, struct devlink_region { struct devlink *devlink; struct list_head list; - const char *name; + const struct devlink_region_ops *ops; struct list_head snapshot_list; u32 max_snapshots; u32 cur_snapshots; @@ -365,7 +365,7 @@ devlink_region_get_by_name(struct devlink *devlink, const char *region_name) struct devlink_region *region; list_for_each_entry(region, &devlink->region_list, list) - if (!strcmp(region->name, region_name)) + if (!strcmp(region->ops->name, region_name)) return region; return NULL; @@ -3695,7 +3695,7 @@ static int devlink_nl_region_fill(struct sk_buff *msg, struct devlink *devlink, if (err) goto nla_put_failure; - err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->name); + err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, region->ops->name); if (err) goto nla_put_failure; @@ -3741,7 +3741,7 @@ static void devlink_nl_region_notify(struct devlink_region *region, goto out_cancel_msg; err = nla_put_string(msg, DEVLINK_ATTR_REGION_NAME, - region->name); + region->ops->name); if (err) goto out_cancel_msg; @@ -7647,21 +7647,21 @@ EXPORT_SYMBOL_GPL(devlink_param_value_str_fill); * devlink_region_create - create a new address region * * @devlink: devlink - * @region_name: region name + * @ops: region operations and name * @region_max_snapshots: Maximum supported number of snapshots for region * @region_size: size of region */ -struct devlink_region *devlink_region_create(struct devlink *devlink, - const char *region_name, - u32 region_max_snapshots, - u64 region_size) +struct devlink_region * +devlink_region_create(struct devlink *devlink, + const struct devlink_region_ops *ops, + u32 region_max_snapshots, u64 region_size) { struct devlink_region *region; int err = 0; mutex_lock(&devlink->lock); - if (devlink_region_get_by_name(devlink, region_name)) { + if (devlink_region_get_by_name(devlink, ops->name)) { err = -EEXIST; goto unlock; } @@ -7674,7 +7674,7 @@ struct devlink_region *devlink_region_create(struct devlink *devlink, region->devlink = devlink; region->max_snapshots = region_max_snapshots; - region->name = region_name; + region->ops = ops; region->size = region_size; INIT_LIST_HEAD(®ion->snapshot_list); list_add_tail(®ion->list, &devlink->region_list);