lkml.org 
[lkml]   [2021]   [Dec]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [net-next RFC PATCH v4 01/15] net: dsa: provide switch operations for tracking the master state
    From


    On 12/11/2021 11:57 AM, Ansuel Smith wrote:
    > From: Vladimir Oltean <vladimir.oltean@nxp.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@nxp.com>
    > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.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

    \
     
     \ /
      Last update: 2021-12-12 05:23    [W:3.507 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site