| 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:
<TY4PR01MB14282FDB5F80A61205F7080A582C3A@TY4PR01MB14282.jpnprd01.prod.outlook.com>
Date: Fri, 7 Nov 2025 13:35:35 +0000
From: Michael Dege <michael.dege@...esas.com>
To: Andrew Lunn <andrew@...n.ch>
CC: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>, 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>, Richard Cochran <richardcochran@...il.com>,
niklas.soderlund <niklas.soderlund@...natech.se>, Paul Barker
<paul@...rker.dev>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Geert Uytterhoeven
<geert+renesas@...der.be>, magnus.damm <magnus.damm@...il.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>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, Nikita Yushchenko
<nikita.yoush@...entembedded.com>, Christophe JAILLET
<christophe.jaillet@...adoo.fr>
Subject: RE: [PATCH net-next 09/10] net: renesas: rswitch: add simple l3
routing
> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Friday, November 7, 2025 3:32 AM
> To: Michael Dege <michael.dege@...esas.com>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>; 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>; Richard Cochran <richardcochran@...il.com>; niklas.soderlund
> <niklas.soderlund@...natech.se>; Paul Barker <paul@...rker.dev>; Rob Herring <robh@...nel.org>;
> Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; Geert Uytterhoeven
> <geert+renesas@...der.be>; magnus.damm <magnus.damm@...il.com>; netdev@...r.kernel.org; linux-renesas-
> soc@...r.kernel.org; linux-kernel@...r.kernel.org; devicetree@...r.kernel.org; Nikita Yushchenko
> <nikita.yoush@...entembedded.com>; Christophe JAILLET <christophe.jaillet@...adoo.fr>
> Subject: Re: [PATCH net-next 09/10] net: renesas: rswitch: add simple l3 routing
>
> > +void rswitch_update_l3_offload(struct rswitch_private *priv) {
> > + u32 all_ports_mask = GENMASK(RSWITCH_NUM_AGENTS - 1, 0);
> > + struct rswitch_device *rdev;
> > + bool l3_offload_enable_cond;
> > + u32 l3_rdev_count;
> > + u32 l3_ports_mask;
> > +
> > + l3_ports_mask = all_ports_mask;
> > +
> > + l3_rdev_count = 0;
> > + rswitch_for_all_ports(priv, rdev) {
> > + if (rdev_for_l3_offload(rdev)) {
> > + l3_rdev_count++;
> > + l3_ports_mask &= ~BIT(rdev->port);
> > + }
> > + }
> > +
> > + l3_offload_enable_cond = (l3_rdev_count >= 2);
> > +
> > +#define FWPC0_L3_MASK (FWPC0_LTHTA | FWPC0_IP4UE | FWPC0_IP4TE | FWPC0_IP4OE)
> > + rswitch_for_all_ports(priv, rdev) {
> > + if (rdev_for_l3_offload(rdev) && l3_offload_enable_cond) {
> > + /* Update allowed offload destinations even for ports
> > + * with l3 offload enabled earlier.
> > + *
> > + * Allow offload routing to self for hw port.
> > + */
> > + rswitch_modify(priv->addr, FWPC1(rdev->port),
> > + FWPC1_LTHFW_MASK,
> > + FIELD_PREP(FWPC1_LTHFW_MASK, l3_ports_mask));
> > + if (!rdev->l3_offload_enabled) {
> > + rswitch_modify(priv->addr, FWPC0(rdev->port),
> > + 0,
> > + FWPC0_L3_MASK);
> > + rdev->l3_offload_enabled = 1;
> > + netdev_info(rdev->ndev, "starting l3 offload\n");
>
> This, and the other netdev_info calls should probably be debug.
Thank you for the comment. I will update both places.
>
> > +static bool rswitch_l23update_hw_op(struct rswitch_private *priv,
> > + struct rswitch_l23update *update,
> > + bool install)
> > +{
> > + u8 *dst_mac = update->spec.dst_mac;
> > + u32 val;
> > + int ret;
> > +
> > + val = FIELD_PREP(FWL23URL0_RN, update->index) |
> > + FIELD_PREP(FWL23URL0_PV,
> > + install ? GENMASK(RSWITCH_NUM_AGENTS - 1, 0) : 0);
> > + iowrite32(val, priv->addr + FWL23URL0);
> > +
> > + val = FWL23URL1_TTLU |
> > + FWL23URL1_MSAU |
> > + FWL23URL1_MDAU |
> > + (dst_mac[0] << 8) | (dst_mac[1] << 0);
> > + iowrite32(val, priv->addr + FWL23URL1);
> > +
> > + val = (dst_mac[2] << 24) | (dst_mac[3] << 16) |
> > + (dst_mac[4] << 8) | (dst_mac[5] << 0);
> > + iowrite32(val, priv->addr + FWL23URL2);
> > +
> > + iowrite32(0, priv->addr + FWL23URL3);
> > +
> > + /* Rule write starts after writing to FWL23URL3 */
> > +
> > + ret = rswitch_reg_wait(priv->addr, FWL23URLR, FWL23URLR_L, 0);
> > + if (ret) {
> > + dev_err(&priv->pdev->dev, "timeout writing l23_update\n");
> > + return false;
>
> Why not make this an int function and return -ETIMEDOUT?
Good suggestion. I will rework the code accordingly.
>
> > +static bool rmon_ipv4_dst_offload_hw_op(struct rswitch_route_monitor *rmon,
> > + struct rmon_ipv4_dst_offload *offload,
> > + u8 frame_type, bool install)
>
> Why all this bool functions? Especially when you have calls returning error codes you are throwing
> away.
I will look at the code and make improvements.
>
> > +static struct rswitch_l23update *rswitch_get_l23update(struct rswitch_private *priv,
> > + struct rswitch_l23update_spec *spec) {
> > + struct rswitch_l23update *update;
> > +
> > + spin_lock(&priv->l3_lock);
> > +
> > + list_for_each_entry(update, &priv->l23_update_list, list) {
> > + if (rswitch_l23update_matches_spec(update, spec)) {
> > + update->use_count++;
> > + goto out;
> > + }
> > + }
> > +
> > + update = kzalloc(sizeof(*update), GFP_ATOMIC);
> > + if (!update)
> > + goto out;
> > +
> > + update->use_count = 1;
> > + update->spec = *spec;
> > + update->index = find_first_zero_bit(priv->l23_update_bitmap,
> > + RSWITCH_MAX_NUM_RRULE);
> > + if (update->index == RSWITCH_MAX_NUM_RRULE) {
> > + dev_err_ratelimited(&priv->pdev->dev,
> > + "out of l23_update entries\n");
> > + /* FIXME: trigger expire? */
> > + goto no_free_bit;
> > + }
> > + set_bit(update->index, priv->l23_update_bitmap);
> > +
> > + if (!rswitch_l23update_hw_op(priv, update, true))
> > + goto hw_op_failed;
> > +
> > + list_add(&update->list, &priv->l23_update_list);
> > +out:
> > + spin_unlock(&priv->l3_lock);
> > +
> > + return update;
> > +
> > +hw_op_failed:
> > + clear_bit(update->index, priv->l23_update_bitmap);
> > +no_free_bit:
> > + kfree(update);
> > + update = NULL;
> > + goto out;
>
> It is pretty unusual to have a backwards goto, especially in error handling. This is one case where a
> scoped_guard() might make sense.
Thank you for your comment I will investigate and rework it.
Michael
>
> Andrew
________________________________
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