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

[Andrew Lunn]

On Wed, Feb 25, 2026 at 10:47:18PM +0800, David Yang wrote:
> 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.

Writing functions will then help makes the types clearer. Why are type
conversions needed? Making them explicit would make the code better.

> > > +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 they are unusual, then maybe ERANGE makes more sense, the user has
probably misunderstood the API, which can be quite easy with tc :-(

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

     } else if (WARN_ON(meter.cir > cir_max)) {
             meter.cir = cir_max;

would be more normal.

      Andrew