| Message ID | 1415530280-9190-8-git-send-email-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 11/09/14 05:51, Jiri Pirko wrote: > From: Scott Feldman <sfeldma@gmail.com> > > To notify switch driver of change in STP state of bridge port, add new > .ndo op and provide swdev wrapper func to call ndo op. Use it in bridge > code then. > > Signed-off-by: Scott Feldman <sfeldma@gmail.com> > Signed-off-by: Jiri Pirko <jiri@resnulli.us> > --- > include/linux/netdevice.h | 6 ++++++ > include/net/switchdev.h | 6 ++++++ > net/bridge/br_netlink.c | 2 ++ > net/bridge/br_stp.c | 4 ++++ > net/bridge/br_stp_if.c | 3 +++ > net/bridge/br_stp_timer.c | 2 ++ > net/switchdev/switchdev.c | 19 +++++++++++++++++++ > 7 files changed, 42 insertions(+) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 116a19d..35f21a95 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1033,6 +1033,10 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev, > * const unsigned char *addr, > * u16 vid); > * Called to delete a fdb from switch device port. > + * > + * int (*ndo_sw_port_stp_update)(struct net_device *dev, u8 state); > + * Called to notify switch device port of bridge port STP > + * state change. You are unconditionally calling netdev_sw_port_stp_update(p->dev, p->state); Again issue is policy. Could you make this work the same way the fdb_add e.g user intent of whether i want to turn a port in hardware and/or software to disabled/learning/etc is reflected? btw: does _sw_ stand for switch? why not _hw_ ? Could we have one ndo for all flags instead of individual ones. I know the current user space code uses u8 as a bitflag; but maybe we can introduce a new u32 flag bitmask that has all the flags set for backward compat? I can count about a total of 10. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/14 at 08:11am, Jamal Hadi Salim wrote: > You are unconditionally calling > netdev_sw_port_stp_update(p->dev, p->state); > Again issue is policy. Could you make this work the same > way the fdb_add e.g user intent of whether i want to turn > a port in hardware and/or software to disabled/learning/etc > is reflected? Agreed. Can be added in a next series perhaps? > btw: does _sw_ stand for switch? why not _hw_ ? > Could we have one ndo for all flags instead of individual ones. > > I know the current user space code uses u8 as a bitflag; but > maybe we can introduce a new u32 flag bitmask that has all the > flags set for backward compat? I can count about a total of 10. I think we can just extend the size of IFLA_BRPORT_STATE, accept both a u8 and u32, and return a u32 that that is compatible to existing u8 readers. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/14, 5:11 AM, Jamal Hadi Salim wrote: > On 11/09/14 05:51, Jiri Pirko wrote: >> From: Scott Feldman <sfeldma@gmail.com> >> >> To notify switch driver of change in STP state of bridge port, add new >> .ndo op and provide swdev wrapper func to call ndo op. Use it in bridge >> code then. >> >> Signed-off-by: Scott Feldman <sfeldma@gmail.com> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us> >> --- >> include/linux/netdevice.h | 6 ++++++ >> include/net/switchdev.h | 6 ++++++ >> net/bridge/br_netlink.c | 2 ++ >> net/bridge/br_stp.c | 4 ++++ >> net/bridge/br_stp_if.c | 3 +++ >> net/bridge/br_stp_timer.c | 2 ++ >> net/switchdev/switchdev.c | 19 +++++++++++++++++++ >> 7 files changed, 42 insertions(+) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 116a19d..35f21a95 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1033,6 +1033,10 @@ typedef u16 (*select_queue_fallback_t)(struct >> net_device *dev, >> * const unsigned char *addr, >> * u16 vid); >> * Called to delete a fdb from switch device port. >> + * >> + * int (*ndo_sw_port_stp_update)(struct net_device *dev, u8 state); >> + * Called to notify switch device port of bridge port STP >> + * state change. > > You are unconditionally calling > netdev_sw_port_stp_update(p->dev, p->state); > Again issue is policy. Could you make this work the same > way the fdb_add e.g user intent of whether i want to turn > a port in hardware and/or software to disabled/learning/etc > is reflected? > > btw: does _sw_ stand for switch? why not _hw_ ? > Could we have one ndo for all flags instead of individual ones. I agree. There is the bridge port state and a bunch of bridge port flags. A generic ndo will be good. > > I know the current user space code uses u8 as a bitflag; but > maybe we can introduce a new u32 flag bitmask that has all the > flags set for backward compat? I can count about a total of 10. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/14 09:04, Thomas Graf wrote: > On 11/10/14 at 08:11am, Jamal Hadi Salim wrote: >> You are unconditionally calling >> netdev_sw_port_stp_update(p->dev, p->state); >> Again issue is policy. Could you make this work the same >> way the fdb_add e.g user intent of whether i want to turn >> a port in hardware and/or software to disabled/learning/etc >> is reflected? > > Agreed. Can be added in a next series perhaps? > Doesnt seem be hard to fix now. As Andy was pointing out, we have the opportunity to get the basics right in the beggining. > I think we can just extend the size of IFLA_BRPORT_STATE, accept > both a u8 and u32, and return a u32 that that is compatible to > existing u8 readers. > Iam thinking we have an opportunity for a totally different new attribute instead of growing IFLA_BRPORT_STATE to add another bit. Almost every single u8 that is being carried today in its own attribute in the bridge code is in fact a boolean (0/1). We could leave just intro IFLA_BRIDGE_FLAGS and use it for both FDB and BRPORT. cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 116a19d..35f21a95 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1033,6 +1033,10 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev, * const unsigned char *addr, * u16 vid); * Called to delete a fdb from switch device port. + * + * int (*ndo_sw_port_stp_update)(struct net_device *dev, u8 state); + * Called to notify switch device port of bridge port STP + * state change. */ struct net_device_ops { int (*ndo_init)(struct net_device *dev); @@ -1193,6 +1197,8 @@ struct net_device_ops { int (*ndo_sw_port_fdb_del)(struct net_device *dev, const unsigned char *addr, u16 vid); + int (*ndo_sw_port_stp_update)(struct net_device *dev, + u8 state); #endif }; diff --git a/include/net/switchdev.h b/include/net/switchdev.h index 130cef7..bbf7369 100644 --- a/include/net/switchdev.h +++ b/include/net/switchdev.h @@ -21,6 +21,7 @@ int netdev_sw_port_fdb_add(struct net_device *dev, const unsigned char *addr, u16 vid); int netdev_sw_port_fdb_del(struct net_device *dev, const unsigned char *addr, u16 vid); +int netdev_sw_port_stp_update(struct net_device *dev, u8 state); #else @@ -42,6 +43,11 @@ static inline int netdev_sw_port_fdb_del(struct net_device *dev, return -EOPNOTSUPP; } +static inline int netdev_sw_port_stp_update(struct net_device *dev, u8 state) +{ + return -EOPNOTSUPP; +} + #endif #endif /* _LINUX_SWITCHDEV_H_ */ diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 86c239b..13fecf1 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -17,6 +17,7 @@ #include <net/net_namespace.h> #include <net/sock.h> #include <uapi/linux/if_bridge.h> +#include <net/switchdev.h> #include "br_private.h" #include "br_private_stp.h" @@ -304,6 +305,7 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state) br_set_state(p, state); br_log_state(p); + netdev_sw_port_stp_update(p->dev, p->state); br_port_state_selection(p->br); return 0; } diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c index 2b047bc..c00139b 100644 --- a/net/bridge/br_stp.c +++ b/net/bridge/br_stp.c @@ -12,6 +12,7 @@ */ #include <linux/kernel.h> #include <linux/rculist.h> +#include <net/switchdev.h> #include "br_private.h" #include "br_private_stp.h" @@ -114,6 +115,7 @@ static void br_root_port_block(const struct net_bridge *br, br_set_state(p, BR_STATE_LISTENING); br_log_state(p); + netdev_sw_port_stp_update(p->dev, p->state); br_ifinfo_notify(RTM_NEWLINK, p); if (br->forward_delay > 0) @@ -394,6 +396,7 @@ static void br_make_blocking(struct net_bridge_port *p) br_set_state(p, BR_STATE_BLOCKING); br_log_state(p); + netdev_sw_port_stp_update(p->dev, p->state); br_ifinfo_notify(RTM_NEWLINK, p); del_timer(&p->forward_delay_timer); @@ -419,6 +422,7 @@ static void br_make_forwarding(struct net_bridge_port *p) br_multicast_enable_port(p); br_log_state(p); + netdev_sw_port_stp_update(p->dev, p->state); br_ifinfo_notify(RTM_NEWLINK, p); if (br->forward_delay != 0) diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c index 4114687..91279f8 100644 --- a/net/bridge/br_stp_if.c +++ b/net/bridge/br_stp_if.c @@ -15,6 +15,7 @@ #include <linux/kmod.h> #include <linux/etherdevice.h> #include <linux/rtnetlink.h> +#include <net/switchdev.h> #include "br_private.h" #include "br_private_stp.h" @@ -89,6 +90,7 @@ void br_stp_enable_port(struct net_bridge_port *p) br_init_port(p); br_port_state_selection(p->br); br_log_state(p); + netdev_sw_port_stp_update(p->dev, p->state); br_ifinfo_notify(RTM_NEWLINK, p); } @@ -105,6 +107,7 @@ void br_stp_disable_port(struct net_bridge_port *p) p->config_pending = 0; br_log_state(p); + netdev_sw_port_stp_update(p->dev, p->state); br_ifinfo_notify(RTM_NEWLINK, p); del_timer(&p->message_age_timer); diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c index 4fcaa67..5bb8997 100644 --- a/net/bridge/br_stp_timer.c +++ b/net/bridge/br_stp_timer.c @@ -13,6 +13,7 @@ #include <linux/kernel.h> #include <linux/times.h> +#include <net/switchdev.h> #include "br_private.h" #include "br_private_stp.h" @@ -97,6 +98,7 @@ static void br_forward_delay_timer_expired(unsigned long arg) netif_carrier_on(br->dev); } br_log_state(p); + netdev_sw_port_stp_update(p->dev, p->state); br_ifinfo_notify(RTM_NEWLINK, p); spin_unlock(&br->lock); } diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index 93d47b7..75997a5 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -72,3 +72,22 @@ int netdev_sw_port_fdb_del(struct net_device *dev, return ops->ndo_sw_port_fdb_del(dev, addr, vid); } EXPORT_SYMBOL(netdev_sw_port_fdb_del); + +/** + * netdev_sw_port_stp_update - Notify switch device port of STP + * state change + * @dev: port device + * @state: port STP state + * + * Notify switch device port of bridge port STP state change. + */ +int netdev_sw_port_stp_update(struct net_device *dev, u8 state) +{ + const struct net_device_ops *ops = dev->netdev_ops; + + if (!ops->ndo_sw_port_stp_update) + return -EOPNOTSUPP; + WARN_ON(!ops->ndo_sw_parent_id_get); + return ops->ndo_sw_port_stp_update(dev, state); +} +EXPORT_SYMBOL(netdev_sw_port_stp_update);