Re: [PATCH net-next 11/15] mlxsw: spectrum_trap: Add devlink-trap policer support

[Ido Schimmel <idosch@idosch.org>]

On Tue, Mar 24, 2020 at 08:33:49PM -0700, Jakub Kicinski wrote:
> On Tue, 24 Mar 2020 21:32:46 +0200 Ido Schimmel wrote:
> > +static int mlxsw_sp_trap_policer_params_check(u64 rate, u64 burst,
> > +					      u8 *p_burst_size,
> > +					      struct netlink_ext_ack *extack)
> > +{
> > +	int bs = fls64(burst);
> > +
> > +	if (rate < MLXSW_REG_QPCR_LOWEST_CIR) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Policer rate lower than limit");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (rate > MLXSW_REG_QPCR_HIGHEST_CIR) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Policer rate higher than limit");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!bs) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Policer burst size lower than limit");
> > +		return -EINVAL;
> > +	}
> > +
> > +	--bs;
> > +
> > +	if (burst != (1 << bs)) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Policer burst size is not power of two");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (bs < MLXSW_REG_QPCR_LOWEST_CBS) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Policer burst size lower than limit");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (bs > MLXSW_REG_QPCR_HIGHEST_CBS) {
> > +		NL_SET_ERR_MSG_MOD(extack, "Policer burst size higher than limit");
> > +		return -EINVAL;
> > +	}
> 
> Any chance we could make the min/max values part of the policer itself?
> Are they dynamic? Seems like most drivers will have to repeat this
> checking against constants while maybe core could have done it?

Yea, it's a good idea. We can also expose these values to user space,
but I think it's not really necessary and I prefer not to extend uAPI
unless we really have to.

> 
> > +
> > +	*p_burst_size = bs;
> > +
> > +	return 0;
> > +}