Date:   Mon, 23 May 2022 11:41:45 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org
Cc:     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,
        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>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [RFC PATCH net-next 10/12] net: dsa: allow the DSA master to be
 seen and changed through rtnetlink

On 5/23/22 03:42, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> Some DSA switches have multiple CPU ports, which can be used to improve
> CPU termination throughput, but DSA, through dsa_tree_setup_cpu_ports(),
> sets up only the first one, leading to suboptimal use of hardware.
> 
> The desire is to not change the default configuration but to permit the
> user to create a dynamic mapping between individual user ports and the
> CPU port that they are served by, configurable through rtnetlink. It is
> also intended to permit load balancing between CPU ports, and in that
> case, the foreseen model is for the DSA master to be a bonding interface
> whose lowers are the physical DSA masters.
> 
> To that end, we create a struct rtnl_link_ops for DSA user ports with
> the "dsa" kind. We expose the IFLA_DSA_MASTER link attribute that
> contains the ifindex of the newly desired DSA master.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---

[snip]

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

> +		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?

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

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

[snip]

> +	if (!ds->ops->port_change_master)
> +		return -EOPNOTSUPP;

This could be provided over extactk since it is not even supposed to be 
happening.
-- 
Florian

Powered by blists - more mailing lists