| 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
| ||
|
Message-ID: <20211210201004.ysxelsshgtumk67g@skbuf>
Date: Fri, 10 Dec 2021 20:10:05 +0000
From: Vladimir Oltean <vladimir.oltean@....com>
To: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Ansuel Smith <ansuelsmth@...il.com>
Subject: Re: [RFC PATCH v2 net-next 1/4] net: dsa: provide switch operations
for tracking the master state
On Thu, Dec 09, 2021 at 07:39:24PM +0200, Vladimir Oltean wrote:
> Certain drivers may need to send management traffic to the switch for
> things like register access, FDB dump, etc, to accelerate what their
> slow bus (SPI, I2C, MDIO) can already do.
>
> Ethernet is faster (especially in bulk transactions) but is also more
> unreliable, since the user may decide to bring the DSA master down (or
> not bring it up), therefore severing the link between the host and the
> attached switch.
>
> Drivers needing Ethernet-based register access already should have
> fallback logic to the slow bus if the Ethernet method fails, but that
> fallback may be based on a timeout, and the I/O to the switch may slow
> down to a halt if the master is down, because every Ethernet packet will
> have to time out. The driver also doesn't have the option to turn off
> Ethernet-based I/O momentarily, because it wouldn't know when to turn it
> back on.
>
> Which is where this change comes in. By tracking NETDEV_CHANGE,
> NETDEV_UP and NETDEV_GOING_DOWN events on the DSA master, we should know
> the exact interval of time during which this interface is reliably
> available for traffic. Provide this information to switches so they can
> use it as they wish.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> include/net/dsa.h | 11 +++++++++++
> net/dsa/dsa2.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> net/dsa/dsa_priv.h | 13 +++++++++++++
> net/dsa/slave.c | 27 +++++++++++++++++++++++++++
> net/dsa/switch.c | 15 +++++++++++++++
> 5 files changed, 112 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index bdf308a5c55e..8690b9c6d674 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -296,6 +296,10 @@ struct dsa_port {
> struct list_head fdbs;
> struct list_head mdbs;
>
> + /* Master state bits, valid only on CPU ports */
> + u8 master_admin_up:1,
> + master_oper_up:1;
> +
> bool setup;
> };
>
> @@ -1011,6 +1015,13 @@ struct dsa_switch_ops {
> int (*tag_8021q_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
> u16 flags);
> int (*tag_8021q_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
> +
> + /*
> + * DSA master tracking operations
> + */
> + void (*master_state_change)(struct dsa_switch *ds,
> + const struct net_device *master,
> + bool operational);
> };
>
> #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes) \
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 8814fa0e44c8..a6cb3470face 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -1187,6 +1187,52 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
> return err;
> }
>
> +static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
> + struct net_device *master)
> +{
> + struct dsa_notifier_master_state_info info;
> + struct dsa_port *cpu_dp = master->dsa_ptr;
> +
> + info.master = master;
> + info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up;
> +
> + dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
> +}
> +
> +void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
> + struct net_device *master,
> + bool up)
> +{
> + struct dsa_port *cpu_dp = master->dsa_ptr;
> + bool notify = false;
> +
> + if ((cpu_dp->master_admin_up && cpu_dp->master_oper_up) !=
> + (up && cpu_dp->master_oper_up))
> + notify = true;
> +
> + cpu_dp->master_admin_up = up;
> +
> + if (notify)
> + dsa_tree_master_state_change(dst, master);
> +}
> +
> +void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
> + struct net_device *master,
> + bool up)
> +{
> + struct dsa_port *cpu_dp = master->dsa_ptr;
> + bool notify = false;
> +
> + if ((cpu_dp->master_admin_up && cpu_dp->master_oper_up) !=
> + (cpu_dp->master_admin_up && up))
> + notify = true;
> +
> + cpu_dp->master_oper_up = up;
> +
> + if (notify)
> + dsa_tree_master_state_change(dst, master);
> +}
> +
> static struct dsa_port *dsa_port_touch(struct dsa_switch *ds, int index)
> {
> struct dsa_switch_tree *dst = ds->dst;
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 38ce5129a33d..c47864446adc 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -43,6 +43,7 @@ enum {
> DSA_NOTIFIER_MRP_DEL_RING_ROLE,
> DSA_NOTIFIER_TAG_8021Q_VLAN_ADD,
> DSA_NOTIFIER_TAG_8021Q_VLAN_DEL,
> + DSA_NOTIFIER_MASTER_STATE_CHANGE,
> };
>
> /* DSA_NOTIFIER_AGEING_TIME */
> @@ -126,6 +127,12 @@ struct dsa_notifier_tag_8021q_vlan_info {
> u16 vid;
> };
>
> +/* DSA_NOTIFIER_MASTER_STATE_CHANGE */
> +struct dsa_notifier_master_state_info {
> + const struct net_device *master;
> + bool operational;
> +};
> +
> struct dsa_switchdev_event_work {
> struct dsa_switch *ds;
> int port;
> @@ -506,6 +513,12 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
> struct net_device *master,
> const struct dsa_device_ops *tag_ops,
> const struct dsa_device_ops *old_tag_ops);
> +void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
> + struct net_device *master,
> + bool up);
> +void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
> + struct net_device *master,
> + bool up);
> unsigned int dsa_bridge_num_get(const struct net_device *bridge_dev, int max);
> void dsa_bridge_num_put(const struct net_device *bridge_dev,
> unsigned int bridge_num);
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 2b153b366118..9f3b25c08c13 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2349,6 +2349,31 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
> err = dsa_port_lag_change(dp, info->lower_state_info);
> return notifier_from_errno(err);
> }
> + case NETDEV_CHANGE: {
> + if (netdev_uses_dsa(dev)) {
> + struct dsa_port *cpu_dp = dev->dsa_ptr;
> + struct dsa_switch_tree *dst = cpu_dp->ds->dst;
> +
> + dsa_tree_master_oper_state_change(dst, dev,
> + netif_oper_up(dev));
must also add a call here to change the admin state, due to the fact
that linkwatch_do_dev may call netdev_state_change() after dev_activate().
So it seems that "case NETDEV_CHANGE" and "case NETDEV_UP" may share the
same implementation, like this:
case NETDEV_CHANGE:
case UP:
if (netdev_uses_dsa(dev)) {
struct dsa_port *cpu_dp = dev->dsa_ptr;
struct dsa_switch_tree *dst = cpu_dp->ds->dst;
dsa_tree_master_admin_state_change(dst, dev,
qdisc_tx_is_noop(dev));
dsa_tree_master_oper_state_change(dst, dev,
netif_oper_up(dev));
return NOTIFY_OK;
}
return NOTIFY_DONE;
}
Would be good to also add some comments.
> +
> + return NOTIFY_OK;
> + }
> +
> + return NOTIFY_DONE;
> + }
> + case NETDEV_UP: {
> + if (netdev_uses_dsa(dev)) {
> + struct dsa_port *cpu_dp = dev->dsa_ptr;
> + struct dsa_switch_tree *dst = cpu_dp->ds->dst;
> +
> + dsa_tree_master_admin_state_change(dst, dev, true);
s/true/qdisc_tx_is_noop(dev)/
> +
> + return NOTIFY_OK;
> + }
> +
> + return NOTIFY_DONE;
> + }
> case NETDEV_GOING_DOWN: {
> struct dsa_port *dp, *cpu_dp;
> struct dsa_switch_tree *dst;
> @@ -2360,6 +2385,8 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
> cpu_dp = dev->dsa_ptr;
> dst = cpu_dp->ds->dst;
>
> + dsa_tree_master_admin_state_change(dst, dev, false);
> +
> list_for_each_entry(dp, &dst->ports, list) {
> if (!dsa_port_is_user(dp))
> continue;
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 9c92edd96961..78816a6805c8 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -699,6 +699,18 @@ dsa_switch_mrp_del_ring_role(struct dsa_switch *ds,
> return 0;
> }
>
> +static int
> +dsa_switch_master_state_change(struct dsa_switch *ds,
> + struct dsa_notifier_master_state_info *info)
> +{
> + if (!ds->ops->master_state_change)
> + return 0;
> +
> + ds->ops->master_state_change(ds, info->master, info->operational);
> +
> + return 0;
> +}
> +
> static int dsa_switch_event(struct notifier_block *nb,
> unsigned long event, void *info)
> {
> @@ -784,6 +796,9 @@ static int dsa_switch_event(struct notifier_block *nb,
> case DSA_NOTIFIER_TAG_8021Q_VLAN_DEL:
> err = dsa_switch_tag_8021q_vlan_del(ds, info);
> break;
> + case DSA_NOTIFIER_MASTER_STATE_CHANGE:
> + err = dsa_switch_master_state_change(ds, info);
> + break;
> default:
> err = -EOPNOTSUPP;
> break;
> --
> 2.25.1
>
Powered by blists - more mailing lists