Re: [PATCH net-next 03/11] mlxsw: spectrum_policer: Add policer core

[Ido Schimmel <idosch@idosch.org>]

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