Re: [net-next PATCH] net: dsa: qca8k: add support for port_change_master

[Christian Marangi]

On Tue, Jun 20, 2023 at 11:12:27PM +0300, Vladimir Oltean wrote:
> Hi Christian,
> 
> On Tue, Jun 20, 2023 at 08:37:47AM +0200, Christian Marangi wrote:
> > Add support for port_change_master to permit assigning an alternative
> > CPU port if the switch have both CPU port connected or create a LAG on
> > both CPU port and assign the LAG as DSA master.
> > 
> > On port change master request, we check if the master is a LAG.
> > With LAG we compose the cpu_port_mask with the CPU port in the LAG, if
> > master is a simple dsa_port, we derive the index.
> > 
> > Finally we apply the new cpu_port_mask to the LOOKUP MEMBER to permit
> > the port to receive packet by the new CPU port setup for the port and
> > we reenable the target port previously disabled.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > ---
> >  drivers/net/dsa/qca/qca8k-8xxx.c | 54 ++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/qca/qca8k.h      |  1 +
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> > index dee7b6579916..435b69c1c552 100644
> > --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> > +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> > @@ -1713,6 +1713,59 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
> >  	return DSA_TAG_PROTO_QCA;
> >  }
> >  
> > +static int qca8k_port_change_master(struct dsa_switch *ds, int port,
> > +				    struct net_device *master,
> > +				    struct netlink_ext_ack *extack)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> > +	u32 val, cpu_port_mask = 0;
> > +	struct dsa_port *dp;
> > +	int ret;
> > +
> > +	/* With LAG of CPU port, compose the mask for LOOKUP MEMBER */
> > +	if (netif_is_lag_master(master)) {
> > +		struct dsa_lag *lag;
> > +		int id;
> > +
> > +		id = dsa_lag_id(ds->dst, master);
> > +		lag = dsa_lag_by_id(ds->dst, id);
> > +
> > +		dsa_lag_foreach_port(dp, ds->dst, lag)
> 
> I think you use ds->dst often enough that you could assign it to its own
> local variable.
>

Will do thanks!

> > +			if (dsa_port_is_cpu(dp))
> > +				cpu_port_mask |= BIT(dp->index);
> > +	} else {
> > +		dp = dsa_port_from_netdev(master);
> 
> dsa_port_from_netdev() is implemented by calling:
> 
> static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
> {
> 	struct dsa_slave_priv *p = netdev_priv(dev);
> 
> 	return p->dp;
> }
> 
> The "struct net_device *master" does not have a netdev_priv() of the
> type "struct dsa_slave_priv *". So, this function does not do what you
> want, but instead it messes through the guts of an unrelated private
> structure, treating whatever it finds at offset 16 as a pointer, and
> dereferincing that as a struct dsa_port *. I'm surprised it didn't
> crash, to be frank.
> 
> To find the cpu_dp behind the master, you need to dereference
> master->dsa_ptr (for which we don't have a helper).
> 

I was searching for an helper but no luck. Is it safe to access
master->dsa_ptr? In theory the caller of port_change_master should
already check that the passed master is a dsa port?

I see in other context that master->dsa_ptr is checked if not NULL.
Should I do the same check here?

> > +		cpu_port_mask |= BIT(dp->index);
> > +	}
> > +
> > +	/* Disable port */
> > +	qca8k_port_set_status(priv, port, 0);
> > +
> > +	/* Connect it to new cpu port */
> > +	ret = qca8k_read(priv, QCA8K_PORT_LOOKUP_CTRL(port), &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Reset connected CPU port in LOOKUP MEMBER */
> > +	val &= QCA8K_PORT_LOOKUP_USER_MEMBER;
> 
> val &= GENMASK(5, 1) practically has the effect of unsetting BIT(0) and BIT(6).
> I suppose those are the 2 possible CPU ports? If so, then use ~dsa_cpu_ports(ds),
> it's more readable at least for me as a fallback maintainer.
> 

Yes they are and yes I love this so I can also drop the stupid define.

> > +	/* Assign the new CPU port in LOOKUP MEMBER */
> > +	val |= cpu_port_mask;
> > +
> > +	ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(port),
> > +			QCA8K_PORT_LOOKUP_MEMBER,
> > +			val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Fast Age the port to flush FDB table */
> > +	qca8k_port_fast_age(ds, port);
> 
> Why do you have to fast age the (user) port?
> 

The 2 CPU port have a different mac address, is it a problem?

> > +
> > +	/* Reenable port */
> > +	qca8k_port_set_status(priv, port, 1);
> 
> or disable/enable it, for that matter?
> 

The idea is sto stop any traffic flowing to one CPU to another before
doing the change.

> > +
> > +	return 0;
> > +}
> > +
> >  static void
> >  qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> >  		    bool operational)
> > @@ -1996,6 +2049,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.get_phy_flags		= qca8k_get_phy_flags,
> >  	.port_lag_join		= qca8k_port_lag_join,
> >  	.port_lag_leave		= qca8k_port_lag_leave,
> > +	.port_change_master	= qca8k_port_change_master,
> 
> From my notes in commit eca70102cfb1 ("net: dsa: felix: add support for
> changing DSA master"), I recall this:
> 
>     When we change the DSA master to a LAG device, DSA guarantees us that
>     the LAG has at least one lower interface as a physical DSA master.
>     But DSA masters can come and go as lowers of that LAG, and
>     ds->ops->port_change_master() will not get called, because the DSA
>     master is still the same (the LAG). So we need to hook into the
>     ds->ops->port_lag_{join,leave} calls on the CPU ports and update the
>     logical port ID of the LAG that user ports are assigned to.
> 
> Otherwise said:
> 
> $ ip link add bond0 type bond mode balance-xor && ip link set bond0 up
> $ ip link set eth0 down && ip link set eth0 master bond0 # .port_change_master() gets called
> $ ip link set eth1 down && ip link set eth1 master bond0 # .port_change_master() does not get called
> $ ip link set eth0 nomaster # .port_change_master() does not get called
> 
> Unless something has changed, I believe that you need to handle these as well,
> and update the QCA8K_PORT_LOOKUP_MEMBER field. In the case above, your
> CPU port association would remain towards eth0, but the bond's lower interface
> is eth1.
> 

Can you better describe this case?

In theory from the switch view, with a LAG we just set that an user port
can receive packet from both CPU port.

Or you are saying that when an additional memeber is added to the LAG,
port_change_master is not called and we could face a scenario where:

- dsa master is LAG
- LAG have the 2 CPU port
- user port have LAG as master but QCA8K_PORT_LOOKUP_MEMBER with only
  one CPU?

If I got this right, then I get what you mean with the fact that I
should update the lag_join/leave definition and refresh each
configuration.

> >  	.master_state_change	= qca8k_master_change,
> >  	.connect_tag_protocol	= qca8k_connect_tag_protocol,
> >  };
> > diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> > index c5cc8a172d65..424f851db881 100644
> > --- a/drivers/net/dsa/qca/qca8k.h
> > +++ b/drivers/net/dsa/qca/qca8k.h
> > @@ -250,6 +250,7 @@
> >  #define   QCA8K_GLOBAL_FW_CTRL1_MC_DP_MASK		GENMASK(14, 8)
> >  #define   QCA8K_GLOBAL_FW_CTRL1_UC_DP_MASK		GENMASK(6, 0)
> >  #define QCA8K_PORT_LOOKUP_CTRL(_i)			(0x660 + (_i) * 0xc)
> > +#define   QCA8K_PORT_LOOKUP_USER_MEMBER			GENMASK(5, 1)
> >  #define   QCA8K_PORT_LOOKUP_MEMBER			GENMASK(6, 0)
> >  #define   QCA8K_PORT_LOOKUP_VLAN_MODE_MASK		GENMASK(9, 8)
> >  #define   QCA8K_PORT_LOOKUP_VLAN_MODE(x)		FIELD_PREP(QCA8K_PORT_LOOKUP_VLAN_MODE_MASK, x)
> > -- 
> > 2.40.1
> > 
> 

-- 
	Ansuel