Re: [PATCH net-next v2 4/4] net: marvell: prestera: Offload FLOW_ACTION_POLICE

Hi Ido,

Thanks for the review. Pls see the comments inline.

On Mon, Aug 02, 2021 at 05:08:49PM +0300, Vadym Kochan wrote:

quoted

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_flower.c b/drivers/net/ethernet/marvell/prestera/prestera_flower.c
index e571ba09ec08..76f30856ac98 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_flower.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_flower.c

@@ -5,6 +5,8 @@

 #include "prestera_acl.h"
 #include "prestera_flower.h"

+#define PRESTERA_HW_TC_NUM   8
+
 static int prestera_flower_parse_actions(struct prestera_flow_block *block,
                                        struct prestera_acl_rule *rule,
                                        struct flow_action *flow_action,

@@ -30,6 +32,11 @@ static int prestera_flower_parse_actions(struct prestera_flow_block *block,

               case FLOW_ACTION_TRAP:
                       a_entry.id = PRESTERA_ACL_RULE_ACTION_TRAP;
                       break;
+             case FLOW_ACTION_POLICE:
+                     a_entry.id = PRESTERA_ACL_RULE_ACTION_POLICE;
+                     a_entry.police.rate = act->police.rate_bytes_ps;
+                     a_entry.police.burst = act->police.burst;

If packet rate based policing is not supported, an error should be
returned here with extack.

Agree, it makes sense.

It seems the implementation assumes that each rule has a different
policer, so an error should be returned in case the same policer is
shared between different rules.

Each rule has a different policer assigned by HW. Do you mean the police.index should be checked here ?

quoted

+                     break;
               default:
                       NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
                       pr_err("Unsupported action\n");

@@ -110,6 +117,17 @@ static int prestera_flower_parse(struct prestera_flow_block *block,

               return -EOPNOTSUPP;
       }

+     if (f->classid) {
+             int hw_tc = __tc_classid_to_hwtc(PRESTERA_HW_TC_NUM, f->classid);
+
+             if (hw_tc < 0) {
+                     NL_SET_ERR_MSG_MOD(f->common.extack, "Unsupported HW TC");
+                     return hw_tc;
+             }
+
+             prestera_acl_rule_hw_tc_set(rule, hw_tc);
+     }

Not sure what this is. Can you show a command line example of how this
is used?

This is HW traffic class used for packets that are trapped to CPU port. The usage is as the following:

tc qdisc add dev DEV clsact
tc filter add dev DEV ingress flower skip_sw dst_mac 00:AA:AA:AA:AA:00 hw_tc 1 action trap

What about visibility regarding number of packets that were dropped by
the policer?

This is not support at this moment by the driver, so it is always zero now.