| Message ID | 20210327223334.24655-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] mlxsw: spectrum_router: remove redundant initialization of variable force | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Sat, Mar 27, 2021 at 10:33:34PM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The variable force is being initialized with a value that is > never read and it is being updated later with a new value. The > initialization is redundant and can be removed. > > Addresses-Coverity: ("Unused value") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > index 6ccaa194733b..ff240e3c29f7 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp *mlxsw_sp, > { > u16 bucket_index = info->nh_res_bucket->bucket_index; > struct netlink_ext_ack *extack = info->extack; > - bool force = info->nh_res_bucket->force; > + bool force; > char ratr_pl_new[MLXSW_REG_RATR_LEN]; > char ratr_pl[MLXSW_REG_RATR_LEN]; > u32 adj_index; Reverse Christmas tree. regards, dan carpenter
On Sat, Mar 27, 2021 at 10:33:34PM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The variable force is being initialized with a value that is > never read and it is being updated later with a new value. The > initialization is redundant and can be removed. > > Addresses-Coverity: ("Unused value") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > index 6ccaa194733b..ff240e3c29f7 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp *mlxsw_sp, > { > u16 bucket_index = info->nh_res_bucket->bucket_index; > struct netlink_ext_ack *extack = info->extack; > - bool force = info->nh_res_bucket->force; > + bool force; Actually, there is a bug to be fixed here: ``` diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c index 6ccaa194733b..41259c0004d1 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c @@ -5068,8 +5068,9 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp *mlxsw_sp, /* No point in trying an atomic replacement if the idle timer interval * is smaller than the interval in which we query and clear activity. */ - force = info->nh_res_bucket->idle_timer_ms < - MLXSW_SP_NH_GRP_ACTIVITY_UPDATE_INTERVAL; + if (!force && info->nh_res_bucket->idle_timer_ms < + MLXSW_SP_NH_GRP_ACTIVITY_UPDATE_INTERVAL) + force = true; adj_index = nh->nhgi->adj_index + bucket_index; err = mlxsw_sp_nexthop_update(mlxsw_sp, adj_index, nh, force, ratr_pl); ``` We should only fallback to a non-atomic replacement when the current replacement is atomic and the idle timer is too short. We currently ignore the value of 'force'. This means that a non-atomic replacement ('force' is true) can be made atomic if idle timer is larger than 1 second. Colin, do you mind if I submit it formally as a fix later this week? I want to run it through our usual process. Will mention you in Reported-by, obviously. > char ratr_pl_new[MLXSW_REG_RATR_LEN]; > char ratr_pl[MLXSW_REG_RATR_LEN]; > u32 adj_index; > -- > 2.30.2 >
On 29/03/2021 08:13, Ido Schimmel wrote: > On Sat, Mar 27, 2021 at 10:33:34PM +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> The variable force is being initialized with a value that is >> never read and it is being updated later with a new value. The >> initialization is redundant and can be removed. >> >> Addresses-Coverity: ("Unused value") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >> index 6ccaa194733b..ff240e3c29f7 100644 >> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c >> @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp *mlxsw_sp, >> { >> u16 bucket_index = info->nh_res_bucket->bucket_index; >> struct netlink_ext_ack *extack = info->extack; >> - bool force = info->nh_res_bucket->force; >> + bool force; > > Actually, there is a bug to be fixed here: > > ``` > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > index 6ccaa194733b..41259c0004d1 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > @@ -5068,8 +5068,9 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp *mlxsw_sp, > /* No point in trying an atomic replacement if the idle timer interval > * is smaller than the interval in which we query and clear activity. > */ > - force = info->nh_res_bucket->idle_timer_ms < > - MLXSW_SP_NH_GRP_ACTIVITY_UPDATE_INTERVAL; > + if (!force && info->nh_res_bucket->idle_timer_ms < > + MLXSW_SP_NH_GRP_ACTIVITY_UPDATE_INTERVAL) > + force = true; > > adj_index = nh->nhgi->adj_index + bucket_index; > err = mlxsw_sp_nexthop_update(mlxsw_sp, adj_index, nh, force, ratr_pl); > ``` > > We should only fallback to a non-atomic replacement when the current > replacement is atomic and the idle timer is too short. > > We currently ignore the value of 'force'. This means that a non-atomic > replacement ('force' is true) can be made atomic if idle timer is larger > than 1 second. > > Colin, do you mind if I submit it formally as a fix later this week? I > want to run it through our usual process. Will mention you in > Reported-by, obviously. Sure. Good idea. > >> char ratr_pl_new[MLXSW_REG_RATR_LEN]; >> char ratr_pl[MLXSW_REG_RATR_LEN]; >> u32 adj_index; >> -- >> 2.30.2 >>
On Mon, 2021-03-29 at 09:04 +0300, Dan Carpenter wrote: > On Sat, Mar 27, 2021 at 10:33:34PM +0000, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > The variable force is being initialized with a value that is > > never read and it is being updated later with a new value. The > > initialization is redundant and can be removed. > > > > Addresses-Coverity: ("Unused value") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > > index 6ccaa194733b..ff240e3c29f7 100644 > > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c > > @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp *mlxsw_sp, > > { > > u16 bucket_index = info->nh_res_bucket->bucket_index; > > struct netlink_ext_ack *extack = info->extack; > > - bool force = info->nh_res_bucket->force; > > + bool force; > > char ratr_pl_new[MLXSW_REG_RATR_LEN]; > > char ratr_pl[MLXSW_REG_RATR_LEN]; > > u32 adj_index; > > Reverse Christmas tree. Is still terrible style.
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c index 6ccaa194733b..ff240e3c29f7 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct mlxsw_sp *mlxsw_sp, { u16 bucket_index = info->nh_res_bucket->bucket_index; struct netlink_ext_ack *extack = info->extack; - bool force = info->nh_res_bucket->force; + bool force; char ratr_pl_new[MLXSW_REG_RATR_LEN]; char ratr_pl[MLXSW_REG_RATR_LEN]; u32 adj_index;