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

[Simon Horman <horms@kernel.org>]

On Tue, Jul 08, 2025 at 11:27:39AM +0200, 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: Michael Dege <michael.dege@renesas.com>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>

...

> diff --git a/drivers/net/ethernet/renesas/rswitch_l2.c b/drivers/net/ethernet/renesas/rswitch_l2.c

...

> +static void rswitch_update_offload_brdev(struct rswitch_private *priv,
> +					 bool force_update_l2_offload)
> +{
> +	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) {
> +		if (offload_brdev && force_update_l2_offload)
> +			rswitch_update_l2_offload(priv);
> +		return;
> +	}
> +
> +	if (offload_brdev && !priv->offload_brdev)
> +		dev_dbg(&priv->pdev->dev, "starting l2 offload for %s\n",
> +			netdev_name(offload_brdev));
> +	else if (!offload_brdev && priv->offload_brdev)
> +		dev_dbg(&priv->pdev->dev, "stopping l2 offload for %s\n",
> +			netdev_name(priv->offload_brdev));
> +	else
> +		dev_dbg(&priv->pdev->dev,
> +			"changing l2 offload from %s to %s\n",
> +			netdev_name(priv->offload_brdev),
> +			netdev_name(offload_brdev));

Smatch flags a false-positive about possible NULL references by the
netdev_name() calls on the line above.

Due to the previous if statement it seems to me that cannot occur.
But it did take me a few moments to convince myself of that.

So while I don't think we should write our code to static analysis tooling.
I did play around a bit to see if I could come up with something that is
both easier on the eyes and keeps Smatch happy.

Perhaps it isn't easier on the eyes, but rather I'm just more familiar with
the code now. But in any case, I'm sharing what I came up with in case it
is useful. (Compile tested only!).


        if (!offload_brdev && !priv->offload_brdev)
                return;

        if (!priv->offload_brdev)
                dev_dbg(&priv->pdev->dev, "starting l2 offload for %s\n",
                        netdev_name(offload_brdev));
        else if (!offload_brdev)
                dev_dbg(&priv->pdev->dev, "stopping l2 offload for %s\n",
                        netdev_name(priv->offload_brdev));
        else if (offload_brdev != priv->offload_brdev)
                dev_dbg(&priv->pdev->dev,
                        "changing l2 offload from %s to %s\n",
                        netdev_name(priv->offload_brdev),
                        netdev_name(offload_brdev));
        else if (!force_update_l2_offload)
                return;

> +
> +	priv->offload_brdev = offload_brdev;
> +
> +	rswitch_update_l2_offload(priv);
> +}

...