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

[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 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.

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