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

From: Vladimir Oltean <olteanv@gmail.com>
To: Horatiu Vultur <horatiu.vultur@microchip.com>
Cc: netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>, Ido Schimmel <idosch@idosch.org>,
	DENG Qingfang <dqfext@gmail.com>,
	Tobias Waldekranz <tobias@waldekranz.com>,
	George McCollister <george.mccollister@gmail.com>,
	Kurt Kanzenbach <kurt@linutronix.de>
Subject: Re: [RFC PATCH net-next 09/12] Documentation: networking: dsa: add paragraph for the MRP offload
Date: Tue, 23 Feb 2021 15:50:15 +0200	[thread overview]
Message-ID: <20210223135015.ssqm3t7fajplceyx@skbuf> (raw)
In-Reply-To: <20210223133028.sem3hykvm5ld2unq@soft-dev3-1.localhost>

On Tue, Feb 23, 2021 at 02:30:28PM +0100, Horatiu Vultur wrote:
> 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.

A physical port can't offload more than one ring id under any
circumstance, no? So why keep a list and not just keep the MRP ring id
in the ocelot_port structure, then when the ring role changes, just
iterate through all ports and update the trapping rule on those having
the same ring id?

Also, why is it important to know which port is primary and which is
secondary?

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

Interesting, so I think with a little bit more forethought, the
intentions with this MRP hardware assist would have been much clearer.
From what you explained, the better implementation wouldn't have been
more complicated than the current one is, just cleaner.