netdev - Re: [PATCH 2/3] net: renesas: rswitch: add offloading for L2 switching

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64e7de61-c4ed-4b42-83c6-5001a9d28ec0@lunn.ch>
Date: Fri, 4 Jul 2025 10:43:57 +0200
From: Andrew Lunn <andrew@...n.ch>
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 2/3] net: renesas: rswitch: add offloading for L2
 switching

>  #include <linux/platform_device.h>
> +#include <linux/phy.h>
> +

It seems odd that a patch adding L2 support needs to touch PHYs?

> @@ -994,10 +1018,18 @@ struct rswitch_device {
>  	DECLARE_BITMAP(ts_skb_used, TS_TAGS_PER_PORT);
>  	bool disabled;
>  
> +	struct list_head list;
> +
>  	int port;
>  	struct rswitch_etha *etha;
>  	struct device_node *np_port;
>  	struct phy *serdes;
> +
> +	struct net_device *brdev;	/* master bridge device */

How many ports does this device have? If it is just two, this might
work. But for a multi-port device, you need to keep this in the port
structure.

> +bool is_rdev(const struct net_device *ndev);
> +void rswitch_modify(void __iomem *addr, enum rswitch_reg reg, u32 clear, u32 set);

Are these actually needed? It seems like they could be local
functions.

> +	if (offload_brdev && !priv->offload_brdev)
> +		dev_info(&priv->pdev->dev, "starting l2 offload for %s\n",
> +			 netdev_name(offload_brdev));
> +	else if (!offload_brdev && priv->offload_brdev)
> +		dev_info(&priv->pdev->dev, "stopping l2 offload for %s\n",
> +			 netdev_name(priv->offload_brdev));

Please don't spam the log like this dev_dbg() maybe.

> @@ -128,6 +134,14 @@ static void rswitch_fwd_init(struct rswitch_private *priv)
>  		iowrite32(0, priv->addr + FWPBFC(i));
>  	}
>  
> +	/* Configure MAC table aging */
> +	rswitch_modify(priv->addr, FWMACAGUSPC, FWMACAGUSPC_MACAGUSP,
> +		       FIELD_PREP(FWMACAGUSPC_MACAGUSP, 0x140));
> +
> +	reg_val = FIELD_PREP(FWMACAGC_MACAGT, RSW_AGEING_TIME);
> +	reg_val |= FWMACAGC_MACAGE | FWMACAGC_MACAGSL;
> +	iowrite32(reg_val, priv->addr + FWMACAGC);
> +

Please pull ageing out into a patch of its own.

    Andrew

---
pw-bot: cr