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

On Tue, Feb 23, 2021 at 02:30:28PM +0100, Horatiu Vultur wrote:

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

quoted

Hi Vladimir,

quoted

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.

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?

quoted

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.

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.