| 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:
<TY4PR01MB14282F9A7392582A9F6D066188228A@TY4PR01MB14282.jpnprd01.prod.outlook.com>
Date: Mon, 11 Aug 2025 10:46:11 +0000
From: Michael Dege <michael.dege@...esas.com>
To: Paolo Abeni <pabeni@...hat.com>, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@...esas.com>, niklas.soderlund
<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>
CC: "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 v3 3/4] net: renesas: rswitch: add offloading for L2
switching
Hello Poalo,
> -----Original Message-----
> From: Paolo Abeni <pabeni@...hat.com>
> Sent: Tuesday, July 15, 2025 1:00 PM
> To: Michael Dege <michael.dege@...esas.com>; Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>;
> niklas.soderlund <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>
> Cc: netdev@...r.kernel.org; linux-renesas-soc@...r.kernel.org; linux-kernel@...r.kernel.org; Nikita
> Yushchenko <nikita.yoush@...entembedded.com>
> Subject: Re: [PATCH v3 3/4] net: renesas: rswitch: add offloading for L2 switching
>
> On 7/10/25 2:31 PM, 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: Nikita Yushchenko <nikita.yoush@...entembedded.com>
> > Signed-off-by: Michael Dege <michael.dege@...esas.com>
>
> Minor nit: you should specify the target tree in the subj prefix (in this case 'net-next').
>
> [...]
> > +static void rswitch_update_l2_hw_learning(struct rswitch_private
> > +*priv) {
> > + bool learning_needed;
> > + struct rswitch_device *rdev;
> > +
> > + rswitch_for_all_ports(priv, rdev) {
> > + if (rdev_for_l2_offload(rdev))
> > + learning_needed = rdev->learning_requested;
> > + else
> > + learning_needed = false;
> > +
> > + if (!rdev->learning_offloaded && learning_needed) {
> > + rswitch_modify(priv->addr, FWPC0(rdev->port),
> > + 0,
> > + FWPC0_MACSSA | FWPC0_MACHLA | FWPC0_MACHMA);
> > +
> > + rdev->learning_offloaded = true;
> > + netdev_info(rdev->ndev, "starting hw learning\n");
> > + }
> > +
> > + if (rdev->learning_offloaded && !learning_needed) {
> > + rswitch_modify(priv->addr, FWPC0(rdev->port),
> > + FWPC0_MACSSA | FWPC0_MACHLA | FWPC0_MACHMA,
> > + 0);
> > +
> > + rdev->learning_offloaded = false;
> > + netdev_info(rdev->ndev, "stopping hw learning\n");
>
> You could factor out the above 3 statements is a separare helper receving the new 'leraning_offloaded'
> status and save some code duplication.
I took a deeper look at the above code and there isn't really any duplication to be found. If you look
carefully the second and third if statement are the exact opposite of each other. Of course it would be
possible to pull out the if statements and put the register modification into a separate function. But,
overall code amount would increase since I would have to track the state change and make sure the HW is
only modified incase of a state change.
Do you have any suggestions?
>
> > + }
> > + }
> > +}
> > +
> > +static void rswitch_update_l2_hw_forwarding(struct rswitch_private
> > +*priv) {
> > + struct rswitch_device *rdev;
> > + unsigned int fwd_mask;
> > +
> > + /* calculate fwd_mask with zeroes in bits corresponding to ports that
> > + * shall participate in hardware forwarding
> > + */
> > + fwd_mask = GENMASK(RSWITCH_NUM_AGENTS - 1, 0);
> > +
> > + rswitch_for_all_ports(priv, rdev) {
> > + if (rdev_for_l2_offload(rdev) && rdev->forwarding_requested)
> > + fwd_mask &= ~BIT(rdev->port);
> > + }
> > +
> > + rswitch_for_all_ports(priv, rdev) {
> > + if (rdev_for_l2_offload(rdev) && rdev->forwarding_requested) {
> > + /* Update allowed offload destinations even for ports
> > + * with L2 offload enabled earlier.
> > + *
> > + * Do not allow L2 forwarding to self for hw port.
> > + */
> > + iowrite32(FIELD_PREP(FWCP2_LTWFW_MASK, fwd_mask | BIT(rdev->port)),
> > + priv->addr + FWPC2(rdev->port));
> > +
> > + if (!rdev->forwarding_offloaded) {
> > + rswitch_modify(priv->addr, FWPC0(rdev->port),
> > + 0,
> > + FWPC0_MACDSA);
> > +
> > + rdev->forwarding_offloaded = true;
> > + netdev_info(rdev->ndev,
> > + "starting hw forwarding\n");
> > + }
> > + } else if (rdev->forwarding_offloaded) {
> > + iowrite32(FIELD_PREP(FWCP2_LTWFW_MASK, fwd_mask | BIT(rdev->port)),
> > + priv->addr + FWPC2(rdev->port));
> > +
> > + rswitch_modify(priv->addr, FWPC0(rdev->port),
> > + FWPC0_MACDSA,
> > + 0);
> > +
> > + rdev->forwarding_offloaded = false;
> > + netdev_info(rdev->ndev, "stopping hw forwarding\n");
>
> Similar thing above.
The only code duplication here is this statement:
+ iowrite32(FIELD_PREP(FWCP2_LTWFW_MASK, fwd_mask | BIT(rdev->port)),
+ priv->addr + FWPC2(rdev->port));
Also, here we have the situation, that we only want to modify the HW if the state changes. Therefore
this code duplication makes sense to me. Any effort to remove that would in my opinion lead to more
and more difficult code.
Any thought or suggestions?
Best regards,
Michael
>
> > + }
> > + }
> > +}
> > +
> > +void rswitch_update_l2_offload(struct rswitch_private *priv) {
> > + rswitch_update_l2_hw_learning(priv);
> > + rswitch_update_l2_hw_forwarding(priv);
> > +}
> > +
> > +static void rswitch_update_offload_brdev(struct rswitch_private *priv,
> > + bool force_update_l2_offload)
>
> Apparently always called with force_update_l2_offload == false, if so you should drop such argument...
>
> > +{
> > + 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 && !force_update_l2_offload)
> > + return;
> > +
> > + if (offload_brdev == priv->offload_brdev)
>
> ... otherwise (this function can be called with force_update_l2_offload == true) we can reach here
> with priv->offload_brdev and/or offload_brdev == NULL and the following statement will cause a NULL
> ptr dereference.
>
> /P
________________________________
Renesas Electronics Europe GmbH
Registered Office: Arcadiastrasse 10
DE-40472 Duesseldorf
Commercial Registry: Duesseldorf, HRB 3708
Managing Director: Carsten Jauch
VAT-No.: DE 14978647
Tax-ID-No: 105/5839/1793
Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.
Powered by blists - more mailing lists