Message-ID: <20210222202506.27qp2ltdkgmqgmec@skbuf>
Date:   Mon, 22 Feb 2021 22:25:06 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Horatiu Vultur <horatiu.vultur@...rochip.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

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?

> > - 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?

> > - 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?

Powered by blists - more mailing lists