Message-ID: <20210223133028.sem3hykvm5ld2unq@soft-dev3-1.localhost>
Date:   Tue, 23 Feb 2021 14:30:28 +0100
From:   Horatiu Vultur <horatiu.vultur@...rochip.com>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Ido Schimmel <idosch@...sch.org>,
        DENG Qingfang <dqfext@...il.com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        George McCollister <george.mccollister@...il.com>,
        Kurt Kanzenbach <kurt@...utronix.de>
Subject: 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:
> > > - 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

Powered by blists - more mailing lists