Message-ID: <20220523230834.4jv2kyg6eqx5rmi3@skbuf>
Date:   Mon, 23 May 2022 23:08:35 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Florian Fainelli <f.fainelli@...il.com>
CC:     Vladimir Oltean <olteanv@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Marek Behún <kabel@...nel.org>,
        Ansuel Smith <ansuelsmth@...il.com>,
        DENG Qingfang <dqfext@...il.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Frank Wunderlich <frank-w@...lic-files.de>
Subject: Re: [RFC PATCH net-next 10/12] net: dsa: allow the DSA master to be
 seen and changed through rtnetlink

On Mon, May 23, 2022 at 11:41:45AM -0700, Florian Fainelli wrote:
> > +static int dsa_changelink(struct net_device *dev, struct nlattr *tb[],
> > +			  struct nlattr *data[],
> > +			  struct netlink_ext_ack *extack)
> > +{
> > +	int err;
> > +
> > +	if (!data)
> > +		return 0;
> > +
> > +	if (data[IFLA_DSA_MASTER]) {
> 
> We could add a comment to explain that IFLA_LINK is "reserved" for standard
> usage of associating the DSA device with a different upper type, like VLAN,
> bridge master etc.

TBH I don't have a very strong opinion here. IFLA_LINK does not mean the
same thing for all virtual netdevices, it means one thing for vlan/macvlan
where it describes an upper/lower relationship and another for veth
where it describes the pair, and yet another for DSA where it describes
the host port.

What seems to be universally loved about IFLA_LINK is that the notation
"eth0@...1" used by iproute2 is cute, it lets loose users' imagination.

I did ask here whether it would be good to introduce a more specific
attribute, no response though.
https://lore.kernel.org/netdev/20210411170939.cxmva5vdcpqu4bmi@skbuf/

If the IFLA_LINK meaning is namespaced per netdev kind, I suppose we
could reuse that just fine to change the DSA master. In any case I
wouldn't want to make the debate of the century out of this.

> > +		u32 ifindex = nla_get_u32(data[IFLA_DSA_MASTER]);
> > +		struct net_device *master;
> > +
> > +		master = __dev_get_by_index(dev_net(dev), ifindex);
> > +		if (!master)
> > +			return -EINVAL;
> > +
> > +		err = dsa_slave_change_master(dev, master, extack);
> > +		if (err)
> > +			return err;
> > +	}
> 
> I would be tempted to reduce the indentation here because we are almost
> guaranteed to add code in that conditional section?

The idea was to avoid code movement if we ever add other netlink
attributes other than IFLA_DSA_MASTER. But not sure whether to optimize
for that.

> [snip]
> 
> > +static int dsa_port_assign_master(struct dsa_port *dp,
> > +				  struct net_device *master,
> > +				  struct netlink_ext_ack *extack,
> > +				  bool fail_on_err)
> > +{
> > +	struct dsa_switch *ds = dp->ds;
> > +	int port = dp->index, err;
> > +
> > +	err = ds->ops->port_change_master(ds, port, master, extack);
> > +	if (err && !fail_on_err)
> > +		dev_err(ds->dev, "port %d failed to assign master %s: %pe\n",
> > +			port, master->name, ERR_PTR(err));
> 
> Should not that go over extack instead?

Here we print if "fail_on_err" was false. We avoid failing on errors
when we are in an error rollback code path. This is also the reason why
I did not set extack, presumably because it may have been set before by
ds->ops->port_change_master. Printing to the console shows all errors
along the path, setting the extack shows only the first, or last, error.

> > +
> > +	if (err && fail_on_err)
> > +		return err;
> > +
> > +	dp->cpu_dp = master->dsa_ptr;
> > +
> > +	return 0;
> > +}
> > +
> > +/* Change the dp->cpu_dp affinity for a user port. Note that both cross-chip
> > + * notifiers and drivers have implicit assumptions about user-to-CPU-port
> > + * mappings, so we unfortunately cannot delay the deletion of the objects
> > + * (switchdev, standalone addresses, standalone VLANs) on the old CPU port
> > + * until the new CPU port has been set up. So we need to completely tear down
> > + * the old CPU port before changing it, and restore it on errors during the
> > + * bringup of the new one.
> > + */
> > +int dsa_port_change_master(struct dsa_port *dp, struct net_device *master,
> > +			   struct netlink_ext_ack *extack)
> > +{
> > +	struct net_device *bridge_dev = dsa_port_bridge_dev_get(dp);
> > +	struct net_device *old_master = dsa_port_to_master(dp);
> > +	struct net_device *dev = dp->slave;
> > +	struct dsa_switch *ds = dp->ds;
> > +	int port = dp->index;
> > +	bool vlan_filtering;
> > +	int err, tmp;
> > +
> > +	/* Bridges may hold host FDB, MDB and VLAN objects. These need to be
> > +	 * migrated, so dynamically unoffload and later reoffload the bridge
> > +	 * port.
> > +	 */
> > +	if (bridge_dev) {
> > +		dsa_port_pre_bridge_leave(dp, bridge_dev);
> > +		dsa_port_bridge_leave(dp, bridge_dev);
> > +	}
> > +
> > +	/* The port might still be VLAN filtering even if it's no longer
> > +	 * under a bridge, either due to ds->vlan_filtering_is_global or
> > +	 * ds->needs_standalone_vlan_filtering. In turn this means VLANs
> > +	 * on the CPU port.
> > +	 */
> > +	vlan_filtering = dsa_port_is_vlan_filtering(dp);
> > +	if (vlan_filtering) {
> > +		err = dsa_slave_manage_vlan_filtering(dev, false);
> > +		if (err) {
> > +			dev_err(ds->dev,
> > +				"port %d failed to remove standalone VLANs: %pe\n",
> > +				port, ERR_PTR(err));
> 
> Likewise, should not that be via extack? And likewise for pretty much any
> message down below.

Here we could populate the extack.

> [snip]
> 
> > +	if (!ds->ops->port_change_master)
> > +		return -EOPNOTSUPP;
> 
> This could be provided over extactk since it is not even supposed to be
> happening.

What do you mean it's not supposed to be happening? This is the only
place where we have a NULL check for ds->ops->port_change_master.
I didn't add an extack here because I didn't think there's much to say
beside the usual strerror(EOPNOTSUPP) = "Operation not supported".
I may add an extack saying "Driver does not support changing DSA master"
or some sort of message like that.

Powered by blists - more mailing lists