Re: [PATCH net-next v3 4/5] net: sparx5: Add hardware programming for the port policer

[Daniel Machon]

Hi Mohsin,

Thanks for reviewing.

> 
> 
> > +int sparx5_add_port_policer(struct sparx5_mall_entry *entry,
> > +			    struct netlink_ext_ack *extack)
> > +{
> > +	struct flow_action_police *police = &entry->port_policer.police;
> > +	struct sparx5_port *port = entry->port;
> > +	struct sparx5 *sparx5 = port->sparx5;
> > +	struct sparx5_policer pol = { 0 };
> > +	int idx, err;
> > +
> > +	if (!entry->ingress) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer is not supported on egress");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (entry->port->tc.ingress_block_shared) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer is not supported on shared ingress blocks");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (police->exceed.act_id != FLOW_ACTION_DROP) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer parameters are not supported");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (police->notexceed.act_id != FLOW_ACTION_PIPE &&
> > +	    police->notexceed.act_id != FLOW_ACTION_ACCEPT) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer parameters are not supported");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (police->peakrate_bytes_ps || police->avrate ||
> > +	    police->overhead) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer parameters are not supported");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (police->rate_pkt_ps) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer parameters are not supported");
> > +		return -EOPNOTSUPP;
> > +	}
> 
> Curious if we can have distinct messages for several unrelated checks
> sharing the extack "Policer parameters are not supported";
> 

So the rationale here is that the checks with their own message are
about where the policer is attached (egress, shared block, another
port, a second policer), or a missing rate, while the shared message
covers police parameters the hardware cannot offload.

> > +
> > +	if (!police->rate_bytes_ps) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer requires a rate");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (police->burst > ANA_AC_POL_PORT_THRES_CFG_0_PORT_THRES0 *
> > +			    SPX5_POLICER_BYTE_BURST_UNIT) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer parameters are not supported");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (police->rate_bytes_ps >
> > +	    div_u64((u64)ANA_AC_POL_PORT_RATE_CFG_PORT_RATE *
> > +		    SPX5_POLICER_RATE_UNIT, BITS_PER_BYTE)) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Policer parameters are not supported");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	idx = sparx5_alloc_port_policer_idx(port, entry->cookie);
> > +	if (idx < 0) {
> > +		NL_SET_ERR_MSG_MOD(extack,
> > +				   "Only one port policer per port is supported");
> > +		return idx;
> > +	}
> > +
> 
> Any reason why police->mtu isn't checked? In software, frames longer than
> the police MTU always take the exceed action. Is this intentionally skipped
> here?

Yes, it is intentional. The port policer has no frame length limit of its own,
so it cannot enforce the mtu. It seems to be very common for other drivers to
ignore this too.

> 
> > +	pol.type = SPX5_POL_PORT;
> > +	pol.rate = police->rate_bytes_ps * BITS_PER_BYTE;
> > +	pol.burst = police->burst;
> > +	pol.idx = port->portno * SPX5_POLICERS_PER_PORT + idx;
> > +
> > +	sparx5_init_port_policer_stats(port, idx);
> > +
> > +	err = sparx5_policer_conf_set(sparx5, &pol);
> > +	if (err)
> > +		return err;
> > +
> > +	port->tc.port_policer[idx].policer = entry->cookie;
> > +
> > +	return 0;
> > +}
> > +
>

/Daniel