| 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: <723bd735-5edc-88ee-4870-e91c98c7dd22@gmail.com>
Date: Sat, 11 Dec 2021 20:22:39 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Ansuel Smith <ansuelsmth@...il.com>, Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Cc: Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [net-next RFC PATCH v4 01/15] net: dsa: provide switch operations
for tracking the master state
On 12/11/2021 11:57 AM, Ansuel Smith wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> 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>
> Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> ---
> include/net/dsa.h | 11 +++++++++++
> net/dsa/dsa2.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> net/dsa/dsa_priv.h | 13 +++++++++++++
> net/dsa/slave.c | 32 ++++++++++++++++++++++++++++++++
> net/dsa/switch.c | 15 +++++++++++++++
> 5 files changed, 117 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 8b496c7e62ef..12352aafe1cf 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -299,6 +299,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;
> };
>
> @@ -1023,6 +1027,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 cf6566168620..86b1e2f11469 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -1245,6 +1245,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;
Since this gets tested a few times below, I would be tempted to add a:
dsa_master_is_operational() helper and use it throughout.
> +
> + 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) !=
Here
> + (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) !=
And here as well
> + (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 0db2b26b0c83..d2f2bce2391b 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -44,6 +44,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 */
> @@ -127,6 +128,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;
> @@ -507,6 +514,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 88f7b8686dac..5ccb0616022d 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2348,6 +2348,36 @@ 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:
> + case NETDEV_UP: {
> + /* Track state of master port.
> + * DSA driver may require the master port (and indirectly
> + * the tagger) to be available for some special operation.
> + */
> + if (netdev_uses_dsa(dev)) {
> + struct dsa_port *cpu_dp = dev->dsa_ptr;
> + struct dsa_switch_tree *dst = cpu_dp->ds->dst;
> +
> + /* Track when the master port is UP */
> + dsa_tree_master_oper_state_change(dst, dev,
> + netif_oper_up(dev));
> +
> + /* Track when the master port is ready and can accept
> + * packet.
> + * NETDEV_UP event is not enough to flag a port as ready.
> + * We also have to wait for linkwatch_do_dev to dev_activate
> + * and emit a NETDEV_CHANGE event.
> + * We check if a master port is ready by checking if the dev
> + * have a qdisc assigned and is not noop.
> + */
> + dsa_tree_master_admin_state_change(dst, dev,
> + qdisc_tx_is_noop(dev));
If my kernel is built with no packet scheduler, is not the interface
going to be configured with noop all of the time?
Other than that LGTM!
--
Florian
Powered by blists - more mailing lists