On Mon, Aug 23, 2021 at 12:28:51AM +0000, Alvin Šipraga wrote:

On 8/23/21 1:45 AM, Vladimir Oltean wrote:

quoted

On Sun, Aug 22, 2021 at 11:37:28PM +0000, Alvin Šipraga wrote:

quoted

quoted

quoted

quoted

quoted

+	skb->offload_fwd_mark = 1;

At the very least, please use

	dsa_default_offload_fwd_mark(skb);

which does the right thing when the port is not offloading the bridge.

Sure. Can you elaborate on what you mean by "at the very least"? Can it
be improved even further?

The elaboration is right below. skb->offload_fwd_mark should be set to
zero for packets that have been forwarded only to the host (like packets
that have hit a trapping rule). I guess the switch will denote this
piece of info through the REASON code.

Yes, I think it will be communicated in REASON too. I haven't gotten to
deciphering the contents of this field since it has not been needed so
far: the ports are fully isolated and all bridging is done in software.

In that case, setting skb->offload_fwd_mark to true is absolutely wrong,
since the bridge is told that no software forwarding should be done
between ports, as it was already done in hardware (see nbp_switchdev_allowed_egress).

I wonder how this has ever worked? Are you completely sure that bridging
is done in software?

You are absolutely right, and indeed I checked just now and the bridging
is not working at all.

Deleting the line (i.e. skb->offload_fwd_mark = 0) restores the expected
bridging behaviour.

Based on what you have said, do I understand correctly that
offload_fwd_mark shouldn't be set until bridge hardware offloading has
been implemented?

Thanks for your detailed review so far.

So back to my initial suggestion:

| At the very least, please use
|
| 	dsa_default_offload_fwd_mark(skb);
|
| which does the right thing when the port is not offloading the bridge.

This way, you won't have to touch this code even after you start
implementing .port_bridge_join and .port_bridge_leave. It deals with
both cases.