Hi Ido,

On Thu, Mar 05, 2020 at 04:49:37PM +0200, Ido Schimmel wrote:

On Tue, Feb 25, 2020 at 04:30:54PM +0000, Vadym Kochan wrote:

quoted

+int mvsw_pr_port_learning_set(struct mvsw_pr_port *port, bool learn)
+{
+	return mvsw_pr_hw_port_learning_set(port, learn);
+}
+
+int mvsw_pr_port_flood_set(struct mvsw_pr_port *port, bool flood)
+{
+	return mvsw_pr_hw_port_flood_set(port, flood);
+}

Flooding and learning are per-port attributes? Not per-{port, VLAN} ?
If so, you need to have various restrictions in the driver in case
someone configures multiple vlan devices on top of a port and enslaves
them to different bridges.

quoted

+
+
+	INIT_LIST_HEAD(&port->vlans_list);
+	port->pvid = MVSW_PR_DEFAULT_VID;

If you're using VID 1, then you need to make sure that user cannot
configure a VLAN device with with this VID. If possible, I suggest that
you use VID 4095, as it cannot be configured from user space.

I'm actually not entirely sure why you need a default VID.

quoted

+mvsw_pr_port_vlan_bridge_join(struct mvsw_pr_port_vlan *port_vlan,
+			      struct mvsw_pr_bridge_port *br_port,
+			      struct netlink_ext_ack *extack)
+{
+	struct mvsw_pr_port *port = port_vlan->mvsw_pr_port;
+	struct mvsw_pr_bridge_vlan *br_vlan;
+	u16 vid = port_vlan->vid;
+	int err;
+
+	if (port_vlan->bridge_port)
+		return 0;
+
+	err = mvsw_pr_port_flood_set(port, br_port->flags & BR_FLOOD);
+	if (err)
+		return err;
+
+	err = mvsw_pr_port_learning_set(port, br_port->flags & BR_LEARNING);
+	if (err)
+		goto err_port_learning_set;

It seems that learning and flooding are not per-{port, VLAN} attributes,
so I'm not sure why you have this here.

The fact that you don't undo this in mvsw_pr_port_vlan_bridge_leave()
tells me it should not be here.

 > +

quoted

+void
+mvsw_pr_port_vlan_bridge_leave(struct mvsw_pr_port_vlan *port_vlan)
+{
+	struct mvsw_pr_port *port = port_vlan->mvsw_pr_port;
+	struct mvsw_pr_bridge_vlan *br_vlan;
+	struct mvsw_pr_bridge_port *br_port;
+	int port_count;
+	u16 vid = port_vlan->vid;
+	bool last_port, last_vlan;
+
+	br_port = port_vlan->bridge_port;
+	last_vlan = list_is_singular(&br_port->vlan_list);
+	port_count =
+	    mvsw_pr_bridge_vlan_port_count_get(br_port->bridge_device, vid);
+	br_vlan = mvsw_pr_bridge_vlan_find(br_port, vid);
+	last_port = port_count == 1;
+	if (last_vlan) {
+		mvsw_pr_fdb_flush_port(port, MVSW_PR_FDB_FLUSH_MODE_DYNAMIC);
+	} else if (last_port) {
+		mvsw_pr_fdb_flush_vlan(port->sw, vid,
+				       MVSW_PR_FDB_FLUSH_MODE_DYNAMIC);
+	} else {
+		mvsw_pr_fdb_flush_port_vlan(port, vid,
+					    MVSW_PR_FDB_FLUSH_MODE_DYNAMIC);

If you always flush based on {port, VID}, then why do you need the other
two?

 > +

quoted

+static int mvsw_pr_port_obj_attr_set(struct net_device *dev,
+				     const struct switchdev_attr *attr,
+				     struct switchdev_trans *trans)
+{
+	int err = 0;
+	struct mvsw_pr_port *port = netdev_priv(dev);
+
+	switch (attr->id) {
+	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
+		err = -EOPNOTSUPP;

You don't support STP?

Not, yet. But it will be in the next submission or official patch.

quoted

+		break;

quoted

+	default:
+		kfree(switchdev_work);
+		return NOTIFY_DONE;
+	}
+
+	queue_work(mvsw_owq, &switchdev_work->work);

Once you defer the operation you cannot return an error, which is
problematic. Do you have a way to know if the operation will succeed or
not? That is, if the hardware has enough space for this new FDB entry?

Right, fdb configuration on via fw is blocking operation I still need to
think on it if it is possible by current design.

Why do you need both 'struct mvsw_pr_switchdev' and 'struct
mvsw_pr_bridge'? I think the second is enough. Also, I assume
'switchdev' naming is inspired by mlxsw, but 'bridge' is better.

I changed to use bridge for bridge object, because having bridge_device
may confuse.

Thank you for your comments they were very useful, sorry for so late
answer, I decided to re-implement this version a bit. Regarding flooding
and default vid I still need to check it.

Regards,
Vadym Kochan