| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250711105721.GU721198@horms.kernel.org>
Date: Fri, 11 Jul 2025 11:57:21 +0100
From: Simon Horman <horms@...nel.org>
To: Michael Dege <michael.dege@...esas.com>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
Niklas Söderlund <niklas.soderlund@...natech.se>,
Paul Barker <paul@...rker.dev>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Nikita Yushchenko <nikita.yoush@...entembedded.com>
Subject: Re: [PATCH v2 3/4] net: renesas: rswitch: add offloading for L2
switching
On Thu, Jul 10, 2025 at 12:36:10PM +0000, Michael Dege wrote:
> Hello Simon,
>
> > -----Original Message-----
> > From: Simon Horman <horms@...nel.org>
> > Sent: Tuesday, July 8, 2025 12:48 PM
> > To: Michael Dege <michael.dege@...esas.com>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>; Niklas Söderlund
> > <niklas.soderlund@...natech.se>; Paul Barker <paul@...rker.dev>; Andrew Lunn <andrew+netdev@...n.ch>;
> > David S. Miller <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub Kicinski
> > <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; netdev@...r.kernel.org; linux-renesas-
> > soc@...r.kernel.org; linux-kernel@...r.kernel.org; Nikita Yushchenko <nikita.yoush@...entembedded.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@...esas.com>
> > > Signed-off-by: Nikita Yushchenko <nikita.yoush@...entembedded.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.
Powered by blists - more mailing lists