On Thu, Jul 10, 2025 at 12:36:10PM +0000, Michael Dege wrote:
> Hello Simon,
>
> > -----Original Message-----
> > From: Simon Horman <horms@xxxxxxxxxx>
> > Sent: Tuesday, July 8, 2025 12:48 PM
> > To: Michael Dege <michael.dege@xxxxxxxxxxx>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>; Niklas Söderlund
> > <niklas.soderlund@xxxxxxxxxxxx>; Paul Barker <paul@xxxxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>;
> > David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski
> > <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-renesas-
> > soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxx>
> > > Signed-off-by: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>
> >
> > ...
> >
> > > 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.
