Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 17 Feb 2022 14:49:35 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Jianbo Liu <jianbol@nvidia.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-rdma@vger.kernel.org, andrew@lunn.ch,
        vivien.didelot@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, kuba@kernel.org, rajur@chelsio.com,
        claudiu.manoil@nxp.com, sgoutham@marvell.com, gakula@marvell.com,
        sbhatta@marvell.com, hkelam@marvell.com, saeedm@nvidia.com,
        leon@kernel.org, idosch@nvidia.com, petrm@nvidia.com,
        alexandre.belloni@bootlin.com, UNGLinuxDriver@microchip.com,
        simon.horman@corigine.com, jhs@mojatatu.com,
        xiyou.wangcong@gmail.com, jiri@resnulli.us,
        baowen.zheng@corigine.com, louis.peens@netronome.com,
        peng.zhang@corigine.com, oss-drivers@corigine.com, roid@nvidia.com
Subject: Re: [PATCH net-next v2 2/2] flow_offload: reject offload for all
 drivers with invalid police parameters
Message-ID: <20220217124935.p7pbgv2cfmhpshxv@skbuf>
References: <20220217082803.3881-1-jianbol@nvidia.com>
 <20220217082803.3881-3-jianbol@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220217082803.3881-3-jianbol@nvidia.com>
Precedence: bulk

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 <jianbol@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> 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.