Re: [PATCH net-next v2 2/2] flow_offload: reject offload for all drivers with invalid police parameters
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2022-02-17 12:49:44
Also in:
linux-rdma, lkml
Subsystem:
networking drivers, networking [dsa], the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Andrew Lunn, Vladimir Oltean, Linus Torvalds
On Thu, Feb 17, 2022 at 08:28:03AM +0000, Jianbo Liu wrote:
As more police parameters are passed to flow_offload, driver can check them to make sure hardware handles packets in the way indicated by tc. The conform-exceed control should be drop/pipe or drop/ok. Besides, for drop/ok, the police should be the last action. As hardware can't configure peakrate/avrate/overhead, offload should not be supported if any of them is configured. Signed-off-by: Jianbo Liu <redacted> Reviewed-by: Roi Dayan <redacted> Reviewed-by: Ido Schimmel <idosch@nvidia.com> ---
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> But could we cut down on line length a little? Example for sja1105 (messages were also shortened):
diff --git a/drivers/net/dsa/sja1105/sja1105_flower.c b/drivers/net/dsa/sja1105/sja1105_flower.c
index 8a14df8cf91e..54a16369a39e 100644
--- a/drivers/net/dsa/sja1105/sja1105_flower.c
+++ b/drivers/net/dsa/sja1105/sja1105_flower.c@@ -300,6 +300,46 @@ static int sja1105_flower_parse_key(struct sja1105_private *priv, return -EOPNOTSUPP; } +static int sja1105_policer_validate(const struct flow_action *action, + const struct flow_action_entry *act, + struct netlink_ext_ack *extack) +{ + if (act->police.exceed.act_id != FLOW_ACTION_DROP) { + NL_SET_ERR_MSG_MOD(extack, + "Offload not supported when exceed action is not drop"); + return -EOPNOTSUPP; + } + + if (act->police.notexceed.act_id != FLOW_ACTION_PIPE && + act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) { + NL_SET_ERR_MSG_MOD(extack, + "Offload not supported when conform action is not pipe or ok"); + return -EOPNOTSUPP; + } + + if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT && + !flow_action_is_last_entry(action, act)) { + NL_SET_ERR_MSG_MOD(extack, + "Offload not supported when conform action is ok, but action is not last"); + return -EOPNOTSUPP; + } + + if (act->police.peakrate_bytes_ps || + act->police.avrate || act->police.overhead) { + NL_SET_ERR_MSG_MOD(extack, + "Offload not supported when peakrate/avrate/overhead is configured"); + return -EOPNOTSUPP; + } + + if (act->police.rate_pkt_ps) { + NL_SET_ERR_MSG_MOD(extack, + "QoS offload not support packets per second"); + return -EOPNOTSUPP; + } + + return 0; +} + int sja1105_cls_flower_add(struct dsa_switch *ds, int port, struct flow_cls_offload *cls, bool ingress) {
@@ -321,39 +361,10 @@ int sja1105_cls_flower_add(struct dsa_switch *ds, int port, flow_action_for_each(i, act, &rule->action) { switch (act->id) { case FLOW_ACTION_POLICE: - if (act->police.exceed.act_id != FLOW_ACTION_DROP) { - NL_SET_ERR_MSG_MOD(extack, - "Police offload is not supported when the exceed action is not drop"); - return -EOPNOTSUPP; - } - - if (act->police.notexceed.act_id != FLOW_ACTION_PIPE && - act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) { - NL_SET_ERR_MSG_MOD(extack, - "Police offload is not supported when the conform action is not pipe or ok"); - return -EOPNOTSUPP; - } - - if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT && - !flow_action_is_last_entry(&rule->action, act)) { - NL_SET_ERR_MSG_MOD(extack, - "Police offload is not supported when the conform action is ok, but police action is not last"); - return -EOPNOTSUPP; - } - - if (act->police.peakrate_bytes_ps || - act->police.avrate || act->police.overhead) { - NL_SET_ERR_MSG_MOD(extack, - "Police offload is not supported when peakrate/avrate/overhead is configured"); - return -EOPNOTSUPP; - } - - if (act->police.rate_pkt_ps) { - NL_SET_ERR_MSG_MOD(extack, - "QoS offload not support packets per second"); - rc = -EOPNOTSUPP; + rc = sja1105_policer_validate(&rule->action, act, + extack); + if (rc) goto out; - } rc = sja1105_flower_policer(priv, port, extack, cookie, &key,
Also, if you create a "validate" function for every driver, you'll remove code duplication for those drivers that support both matchall and flower policers.