Re: [PATCH v2 3/4] net: renesas: rswitch: add offloading for L2 switching

[Simon Horman <horms@kernel.org>]

On Thu, Jul 10, 2025 at 12:36:10PM +0000, Michael Dege wrote:
> Hello Simon,
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Tuesday, July 8, 2025 12:48 PM
> > To: Michael Dege <michael.dege@renesas.com>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>; Niklas Söderlund
> > <niklas.soderlund@ragnatech.se>; Paul Barker <paul@pbarker.dev>; 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>; netdev@vger.kernel.org; linux-renesas-
> > soc@vger.kernel.org; linux-kernel@vger.kernel.org; Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> > Subject: Re: [PATCH v2 3/4] net: renesas: rswitch: add offloading for L2 switching
> >
> > On Tue, Jul 08, 2025 at 11:27:39AM +0200, Michael Dege wrote:
> > > This commit adds hardware offloading for L2 switching on R-Car S4.
> > >
> > > On S4 brdev is limited to one per-device (not per port). Reasoning is
> > > that hw L2 forwarding support lacks any sort of source port based
> > > filtering, which makes it unusable to offload more than one bridge
> > > device. Either you allow hardware to forward destination MAC to a
> > > port, or you have to send it to CPU. You can't make it forward only if
> > > src and dst ports are in the same brdev.
> > >
> > > Signed-off-by: Michael Dege <michael.dege@renesas.com>
> > > Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> >
> > ...
> >
> > > diff --git a/drivers/net/ethernet/renesas/rswitch_l2.c
> > > b/drivers/net/ethernet/renesas/rswitch_l2.c
> >
> > ...
> >
> > > +static void rswitch_update_offload_brdev(struct rswitch_private *priv,
> > > +                                    bool force_update_l2_offload)
> > > +{
> > > +   struct net_device *offload_brdev = NULL;
> > > +   struct rswitch_device *rdev, *rdev2;
> > > +
> > > +   rswitch_for_all_ports(priv, rdev) {
> > > +           if (!rdev->brdev)
> > > +                   continue;
> > > +           rswitch_for_all_ports(priv, rdev2) {
> > > +                   if (rdev2 == rdev)
> > > +                           break;
> > > +                   if (rdev2->brdev == rdev->brdev) {
> > > +                           offload_brdev = rdev->brdev;
> > > +                           break;
> > > +                   }
> > > +           }
> > > +           if (offload_brdev)
> > > +                   break;
> > > +   }
> > > +
> > > +   if (offload_brdev == priv->offload_brdev) {
> > > +           if (offload_brdev && force_update_l2_offload)
> > > +                   rswitch_update_l2_offload(priv);
> > > +           return;
> > > +   }
> > > +
> > > +   if (offload_brdev && !priv->offload_brdev)
> > > +           dev_dbg(&priv->pdev->dev, "starting l2 offload for %s\n",
> > > +                   netdev_name(offload_brdev));
> > > +   else if (!offload_brdev && priv->offload_brdev)
> > > +           dev_dbg(&priv->pdev->dev, "stopping l2 offload for %s\n",
> > > +                   netdev_name(priv->offload_brdev));
> > > +   else
> > > +           dev_dbg(&priv->pdev->dev,
> > > +                   "changing l2 offload from %s to %s\n",
> > > +                   netdev_name(priv->offload_brdev),
> > > +                   netdev_name(offload_brdev));
> >
> > Smatch flags a false-positive about possible NULL references by the
> > netdev_name() calls on the line above.
> >
> > Due to the previous if statement it seems to me that cannot occur.
> > But it did take me a few moments to convince myself of that.
> >
> > So while I don't think we should write our code to static analysis tooling.
> > I did play around a bit to see if I could come up with something that is both easier on the eyes and
> > keeps Smatch happy.
> >
> > Perhaps it isn't easier on the eyes, but rather I'm just more familiar with the code now. But in any
> > case, I'm sharing what I came up with in case it is useful. (Compile tested only!).
> >
> >
> >         if (!offload_brdev && !priv->offload_brdev)
> >                 return;
> >
> >         if (!priv->offload_brdev)
> >                 dev_dbg(&priv->pdev->dev, "starting l2 offload for %s\n",
> >                         netdev_name(offload_brdev));
> >         else if (!offload_brdev)
> >                 dev_dbg(&priv->pdev->dev, "stopping l2 offload for %s\n",
> >                         netdev_name(priv->offload_brdev));
> >         else if (offload_brdev != priv->offload_brdev)
> >                 dev_dbg(&priv->pdev->dev,
> >                         "changing l2 offload from %s to %s\n",
> >                         netdev_name(priv->offload_brdev),
> >                         netdev_name(offload_brdev));
> >         else if (!force_update_l2_offload)
> >                 return;
> >
> 
> I updated the code, I hope it is OK, because I had to do it differently from your suggestion, because
> not all cases worked as expected.
> 
> The reworked code is tested.

Thanks. FWIIW, Smatch still complains.

But if your code is correct and tested, then I think we should not
update it a 2nd time to make the tooling happy.