Messages in this thread | | | Date | Thu, 2 May 2019 14:32:45 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next 2/3] net: mscc: ocelot: Implement port policers via tc command |
| |
Hi Joergen
> + > +#define MSCC_RC(expr) \ > + do { \ > + int __rc__ = (expr); \ > + if (__rc__ < 0) \ > + return __rc__; \ > + } \ > + while (0)
I'm sure checkpatch warned about this. A return inside a macros is a bad idea. I inherited code doing this, and broke it when adding locking, because it was not obvious there was a return.
> + > +/* The following two functions do the same as in iproute2 */ > +#define TIME_UNITS_PER_SEC 1000000 > +static unsigned int tc_core_tick2time(unsigned int tick) > +{ > + return (tick * (u32)PSCHED_TICKS2NS(1)) / 1000; > +} > + > +static unsigned int tc_calc_xmitsize(u64 rate, unsigned int ticks) > +{ > + return div_u64(rate * tc_core_tick2time(ticks), TIME_UNITS_PER_SEC); > +}
Should these but put somewhere others can use them?
> + > +enum mscc_qos_rate_mode { > + MSCC_QOS_RATE_MODE_DISABLED, /* Policer/shaper disabled */ > + MSCC_QOS_RATE_MODE_LINE, /* Measure line rate in kbps incl. IPG */ > + MSCC_QOS_RATE_MODE_DATA, /* Measures data rate in kbps excl. IPG */ > + MSCC_QOS_RATE_MODE_FRAME, /* Measures frame rate in fps */ > + __MSCC_QOS_RATE_MODE_END, > + NUM_MSCC_QOS_RATE_MODE = __MSCC_QOS_RATE_MODE_END, > + MSCC_QOS_RATE_MODE_MAX = __MSCC_QOS_RATE_MODE_END - 1, > +}; > + > +/* Round x divided by y to nearest integer. x and y are integers */ > +#define MSCC_ROUNDING_DIVISION(x, y) (((x) + ((y) / 2)) / (y))
linux/kernel.h defines DIV_ROUND_UP(). Maybe add DIV_ROUND_DOWN()?
> + > +/* Round x divided by y to nearest higher integer. x and y are integers */ > +#define MSCC_DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y))
DIV_ROUND_UP() ?
> + /* Limit to maximum values */ > + pir = min_t(u32, GENMASK(15, 0), pir); > + cir = min_t(u32, GENMASK(15, 0), cir); > + pbs = min_t(u32, pbs_max, pbs); > + cbs = min_t(u32, cbs_max, cbs);
If it does need to limit, maybe return -EOPNOTSUPP?
> +int ocelot_port_policer_add(struct ocelot_port *port, > + struct tcf_police *p) > +{ > + struct ocelot *ocelot = port->ocelot; > + struct qos_policer_conf pp; > + > + if (!p) > + return -EINVAL; > + > + netdev_dbg(port->dev, > + "result %d ewma_rate %u burst %lld mtu %u mtu_pktoks %lld\n", > + p->params->tcfp_result, > + p->params->tcfp_ewma_rate, > + p->params->tcfp_burst, > + p->params->tcfp_mtu, > + p->params->tcfp_mtu_ptoks); > + > + if (p->params->rate_present) > + netdev_dbg(port->dev, > + "rate: rate %llu mult %u over %u link %u shift %u\n", > + p->params->rate.rate_bytes_ps, > + p->params->rate.mult, > + p->params->rate.overhead, > + p->params->rate.linklayer, > + p->params->rate.shift); > + > + if (p->params->peak_present) > + netdev_dbg(port->dev, > + "peak: rate %llu mult %u over %u link %u shift %u\n", > + p->params->peak.rate_bytes_ps, > + p->params->peak.mult, > + p->params->peak.overhead, > + p->params->peak.linklayer, > + p->params->peak.shift); > + > + memset(&pp, 0, sizeof(pp));
Rather than memset, you can do:
struct qos_policer_conf pp = { 0 };
Andrew
| |