On Mon, Aug 17, 2020 at 10:38:24AM -0500, Bjorn Helgaas wrote:
> You've likely seen this already, but Coverity found this problem:
>
> *** CID 1466147: Control flow issues (DEADCODE)
> /drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c: 380 in
> mlxsw_sp_policers_init()
> 374 }
> 375
> 376 return 0;
> 377
> 378 err_family_register:
> 379 for (i--; i >= 0; i--) {
> >>> CID 1466147: Control flow issues (DEADCODE)
> >>> Execution cannot reach this statement: "struct
> mlxsw_sp_policer_fam...".
> 380 struct mlxsw_sp_policer_family *family;
> 381
> 382 family = mlxsw_sp->policer_core->family_arr[i];
> 383 mlxsw_sp_policer_family_unregister(mlxsw_sp, family);
> 384 }
> 385 err_init:
>
> I think the problem is that MLXSW_SP_POLICER_TYPE_MAX is 0 because
>
> > +enum mlxsw_sp_policer_type {
> > + MLXSW_SP_POLICER_TYPE_SINGLE_RATE,
> > +
> > + __MLXSW_SP_POLICER_TYPE_MAX,
> > + MLXSW_SP_POLICER_TYPE_MAX = __MLXSW_SP_POLICER_TYPE_MAX - 1,
> > +};
>
> so we can only execute the family_register loop once, with i == 0,
> and if we get to err_family_register via the error exit:
>
> > + for (i = 0; i < MLXSW_SP_POLICER_TYPE_MAX + 1; i++) {
> > + err = mlxsw_sp_policer_family_register(mlxsw_sp,
> > mlxsw_sp_policer_family_arr[i]);
> > + if (err)
> > + goto err_family_register;
>
> i will be 0, so i-- sets i to -1, so we don't enter the
> family_unregister loop body since -1 is not >= 0.
Thanks for the report, but isn't the code doing the right thing here? I
mean, it's dead code now, but as soon as we add another family it will
be executed. It seems error prone to remove it only to please Coverity
and then add it back when it's actually needed.
>
> This code is now upstream as 8d3fbae70d8d ("mlxsw: spectrum_policer:
> Add policer core").
>
> Bjorn
