linux-kernel - Re: [PATCH v2 3/4] 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: <787197f2-c9d2-4ec9-8cac-99c8bbaecdcf@lunn.ch>
Date: Wed, 9 Jul 2025 00:23:16 +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 v2 3/4] net: renesas: rswitch: add offloading for L2
 switching

> +void rswitch_update_l2_offload(struct rswitch_private *priv)
> +{
> +	bool learning_needed, forwarding_needed;
> +	unsigned int all_ports_mask, fwd_mask;
> +	struct rswitch_device *rdev;
> +
> +	/* calculate fwd_mask with zeroes in bits corresponding to ports that
> +	 * shall participate in hardware forwarding
> +	 */
> +	all_ports_mask = GENMASK(RSWITCH_NUM_AGENTS - 1, 0);
> +	fwd_mask = all_ports_mask;
> +
> +	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)) {
> +			learning_needed = rdev->learning_requested;
> +			forwarding_needed = rdev->forwarding_requested;
> +		} else {
> +			learning_needed = false;
> +			forwarding_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");
> +		}
> +
> +		if (forwarding_needed) {
> +			/* 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");
> +		}
> +	}
> +}

This function seems overly complicated.

Normally you can change a ports STP state without needing to consider
other ports. Can you separate STP from joining ports to a bridge? That
might help simplify this function, break it up into two functions.

> +static int rswitch_port_obj_add(struct net_device *ndev, const void *ctx,
> +				const struct switchdev_obj *obj,
> +				struct netlink_ext_ack *extack)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int rswitch_port_obj_del(struct net_device *ndev, const void *ctx,
> +				const struct switchdev_obj *obj)
> +{
> +	return -EOPNOTSUPP;
> +}

> +static int rswitch_switchdev_blocking_event(struct notifier_block *nb,
> +					    unsigned long event, void *ptr)
> +{
> +	struct net_device *ndev = switchdev_notifier_info_to_dev(ptr);
> +	int ret;
> +
> +	switch (event) {
> +	case SWITCHDEV_PORT_OBJ_ADD:
> +		ret = switchdev_handle_port_obj_add(ndev, ptr,
> +						    rswitch_port_check,
> +						    rswitch_port_obj_add);
> +		break;
> +	case SWITCHDEV_PORT_OBJ_DEL:
> +		ret = switchdev_handle_port_obj_del(ndev, ptr,
> +						    rswitch_port_check,
> +						    rswitch_port_obj_del);
> +		break;

Since these two are hard coded to return EOPNOTSUPP, it seems like you
could just return that here?

>  /* Forwarding engine block (MFWD) */
> -static void rswitch_fwd_init(struct rswitch_private *priv)
> +static int rswitch_fwd_init(struct rswitch_private *priv)
>  {
> +	/* Initialize MAC hash table */
> +	iowrite32(FWMACTIM_MACTIOG, priv->addr + FWMACTIM);
> +	ret = rswitch_reg_wait(priv->addr, FWMACTIM, FWMACTIM_MACTIOG, 0);
> +
> +	return ret;

Just

	return rswitch_reg_wait(priv->addr, FWMACTIM, FWMACTIM_MACTIOG, 0);

	Andrew