Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

From: Vladimir Oltean <olteanv@gmail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, netdev <netdev@vger.kernel.org>,
	DENG Qingfang <dqfext@gmail.com>,
	Mauri Sandberg <sandberg@mailfence.com>
Subject: Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags
Date: Tue, 31 Aug 2021 01:20:07 +0300	[thread overview]
Message-ID: <20210830222007.2i6k7pg72yuoygwh@skbuf> (raw)
In-Reply-To: <CACRpkdbVs9H8CPYV9Fgwje40qqS=wxXqVkDc=Du=c82eqeKCAw@mail.gmail.com>

On Mon, Aug 30, 2021 at 11:56:22PM +0200, Linus Walleij wrote:
> On Mon, Aug 30, 2021 at 9:29 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Sun, Aug 29, 2021 at 01:56:19AM +0200, Linus Walleij wrote:
> > > I noticed that only port 0 worked on the RTL8366RB since we
> > > started to use custom tags.
> > >
> > > It turns out that the format of egress custom tags is actually
> > > different from ingress custom tags. While the lower bits just
> > > contain the port number in ingress tags, egress tags need to
> > > indicate destination port by setting the bit for the
> > > corresponding port.
> > >
> > > It was working on port 0 because port 0 added 0x00 as port
> > > number in the lower bits, and if you do this the packet gets
> > > broadcasted to all ports, including the intended port.
> > > Ooops.
> >
> > Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> > for a regular data packet?
> 
> It gets broadcast :/

Okay, so a packet sent to a port mask of zero behaves just the same as a
packet sent to a port mask of all ones is what you're saying?
Sounds a bit... implausible?

When I phrased the question whether it gets "forwarded by MAC DA/VLAN ID",
obviously this includes the possibility of _flooding_, if the MAC
DA/VLAN ID is unknown to the FDB. The behavior of flooding a packet due
to unknown destination can be practically indistinguishable from a
"broadcast" (the latter having the sense that "you've told the switch to
broadcast this packet to all ports", at least this is what is implied by
the context of your commit message).

The point is that if the destination is not unknown, the packet is not
flooded (or "broadcast" as you say). So "broadcast" would be effectively
a mischaracterization of the behavior.

Just want to make sure that the switch does indeed "broadcast" packets
with a destination port mask of zero. Also curious if by "all ports",
the CPU port itself is also included (effectively looping back the packet)?

> > > -     out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> >
> > What was 2 << 8? This patch changes that part.
> 
> It was a bit set in the ingress packets, we don't really know
> what egress tag bits there are so first I just copied this
> and since it turns out the bits in the lower order are not
> correct I dropped this too and it works fine.
> 
> Do you want me to clarify in the commit message and
> resend?

Well, it is definitely not a logical part of the change. Also, a bug fix
patch that goes to stable kernels seems like the last place to me where
you'd want to change something that you don't really know what it does...
In net-next, this extra change is more than welcome. Possibly has
something to do with hardware address learning on the CPU port, but this
is just a very wild guess based on some other Realtek tagging protocol
drivers I've looked at recently. Anyway, more than likely not just a
random number with no effect.