Re: [PATCH net] net: dsa: reference count the host mdb addresses

[Tobias Waldekranz <tobias@waldekranz.com>]

On Sat, Nov 28, 2020 at 00:27, Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> On Sat, Nov 28, 2020 at 12:58:10AM +0100, Tobias Waldekranz wrote:
>> That sounds like a good idea. We have run into another issue with the
>> MDB that maybe could be worked into this changeset. This is what we have
>> observed on 4.19, but from looking at the source it does not look like
>> anything has changed with respect to this issue.
>>
>> The DSA driver handles the addition/removal of router ports by
>> enabling/disabling multicast flooding to the port in question. On
>> mv88e6xxx at least, this is only part of the solution. It only takes
>> care of the unregistered multicast. You also have to iterate through all
>> _registered_ groups and add the port to the destination vector.
>
> And this observation is based on what? Based on this paragraph from RFC4541?

Well in all honesty, it is mostly based on information from a colleague
of mine who knows the ins and outs of all these RFCs.

> 2.1.2.  Data Forwarding Rules
>
>    1) Packets with a destination IP address outside 224.0.0.X which are
>       not IGMP should be forwarded according to group-based port
>       membership tables and must also be forwarded on router ports.

...but yes, that paragraph does not leave a lot of wiggle room :)

> Let me ask you a different question. Why would DSA be in charge of
> updating the MDB records, and not the bridge? Or why DSA and not the end
> driver? Ignore my patch. I'm just trying to understand what you're
> saying. Why precisely DSA, the mid layer? I don't know, this is new
> information to me, I'm still digesting it.

The bridge has all the necessary information for sure. But it has a
different model with a separate list of router ports. Then in
br_multicast_flood you simply forward to the union of the group entry
and the router ports. It is not the bridge's fault that our hardware
does not have a separate bitmask for router ports. Some hardware may
very well have it.

I guess we could create internal APIs to the bridge to retrieve the
information though. There is already `bool br_multicast_router(dev)`, so
we should only need to add `bool br_multicast_member(dev, group, vid)`.

Assuming that those were available we should be able to solve it either
at the DSA or the driver layer. I seem to recall some issue that forced
us to place the cache at the dst level, but I would have to go through
the implementation to figure out what that issue was.