Re: [PATCH net-next 2/3] net: dsa: yt921x: Add port police support
From: Andrew Lunn <andrew@lunn.ch>
Date: 2026-02-25 14:17:58
Also in:
lkml
+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