Re: [PATCH v3 3/4] net: renesas: rswitch: add offloading for L2 switching

[Paolo Abeni <pabeni@redhat.com>]

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@cogentembedded.com>
> Signed-off-by: Michael Dege <michael.dege@renesas.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.

> +		}
> +	}
> +}
> +
> +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.

> +		}
> +	}
> +}
> +
> +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