Re: [RFC PATCH net-next 09/12] Documentation: networking: dsa: add paragraph for the MRP offload

The 02/22/2021 22:25, Vladimir Oltean wrote:

Hi Vladimir,

Hi Horatiu,

On Mon, Feb 22, 2021 at 08:46:26PM +0100, Horatiu Vultur wrote:

quoted

quoted

- Why does ocelot support a single MRP ring if all it does is trap the
  MRP PDUs to the CPU? What is stopping it from supporting more than
  one ring?

So the HW can support to run multiple rings. But to have an initial
basic implementation I have decided to support only one ring. So
basically is just a limitation in the driver.

What should change in the current sw_backup implementation such that
multiple rings are supported?

Instead of single mrp_ring_id, mrp_p_port and mrp_s_port is to have a
list of these. And then when a new MRP instance is added/removed this
list should be updated. When the role is changed then find the MRP ports
from this list and put the rules to these ports.

quoted

quoted

- Why is listening for SWITCHDEV_OBJ_ID_MRP necessary at all, since it
  does nothing related to hardware configuration?

It is listening because it needs to know which ports are part of the
ring. In case you have multiple rings and do forwarding in HW you need
to know which ports are part of which ring. Also in case a MRP frame
will come on a port which is not part of the ring then that frame should
be flooded.

If I understand correctly, you just said below that this is not
applicable to the current implementation because it is simplistic enough
that it doesn't care what ring role does the application use, because it
doesn't attempt to do any forwarding of MRP PDUs at all. If all that
there is to do for a port with sw_backup is to add a trapping rule per
port (rule which is already added per port), then what extra logic is
there to add for the second MRP instance on a different set of 2 ports?

Regarding rules nothing should be changed. You just need to know which
is this new MRP instance so to put the same rules on these 2 ports. And
you can use the ring_id to determin which MRP instance it is and from
there you can find the ports.

quoted

quoted

- Why is ocelot_mrp_del_vcap called from both ocelot_mrp_del and from
  ocelot_mrp_del_ring_role?

To clean after itself. Lets say a user creates a node and sets it up.
Then when she decides to delete the node, what should happen? Should it
first disable the node and then do the cleaning or just do the cleaning?
This userspace application[1] does the second option but I didn't want
to implement the driver to be specific to this application so I have put
the call in both places.

I was actually thinking that the bridge could clean up after itself and
delete the SWITCHDEV_OBJ_ID_RING_ROLE_MRP object.

quoted

quoted

- Why does ocelot not look at the MRM/MRC ring role at all, and it traps
  all MRP PDUs to the CPU, even those which it could forward as an MRC?
  I understood from your commit d8ea7ff3995e ("net: mscc: ocelot: Add
  support for MRP") description that the hardware should be able of
  forwarding the Test PDUs as a client, however it is obviously not
  doing that.

It doesn't look at the role because it doesn't care. Because in both
cases is looking at the sw_backup because it doesn't support any role
completely. Maybe comment was misleading but I have put it under
'current limitations' meaning that the HW can do that but the driver
doesn't take advantage of that yet. The same applies to multiple rings
support.

The idea is to remove these limitations in the next patches and
to be able to remove these limitations then the driver will look also
at the role.

[1] https://github.com/microchip-ung/mrp

By the way, how can Ocelot trap some PDUs to the CPU but forward others?
Doesn't it need to parse the MRP TLVs in order to determine whether they
are Test packets or something else?

No it doesn't need to do that. Because Test packets are send to dmac
01:15:4e:00:00:01 while the other ring MRP frames are send to
01:15:4e:00:00:02. And Ocelot can trap frames based on the dmac.

I will create a patch with these changes when the net-next tree will
open.

-- 
/Horatiu