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

[Mohsin Bashir <mohsin.bashr@gmail.com>]

> +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";

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

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