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