Re: [PATCH net-next v2 2/2] flow_offload: reject offload for all drivers with invalid police parameters

[Jianbo Liu <jianbol@nvidia.com>]

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@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.

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