RE: [net-next,v3,12/13] net: renesas: rswitch: add handler for FDB notification

[Michael Dege]

Hello Paolo,

Thank you for your comments.

> -----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,12/13] net: renesas: rswitch: add handler for FDB notification
>
> 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%7C6a613b4020df4903a3c408de90b7ba4f%7C53d82571da19
> 47e49cb4625a166a4a2a%7C0%7C0%7C639107315963062459%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=NSEhHV2iBtsRceQP2J
> ZppzzmHcbTZ%2FrvCbXxomHOUmQ%3D&reserved=0
> ---
> net: renesas: rswitch: add handler for FDB notification
>
> The SWITCHDEV_FDB_ADD_TO_DEVICE notification writes the bridge MAC address to the GWCA and adds a
> static rule to the MAC table. The SWITCHDEV_FDB_DEL_TO_DEVICE removes the static rule from the MAC
> table.
>
> > diff --git a/drivers/net/ethernet/renesas/rswitch.h
> > b/drivers/net/ethernet/renesas/rswitch.h
> > index 6dfc53c6b985..443fc4e49602 100644
> > --- a/drivers/net/ethernet/renesas/rswitch.h
> > +++ b/drivers/net/ethernet/renesas/rswitch.h
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/renesas/rswitch_l2.c
> > b/drivers/net/ethernet/renesas/rswitch_l2.c
> > index f1c5c96027ba..ea95a87ed234 100644
> > --- a/drivers/net/ethernet/renesas/rswitch_l2.c
> > +++ b/drivers/net/ethernet/renesas/rswitch_l2.c
>
> [ ... ]
>
> > @@ -285,6 +269,181 @@ static int rswitch_switchdev_blocking_event(struct notifier_block *nb,
> >     return notifier_from_errno(ret);
> >  }
> >
> > +static int rswitch_gwca_write_mac_address(struct rswitch_private
> > +*priv, const u8 *mac) {
> > +   int err;
> > +
> > +   err = rswitch_gwca_change_mode(priv, GWMC_OPC_CONFIG);
> > +   if (err < 0)
> > +           return err;
> > +
> > +   iowrite32((mac[0] << 8) | mac[1], priv->addr + GWMAC0);
> > +   iowrite32((mac[2] << 24) | (mac[3] << 16) | (mac[4] << 8) | mac[5],
> > +             priv->addr + GWMAC1);
> > +
> > +   return rswitch_gwca_change_mode(priv, GWMC_OPC_OPERATION); }
> > +
> > +static int rswitch_add_addr_to_mactable(struct rswitch_private *priv,
> > +const u8 *mac) {
> > +   u32 index = priv->gwca.l2_shared_rx_queue->index;
> > +   int err;
> > +
> > +   rswitch_modify(priv->addr, FWMACTL0, FWMACTL0_ED, 0);
> > +   iowrite32((mac[0] << 8) | mac[1], priv->addr + FWMACTL1);
> > +   iowrite32((mac[2] << 24) | (mac[3] << 16) | (mac[4] << 8) | mac[5],
> > +             priv->addr + FWMACTL2);
> > +   iowrite32(FWMACTL3_DSLV | FWMACTL3_SSLV,
> > +             priv->addr + FWMACTL3);
> > +   iowrite32(FIELD_PREP(FWMACTL4_CSDL, index),
> > +             priv->addr + FWMACTL4(GWCA_INDEX));
> > +   iowrite32(FIELD_PREP(FWMACTL5_DV, BIT(AGENT_INDEX_GWCA)),
> > +             priv->addr + FWMACTL5);
> > +
> > +   err = rswitch_reg_wait(priv->addr, FWMACTLR, FWMACTLR_L, 0);
> > +   if (err < 0)
> > +           return err;
> > +
> > +   if (ioread32(priv->addr + FWMACTLR))
> > +           return NOTIFY_BAD;
> > +
> > +   return NOTIFY_DONE;
> > +}
> > +
> > +static int rswitch_del_addr_from_mactable(struct rswitch_private
> > +*priv, const u8 *mac) {
> > +   int err;
> > +
> > +   rswitch_modify(priv->addr, FWMACTL0, 0, FWMACTL0_ED);
> > +   iowrite32((mac[0] << 8) | mac[1], priv->addr + FWMACTL1);
> > +   iowrite32((mac[2] << 24) | (mac[3] << 16) | (mac[4] << 8) | mac[5],
> > +             priv->addr + FWMACTL2);
> > +   iowrite32(FWMACTL3_DSLV | FWMACTL3_SSLV,
> > +             priv->addr + FWMACTL3);
> > +   iowrite32(FIELD_PREP(FWMACTL4_CSDL, BIT(0)),
> > +             priv->addr + FWMACTL4(GWCA_INDEX));
> > +   iowrite32(FIELD_PREP(FWMACTL5_DV, BIT(AGENT_INDEX_GWCA)),
> > +             priv->addr + FWMACTL5);
> > +
> > +   err = rswitch_reg_wait(priv->addr, FWMACTLR, FWMACTLR_L, 0);
> > +   if (err < 0)
> > +           return err;
> > +
> > +   err = ioread32(priv->addr + FWMACTLR);
> > +   if (err)
> > +           return NOTIFY_BAD;
> > +
> > +   return NOTIFY_DONE;
> > +}
> > +
> > +static void rswitch_switchdev_bridge_fdb_event_work(struct
> > +work_struct *work) {
> > +   struct rswitch_switchdev_event_work *switchdev_work;
> > +   struct rswitch_device *rdev;
> > +   struct net_device *ndev;
> > +
> > +   switchdev_work = container_of(work, struct rswitch_switchdev_event_work, work);
> > +   ndev = switchdev_work->ndev;
> > +
> > +   rtnl_lock();
> > +
> > +   /* Unfortunately all net_device members point to br0, there is no simple way to check
> > +    * if the event was triggered by a port device setting.
> > +    */
> > +   rswitch_for_all_ports(switchdev_work->priv, rdev) {
> > +           if (ether_addr_equal(rdev->ndev->dev_addr, switchdev_work->fdb_info.addr))
> > +                   goto out;
> > +   }
> > +
> > +   /* Handle only bridge device */
> > +   if (is_rdev(ndev))
> > +           goto out;
> > +
> > +   switch (switchdev_work->event) {
> > +   case SWITCHDEV_FDB_ADD_TO_DEVICE:
> > +           rswitch_gwca_write_mac_address(switchdev_work->priv, switchdev_work->fdb_info.addr);
> > +           rswitch_add_addr_to_mactable(switchdev_work->priv, switchdev_work->fdb_info.addr);
> > +           break;
> > +   case SWITCHDEV_FDB_DEL_TO_DEVICE:
> > +           rswitch_del_addr_from_mactable(switchdev_work->priv, switchdev_work->fdb_info.addr);
> > +           break;
> > +   default:
> > +           break;
> > +   }
> > +
> > +out:
> > +   rtnl_unlock();
> > +
> > +   kfree(switchdev_work->fdb_info.addr);
> > +   kfree(switchdev_work);
> > +   dev_put(ndev);
> > +}
> > +
> > +/* called under rcu_read_lock() */
> > +static int rswitch_switchdev_event(struct notifier_block *nb,
> > +                              unsigned long event,
> > +                              void *ptr)
> > +{
> > +   struct net_device *ndev = switchdev_notifier_info_to_dev(ptr);
> > +   struct rswitch_switchdev_event_work *switchdev_work;
> > +   struct switchdev_notifier_fdb_info *fdb_info;
> > +   struct switchdev_notifier_info *info = ptr;
> > +   struct rswitch_private *priv;
> > +   int err;
> > +
> > +   priv = container_of(nb, struct rswitch_private,
> > +rswitch_switchdev_nb);
> > +
> > +   switch (event) {
> > +   case SWITCHDEV_FDB_ADD_TO_DEVICE:
> > +           fallthrough;
> > +   case SWITCHDEV_FDB_DEL_TO_DEVICE:
> > +           switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
> > +
> > +           if (!switchdev_work)
> > +                   return NOTIFY_BAD;
> > +
> > +           switchdev_work->ndev = info->dev;
> > +           switchdev_work->priv = priv;
> > +           switchdev_work->event = event;
> > +
> > +           fdb_info = container_of(info,
> > +                                   struct switchdev_notifier_fdb_info,
> > +                                   info);
> > +
> > +           INIT_WORK(&switchdev_work->work,
> > +rswitch_switchdev_bridge_fdb_event_work);
> > +
> > +           memcpy(&switchdev_work->fdb_info, ptr,
> > +sizeof(switchdev_work->fdb_info));
> > +
> > +           switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
> > +           if (!switchdev_work->fdb_info.addr)
> > +                   goto err_addr_alloc;
> > +
> > +           ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
> > +                           fdb_info->addr);
> > +           dev_hold(ndev);
> > +           queue_work(system_long_wq, &switchdev_work->work);
> > +
> > +           break;
> > +   case SWITCHDEV_PORT_ATTR_SET:
> > +           err = switchdev_handle_port_attr_set(ndev, ptr,
> > +                                                is_rdev,
> > +                                                rswitch_port_attr_set);
> > +           return notifier_from_errno(err);
> > +
> > +           if (!is_rdev(ndev))
> > +                   return NOTIFY_DONE;
> > +
> > +           return notifier_from_errno(-EOPNOTSUPP);
>
> Are these lines in the SWITCHDEV_PORT_ATTR_SET case reachable? The return notifier_from_errno(err)
> above them looks like it returns unconditionally.
>
> Note: this dead code is removed in the next commit in the series
> (ed3ab4a0a727 'net: renesas: rswitch: add vlan aware switching'), which moves PORT_ATTR_SET handling
> to rswitch_switchdev_blocking_event and removes this entire case from rswitch_switchdev_event.

I moved this to this patch. Thereby removing the dead code.

Best regards,

Michael
>
> > +   }
> > +
> > +   return NOTIFY_DONE;
> > +
> > +err_addr_alloc:
> > +   kfree(switchdev_work);
> > +
> > +   return NOTIFY_BAD;
> > +}
> > +
> >  int rswitch_register_notifiers(struct rswitch_private *priv)  {
> >     int err;