RE: [PATCH net] net: renesas: rswitch: fix forwarding offload statemachine

[Michael Dege <michael.dege@renesas.com>]

Hello Nikita,

Thank you once more for your comments.

> -----Original Message-----
> From: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> Sent: Thursday, February 5, 2026 8:59 AM
> To: Michael Dege <michael.dege@renesas.com>; Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>;
> Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> Cc: netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net] net: renesas: rswitch: fix forwarding offload statemachine
> 
> 
> 
> WBR,
> Nikita Yushchenko,
> System Software Engineer @ Cogent Embedded
> 
> 05.02.2026 08:47, Nikita Yushchenko wrote:
> > Hello Michael
> >
> >> -        } else if (rdev->forwarding_offloaded) {
> >> +        } else if (rdev->forwarding_offloaded &&
> >> +               !rdev->forwarding_requested) {
> >>               rswitch_change_l2_hw_offloading(rdev, false, false);
> >>           }
> >
> > Although indeed the condition in the current code is not correct, I'm not sure comfortable with this
> fix.
> >
> > Full condition for a port to be a valid candidate for hardware
> > forwarding is
> >
> >    rdev_for_l2_offload() && rdev->forwarding_requested
> >
> > It is not obvious if at this point rdev_for_l2_offload() could get
> > changed from the last call to rswitch_change_l2_hw_offloading(), so
> > using only the partial condition at this point does not look good for me.
> >
> > I'd suggest to either change to something like
> >
> > if (rdev_for_l2_offload() && rdev->forwarding_requested &&
> > !rdev->forwarding_offloaded)
> >      rswitch_change_l2_hw_offloading(rdev, true, false); if
> > (!(rdev_for_l2_offload() && rdev->forwarding_requested) &&
> > rdev->forwarding_offloaded)
> >      rswitch_change_l2_hw_offloading(rdev, false, false);

This works as expected.

> >
> > Or maybe just
> >
> > if (rdev_for_l2_offload() && rdev->forwarding_requested)
> >      rswitch_change_l2_hw_offloading(rdev, true, false); else
> >      rswitch_change_l2_hw_offloading(rdev, false, false);
> >
> > since rswitch_change_l2_hw_offloading() has internal check for the
> > current state and returns early if the requested change is already applied.

Unfortunately, this has a side effect, e.g., if you pull the cable on tsn0 and the link 
goes down, you will see that the offloading is disabled on all ports connected to the 
bridge and not just on tsn0.

> 
> May be even better to add
> 
>    bool new_forwarding_offloaded = rdev_for_l2_offload(rdev) && rdev->forwarding_requested;
> 
> at the beginning of the loop body, and use this flag over the loop - it will make the code shorter and
> cleaner.

Yes, this does make the code cleaner. 

I will send around the updated patch shortly.

Best regards,

Michael