| 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:
<TY4PR01MB142827FE7895F2D49EF347CDD8257A@TY4PR01MB14282.jpnprd01.prod.outlook.com>
Date: Tue, 15 Jul 2025 11:10:08 +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 Paolo,
Thank you for your valuable comments. I will send an updated patch set after my vacation.
> -----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').
OK, will do.
>
> [...]
> > +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.
OK.
>
> > + }
> > + }
> > +}
> > +
> > +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.
OK.
>
> > + }
> > + }
> > +}
> > +
> > +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...
>
Yes, this is probably some legacy stuff. I don't know what Nikita's intention for it was.
I will take it out.
> > +{
> > + 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.
Good catch. I will drop the unused parameter.
>
> /P
Michael
________________________________
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