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

[Michael Dege]

Hello Paolo,

Thank you very much for your review.

> -----Original Message-----
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: Thursday, April 2, 2026 3:00 PM
> To: Michael Dege <michael.dege@xxxxxxxxxxx>
> Cc: paul@xxxxxxxxxxx; niklas.soderlund <niklas.soderlund@xxxxxxxxxxxx>; kuba@xxxxxxxxxx; Yoshihiro
> Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx; andrew+netdev@xxxxxxx;
> davem@xxxxxxxxxxxxx
> Subject: Re: [net-next,v3,06/13] net: renesas: rswitch: add forwarding rules for gwca
>
> 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&data=05%7C02%7Cmichael.dege%40renesas.com%7Cce95b7b1d9df4483358208de90b7b638%7C53d82571da19
> 47e49cb4625a166a4a2a%7C0%7C0%7C639107315865778020%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=dee%2F%2Bmcpl10Pk%
> 2BHTcL0u75PN%2F4FFyv9xB2kWqnwj%2B9Q%3D&reserved=0
> ---
> 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.

Good catch, this is indeed a copy paste error. I fixed it for the next version.

Best regards,

Michael
>
> > +
> > +   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);
> >  }
>
> [ ... ]