Re: [net-next v3 5/6] net: marvell: prestera: Add Switchdev driver implementation

[Jiri Pirko <jiri@resnulli.us>]

Sat, Jul 25, 2020 at 05:06:50PM CEST, vadym.kochan@plvision.eu wrote:
>The following features are supported:
>
>    - VLAN-aware bridge offloading
>    - VLAN-unaware bridge offloading
>    - FDB offloading (learning, ageing)
>    - Switchport configuration
>
>Currently there are some limitations like:
>
>    - Only 1 VLAN-aware bridge instance supported
>    - FDB ageing timeout parameter is set globally per device
>
>Signed-off-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
>Signed-off-by: Serhiy Pshyk <serhiy.pshyk@plvision.eu>
>Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu>
>Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
>---

[...]


>+static void prestera_fdb_event_work(struct work_struct *work)
>+{
>+	struct switchdev_notifier_fdb_info *fdb_info;
>+	struct prestera_fdb_event_work *swdev_work;
>+	struct prestera_port *port;
>+	struct net_device *dev;
>+	int err = 0;
>+
>+	swdev_work = container_of(work, struct prestera_fdb_event_work, work);
>+	dev = swdev_work->dev;
>+
>+	rtnl_lock();
>+
>+	port = prestera_port_dev_lower_find(dev);
>+	if (!port)
>+		goto out;
>+
>+	switch (swdev_work->event) {
>+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
>+		fdb_info = &swdev_work->fdb_info;
>+		if (!fdb_info->added_by_user)
>+			break;
>+
>+		err = prestera_port_fdb_set(port, fdb_info, true);
>+		if (err)
>+			break;
>+
>+		prestera_fdb_offload_notify(port, fdb_info);
>+		break;
>+
>+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>+		fdb_info = &swdev_work->fdb_info;
>+		prestera_port_fdb_set(port, fdb_info, false);
>+		break;
>+	}
>+
>+out:
>+	rtnl_unlock();
>+
>+	kfree(swdev_work->fdb_info.addr);
>+	kfree(swdev_work);
>+	dev_put(dev);
>+}
>+
>+static int prestera_switchdev_event(struct notifier_block *unused,
>+				    unsigned long event, void *ptr)
>+{
>+	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
>+	struct switchdev_notifier_fdb_info *fdb_info;
>+	struct switchdev_notifier_info *info = ptr;
>+	struct prestera_fdb_event_work *swdev_work;
>+	struct net_device *upper;
>+	int err = 0;
>+
>+	if (event == SWITCHDEV_PORT_ATTR_SET) {
>+		err = switchdev_handle_port_attr_set(dev, ptr,
>+						     prestera_netdev_check,
>+						     prestera_port_obj_attr_set);
>+		return notifier_from_errno(err);
>+	}
>+
>+	upper = netdev_master_upper_dev_get_rcu(dev);
>+	if (!upper)
>+		return NOTIFY_DONE;
>+
>+	if (!netif_is_bridge_master(upper))
>+		return NOTIFY_DONE;

Okay, you support upper bridge. Of which interface? I believe you should
put prestera_netdev_check(dev) check here and avoid the lookup in the
work. Otherwise any chain of intermediate lower devices would be
supported, which is wrong.


>+
>+	swdev_work = kzalloc(sizeof(*swdev_work), GFP_ATOMIC);
>+	if (!swdev_work)
>+		return NOTIFY_BAD;
>+
>+	swdev_work->event = event;
>+	swdev_work->dev = dev;
>+
>+	switch (event) {
>+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
>+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>+		fdb_info = container_of(info,
>+					struct switchdev_notifier_fdb_info,
>+					info);
>+
>+		INIT_WORK(&swdev_work->work, prestera_fdb_event_work);
>+		memcpy(&swdev_work->fdb_info, ptr,
>+		       sizeof(swdev_work->fdb_info));
>+
>+		swdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
>+		if (!swdev_work->fdb_info.addr)
>+			goto out;
>+
>+		ether_addr_copy((u8 *)swdev_work->fdb_info.addr,
>+				fdb_info->addr);
>+		dev_hold(dev);
>+
>+		break;
>+
>+	default:
>+		kfree(swdev_work);
>+		return NOTIFY_DONE;
>+	}
>+
>+	queue_work(swdev_wq, &swdev_work->work);
>+	return NOTIFY_DONE;
>+out:
>+	kfree(swdev_work);
>+	return NOTIFY_BAD;
>+}

[...]