Re: [net-next,v3,06/13] net: renesas: rswitch: add forwarding rules for gwca

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: renesas: rswitch: add forwarding rules for gwca

Add rules to forward packets from the Ethernet ports to the CPU port (GWCA)
using L2 forwarding instead of port forwarding.

> diff --git a/drivers/net/ethernet/renesas/rswitch_l2.c b/drivers/net/ethernet/renesas/rswitch_l2.c
> index 709524c8a5c4..cabf1f92ad0c 100644
> --- a/drivers/net/ethernet/renesas/rswitch_l2.c
> +++ b/drivers/net/ethernet/renesas/rswitch_l2.c
> @@ -93,10 +93,32 @@ static void rswitch_update_l2_hw_forwarding(struct rswitch_private *priv)
>  	}
>  }
>
> +static void rswitch_update_l2_hw_forwarding_gwca(struct rswitch_private *priv)
> +{
> +	struct rswitch_device *rdev;
> +	u32 fwpc0_set, fwpc0_clr, fwpc2_set, fwpc2_clr;
> +
> +	fwpc0_clr = FWPC0_MACSSA | FWPC0_MACDSA | FWPC0_MACRUDA;
> +	fwpc0_set = fwpc0_clr;
> +	fwpc2_clr = FIELD_PREP(FWPC2_LTWFW, BIT(AGENT_INDEX_GWCA));
> +	fwpc2_set = fwpc2_clr;
> +
> +	(priv->offload_brdev) ? (fwpc0_clr = 0, fwpc2_set = 0)
> +			      : (fwpc0_set = 0, fwpc2_set = 0);
>                                             ^^^^^^^^^^^

Does the false branch have a copy-paste error? Looking at the ternary
operator:

The TRUE branch (when offload_brdev is set) assigns:
    fwpc0_clr = 0
    fwpc2_set = 0

The FALSE branch (when offload_brdev is NULL) assigns:
    fwpc0_set = 0
    fwpc2_set = 0   <- assigns to fwpc2_set

Both branches assign to fwpc2_set, leaving fwpc2_clr unchanged with the
initialized value FIELD_PREP(FWPC2_LTWFW, BIT(AGENT_INDEX_GWCA)).

This appears to break the logic for non-offloaded mode. The FALSE branch
passes fwpc2_clr with the GWCA bit set to rswitch_modify(), which clears
the GWCA bit from the FWPC2 registers. Is this the intended behavior when
bridge offload is disabled?

The symmetric pattern in the TRUE branch suggests the FALSE branch should
be (fwpc0_set = 0, fwpc2_clr = 0) instead.

> +
> +	rswitch_modify(priv->addr, FWPC0(AGENT_INDEX_GWCA), fwpc0_clr, fwpc0_set);
> +
> +	rswitch_for_all_ports(priv, rdev) {
> +		rswitch_modify(priv->addr, FWPC2(rdev->etha->index),
> +			       fwpc2_clr, fwpc2_set);
> +	}
> +}
> +
>  void rswitch_update_l2_offload(struct rswitch_private *priv)
>  {
>  	rswitch_update_l2_hw_learning(priv);
>  	rswitch_update_l2_hw_forwarding(priv);
> +	rswitch_update_l2_hw_forwarding_gwca(priv);
>  }

[ ... ]