| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 20 Jun 2023 23:12:27 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH] net: dsa: qca8k: add support for
port_change_master
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@...il.com>
> ---
> 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.
> + 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).
> + 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.
> + /* 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?
> +
> + /* Reenable port */
> + qca8k_port_set_status(priv, port, 1);
or disable/enable it, for that matter?
> +
> + 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.
> .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
>
Powered by blists - more mailing lists