Re: [PATCH net-next] net: ocelot: Extend MRP

The 03/11/2021 20:02, Vladimir Oltean wrote:

On Thu, Mar 11, 2021 at 08:30:08PM +0100, Horatiu Vultur wrote:

quoted

quoted

quoted

+static void ocelot_mrp_save_mac(struct ocelot *ocelot,
+                             struct ocelot_port *port)
+{
+     ocelot_mact_learn(ocelot, PGID_MRP, mrp_test_dmac,
+                       port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
+     ocelot_mact_learn(ocelot, PGID_MRP, mrp_control_dmac,
+                       port->pvid_vlan.vid, ENTRYTYPE_LOCKED);

Let me make sure I understand.
By learning these multicast addresses, you mark them as 'not unknown' in
the MAC table, because otherwise they will be flooded, including to the
CPU port module, and there's no way you can remove the CPU from the
flood mask, even if the packets get later redirected through VCAP IS2?

Yes, so far you are right.

quoted

I mean that's the reason why we have the policer on the CPU port for the
drop action in ocelot_vcap_init, no?

I am not sure that would work because I want the action to be redirect
and not policy. Or maybe I am missing something?

Yes, it is not the same context as for tc-drop. The problem for tc-drop
was that the packets would get removed from the hardware datapath, but
they would still get copied to the CPU nonetheless. A policer there was
an OK solution because we wanted to kill those packets completely. Here,
the problem is the same, but we cannot use the same solution, since a
policer will also prevent the frames from being redirected.

quoted

quoted

quoted

diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 425ff29d9389..c41696d2e82b 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h

@@ -51,6 +51,7 @@
  */

 /* Reserve some destination PGIDs at the end of the range:
+ * PGID_MRP: used for not flooding MRP frames to CPU

Could this be named PGID_BLACKHOLE or something? It isn't specific to
MRP if I understand correctly. We should also probably initialize it
with zero.

It shouldn't matter the value, what is important that the CPU port not
to be set. Because the value of this PGID will not be used in the
fowarding decision.
Currently only MRP is using it so that is the reason for naming it like
that but I can rename it and initialized it to 0 to be more clear.

So tell me more about this behavior.
Is there no way to suppress the flooding to CPU action, even if the
frame was hit by a TCAM rule? Let's forget about MRP, assume this is an
broadcast IPv4 packet, and we have a matching src_ip rule to perform
mirred egress redirect to another port.
Would the CPU be flooded with this traffic too? What would you do to
avoid that situation?

I think so, I need to ask around to be able to answer your question.

You have to think about CPU port as a special port. If at any point
while the frame goes through the switch, there is a decision to copy the
frame to CPU, the frame will be copied to CPU regardless of the previous
or next decisions. That is at least my understanding.

-- 
/Horatiu