Re: [PATCH net-next 2/3] net: dsa: yt921x: Add port police support

[Andrew Lunn]

> +static void u32a_update_u64(u32 *arr, u64 mask, u64 val)
> +{
> +	arr[0] &= ~((u32)mask);
> +	arr[1] &= ~((u32)(mask >> 32));
> +	arr[0] |= (u32)val;
> +	arr[1] |= (u32)(val >> 32);
> +}

The name of this function is not helping me understand what it
actually does.

> +/* v * 2^e */
> +#define ldexpi(v, e) ({ \
> +	__auto_type _v = (v); \
> +	__auto_type _e = (e); \
> +	_e >= 0 ? _v << _e : _v >> -_e; \
> +})
> +
> +/* slot (ns) * rate (/s) / 10^9 (ns/s) = 2^C * token * 4^unit */
> +#define rate2token(rate, unit, slot_ns, C) \
> +	((u32)(ldexpi((slot_ns) * (rate), \
> +		      -(2 * (unit) + (C) + YT921X_TOKEN_RATE_C)) / 1000000000))
> +#define token2rate(token, unit, slot_ns, C) \
> +	(ldexpi(1000000000 * (u64)(token), \
> +		2 * (unit) + (C) + YT921X_TOKEN_RATE_C) / (slot_ns))
> +
> +/* burst = 2^C * token * 4^unit */
> +#define burst2token(burst, unit, C) ((u32)ldexpi((burst), -(2 * (unit) + (C))))
> +#define token2burst(token, unit, C) ldexpi((u64)(token), 2 * (unit) + (C))

Please avoid macros like this. Write functions. The compiler will
inline them if that makes sense. But you gain type checking, etc.

> +static struct yt921x_meter
> +yt921x_meter_tfm(struct yt921x_priv *priv, int port, unsigned int slot_ns,
> +		 u64 rate, u64 burst, unsigned int flags,
> +		 u32 cir_max, u32 cbs_max, int unit_max)
> +{
> +	const int C = flags & YT921X_METER_PKT_MODE ? YT921X_TOKEN_PKT_C :
> +		      YT921X_TOKEN_BYTE_C;
> +	struct device *dev = to_device(priv);
> +	struct yt921x_meter meter;
> +	bool distorted;
> +	u64 burst_est;
> +	u64 burst_max;
> +	u64 rate_max;
> +
> +	meter.unit = unit_max;
> +	rate_max = token2rate(cir_max, meter.unit, slot_ns, C);
> +	burst_max = token2burst(cbs_max, meter.unit, C);
> +
> +	/* Clamp rate and burst */
> +	if (rate > rate_max || burst > burst_max) {
> +		dev_warn(dev,
> +			 "rate %llu, burst %llu too high, max is %llu, %llu, clamping...\n",
> +			 rate, burst, rate_max, burst_max);

I'm not sure clamping is the correct thing to do here. -ERANGE would
make it clearer that what the user requests is not possible.

> +
> +		if (rate > rate_max)
> +			rate = rate_max;
> +		if (burst > burst_max)
> +			burst = burst_max;
> +
> +		burst_est = slot_ns * rate / 1000000000;
> +	} else {
> +		u64 burst_sug;
> +
> +		burst_est = slot_ns * rate / 1000000000;
> +		burst_sug = burst_est;
> +		if (flags & YT921X_METER_PKT_MODE)
> +			burst_sug++;
> +		else
> +			burst_sug += ETH_HLEN + yt921x_mtu_fetch(priv, port) +
> +				     ETH_FCS_LEN;
> +		if (burst_sug > burst)
> +			dev_warn(dev,
> +				 "Consider burst at least %llu to match rate %llu\n",
> +				 burst_sug, rate);
> +
> +		/* Select unit */
> +		for (; meter.unit > 0; meter.unit--) {
> +			if (rate > (rate_max >> 2) || burst > (burst_max >> 2))
> +				break;
> +			rate_max >>= 2;
> +			burst_max >>= 2;
> +		}
> +	}
> +
> +	/* Calculate information rate and bucket size */
> +	distorted = false;
> +	meter.cir = rate2token(rate, meter.unit, slot_ns, C);
> +	if (!meter.cir) {
> +		distorted = true;
> +		meter.cir = 1;
> +	} else if (meter.cir > cir_max) {
> +		WARN_ON(1);
> +		meter.cir = cir_max;

What does this WARN_ON() indicate? If this fires, you want somebody
who is debugging it to be able to understand it.

> +	}
> +	meter.cbs = burst2token(burst, meter.unit, C);
> +	if (!meter.cbs) {
> +		distorted = true;
> +		meter.cbs = 1;
> +	} else if (meter.cbs > cbs_max) {
> +		WARN_ON(1);
> +		meter.cbs = cbs_max;
> +	}
> +	if (distorted)
> +		dev_warn(dev,
> +			 "Have to increase rate %llu, burst %llu to %llu, %llu\n",
> +			 rate, burst,
> +			 token2rate(meter.cir, meter.unit, slot_ns, C),
> +			 token2burst(meter.cbs, meter.unit, C));

I see a difference between clamping, and the granularity the hardware
can do. Clamping should be an error. However, rounding to what the
hardware can actually do is expected, so i would not spam the logs
with it.

> +static int
> +yt921x_dsa_port_policer_add(struct dsa_switch *ds, int port,
> +			    const struct flow_action_police *policer)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	u32 ctrls[YT921X_METER_CTRL_S];
> +	struct yt921x_meter meter;
> +	bool pkt_mode;
> +	u64 burst;
> +	u64 rate;
> +	u32 ctrl;
> +	int res;
> +
> +	/* mtu defaults to unlimited but we got 2040 here, don't know why */
> +	if (policer->exceed.act_id != FLOW_ACTION_DROP ||
> +	    policer->notexceed.act_id != FLOW_ACTION_ACCEPT ||
> +	    policer->peakrate_bytes_ps || policer->avrate ||
> +	    policer->overhead) {
> +		dev_err(dev, "Unsupported options specified\n");

dev_dbg()

You might also want to extend .port_policer_add() to pass the extack
dsa_user_add_cls_matchall_police() has, so you can give a user
friendly error message.

	Andrew