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

[David Yang]

On Wed, Feb 25, 2026 at 10:17 PM Andrew Lunn <andrew@xxxxxxx> wrote:
...
> > +/* 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.
>

ldexpi is generic over integer types and we do type conversions in the
formulas below. So I think it cannot be expressed as a function.

> > +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.
>

The maxima are way larger than the physical limitations of GbE, so
they are also acceptable. Still it would be quite unusual for users to
supply such values.

> > +
> > +             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.
>

It means logical error during unit selection. Surely internal error
and the debugger must be familiar with the implementation details, I
think.

...