Re: [PATCH net-next v5 06/14] net: renesas: rswitch: add forwarding rules for gwca

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: renesas: rswitch: add forwarding rules for gwca
    
This commit adds rules to forward packets from the Ethernet ports to the
CPU port (GWCA) using L2 forwarding. It introduces a new function to
configure GWCA forwarding rules and integrates it into the L2 offload
updates.

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

[Severity: Medium]
The commit message mentions using L2 forwarding instead of port forwarding,
but is the existing port forwarding code actually removed or disabled?

If rswitch_fwd_init() still configures FWPBFC to unconditionally forward all
packets to the GWCA port, does having both L2 forwarding and port-based
forwarding active simultaneously lead to duplicated packets and unnecessary
CPU overhead?

> diff --git a/drivers/net/ethernet/renesas/rswitch_l2.c b/drivers/net/ethernet/renesas/rswitch_l2.c
> index 709524c8a5c4d..3cc0ce8762f3c 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_clr = 0);
> +
> +	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);

[Severity: Medium]
Do non-atomic updates to FWPC2 momentarily disable forwarding to the CPU port?

In rswitch_update_l2_hw_forwarding(), iowrite32() is used to overwrite
FWPC2, which unconditionally sets the GWCA bit to 1, disabling hardware L2
forwarding to the CPU port.

Immediately after, rswitch_update_l2_hw_forwarding_gwca() uses
rswitch_modify() to clear the GWCA bit back to 0.

Does this consecutive write sequence create a microsecond window where L2
forwarding to the CPU is incorrectly denied, risking packet drops for
hardware packets destined for the CPU?

>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential kernel panic in
rswitch_update_offload_brdev() when logging is enabled:

drivers/net/ethernet/renesas/rswitch_l2.c:rswitch_update_offload_brdev() {
    ...
    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));
    ...
}

If a single port is added to a bridge, the loop searching for a shared
bridge exits early, leaving offload_brdev as NULL.

If priv->offload_brdev is also NULL (the initial state), the condition
offload_brdev == priv->offload_brdev evaluates to true.

Will this cause a kernel panic when dynamic debugging is active, since
netdev_name(NULL) unconditionally dereferences its argument?

[Severity: High]
This is a pre-existing issue, but do concurrent transmissions cause lost
writes to the shared GWTRC register?

In rswitch_start_xmit(), transmission is triggered by modifying the GWTRC
register:

drivers/net/ethernet/renesas/rswitch_main.c:rswitch_start_xmit() {
    ...
    rswitch_modify(priv->addr, GWTRC, ...);
    ...
}

Because rswitch_modify() performs a lockless read-modify-write:

drivers/net/ethernet/renesas/rswitch_main.c:rswitch_modify() {
    iowrite32((ioread32(addr + reg) & ~clear) | set, addr + reg);
}

Will concurrent ndo_start_xmit calls from different CPUs on different
ports race and overwrite each other's modifications, resulting in lost
transmit triggers and stalled TX queues?

[Severity: High]
This is a pre-existing issue, but does the driver fail to set
skb->offload_fwd_mark for hardware-forwarded L2 packets?

The driver supports hardware L2 forwarding but does not appear to set
skb->offload_fwd_mark = 1 before passing received packets to the network
stack.

Without this flag, the software bridge is unaware that the hardware has
already flooded the packet to other ports.

Will this cause the bridge to re-flood broadcast and multicast packets,
leading to packet duplication and potential network storms?