On Thu, 2022-02-17 at 14:49 +0200, Vladimir Oltean wrote:
> 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@xxxxxxxxxx>
> > Reviewed-by: Roi Dayan <roid@xxxxxxxxxx>
> > Reviewed-by: Ido Schimmel <idosch@xxxxxxxxxx>
> > ---
>
> Tested-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
>
> 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.
Hi Vladimir,
I'd love to hear your suggestion regarding where this validate function
to be placed for drivers/net/ethernet/mscc, as it will be used by both
ocelot_net.c and ocelot_flower.c.
Thanks!
Jianbo
