Message ID | 1416610170-21224-5-git-send-email-roopa@cumulusnetworks.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On 11/21/14 at 02:49pm, roopa@cumulusnetworks.com wrote: > + nla_put_u8(skb, IFLA_BRPORT_GUARD, > + br_get_port_flag(p, IFLA_BRPORT_GUARD, BR_BPDU_GUARD)) || A helper taking nla_put_br_flag(port, skb, attrtype, brflag) would simplify this code a lot. > @@ -305,7 +312,9 @@ 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); > + > + if (BR_HW_OFFLOAD(p->br)) > + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state); I assume the yet unfinished netdev_sw_port_set_attr() will call netdev_sw_port_stp_update()? > @@ -316,13 +325,34 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], > { > if (tb[attrtype]) { > u8 flag = nla_get_u8(tb[attrtype]); > - if (flag) > - p->flags |= mask; > - else > + if (flag) { > + flag_upper = flag & 0xf0 > + if (!flag_upper || (flag_upper & BRPORT_KERNEL)) > + p->flags |= mask; > + if ((flag_upper & BRPORT_HW_OFFLOAD) || > + (BR_HW_OFFLOAD(p->br))) > + /* Also set the port flag in hw */ > + netdev_sw_port_set_attr(p->dev, attrtype, 1); I'm not sure I understand the || here. HW_OFFLOAD enabled on the net_device is a conditional for all netdev_sw_port_set_attr() calls and at the same time implies BRPORT_HW_OFFLOAD on all attributes. As I read this code now, I don't see how you would offload individual features to hardware. > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 8f3f081..3ebd196 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -41,6 +41,8 @@ > /* Path to usermode spanning tree program */ > #define BR_STP_PROG "/sbin/bridge-stp" > > +#define BR_HW_OFFLOAD(br) !!(br->dev->features & NETIF_F_HW_OFFLOAD) Let's not add more non type safe macros. A static inline seems like a better fit here. -- 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/21/14, 3:30 PM, Thomas Graf wrote: > On 11/21/14 at 02:49pm, roopa@cumulusnetworks.com wrote: >> + nla_put_u8(skb, IFLA_BRPORT_GUARD, >> + br_get_port_flag(p, IFLA_BRPORT_GUARD, BR_BPDU_GUARD)) || > A helper taking nla_put_br_flag(port, skb, attrtype, brflag) would simplify > this code a lot. sure > >> @@ -305,7 +312,9 @@ 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); >> + >> + if (BR_HW_OFFLOAD(p->br)) >> + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state); > I assume the yet unfinished netdev_sw_port_set_attr() will call > netdev_sw_port_stp_update()? netdev_sw_port_set_attr will just call the switch driver ndo for bridge port attributes. (ndo_sw_bridge_port_set_attr()). On that note it would be nice to rename the whole thing (the other thread on "sw" vs "offload"), this could be renamed to netdev_offload_bridge_port_set_attr() or something along those lines. > >> @@ -316,13 +325,34 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], >> { >> if (tb[attrtype]) { >> u8 flag = nla_get_u8(tb[attrtype]); >> - if (flag) >> - p->flags |= mask; >> - else >> + if (flag) { >> + flag_upper = flag & 0xf0 >> + if (!flag_upper || (flag_upper & BRPORT_KERNEL)) >> + p->flags |= mask; >> + if ((flag_upper & BRPORT_HW_OFFLOAD) || >> + (BR_HW_OFFLOAD(p->br))) >> + /* Also set the port flag in hw */ >> + netdev_sw_port_set_attr(p->dev, attrtype, 1); > I'm not sure I understand the || here. HW_OFFLOAD enabled on the > net_device is a conditional for all netdev_sw_port_set_attr() calls > and at the same time implies BRPORT_HW_OFFLOAD on all attributes. Just realized this needs another condition, I will fix it. The idea is, the port attribute flags BRPORT_KERNEL and BRPORT_HW_OFFLOAD can be used to override the HW_OFFLOAD flag on the bridge netdev. You will only call the swdev api to offload the bridge port flag, if the bridge netdev has the HW_OFFLOAD flag set and also the per port attribute flag does not override it. > As I read this code now, I don't see how you would offload individual > features to hardware. This is for offloading individual bridge port flags like learning and flooding. You want to be able to HW_OFFLOAD the bridge device, but be able to control offloading of some of the port flags like learning and flooding. For example some of the realtek devices might want to turn learning in hw off by default but learn in software. > >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 8f3f081..3ebd196 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -41,6 +41,8 @@ >> /* Path to usermode spanning tree program */ >> #define BR_STP_PROG "/sbin/bridge-stp" >> >> +#define BR_HW_OFFLOAD(br) !!(br->dev->features & NETIF_F_HW_OFFLOAD) > Let's not add more non type safe macros. A static inline seems like > a better fit here. sure, ack. Thanks for the review. -- 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/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 13fecf1..e92e810 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -57,12 +57,19 @@ static int br_port_fill_attrs(struct sk_buff *skb, nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) || nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) || nla_put_u8(skb, IFLA_BRPORT_MODE, mode) || - nla_put_u8(skb, IFLA_BRPORT_GUARD, !!(p->flags & BR_BPDU_GUARD)) || - nla_put_u8(skb, IFLA_BRPORT_PROTECT, !!(p->flags & BR_ROOT_BLOCK)) || - nla_put_u8(skb, IFLA_BRPORT_FAST_LEAVE, !!(p->flags & BR_MULTICAST_FAST_LEAVE)) || - nla_put_u8(skb, IFLA_BRPORT_LEARNING, !!(p->flags & BR_LEARNING)) || - nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD, !!(p->flags & BR_FLOOD)) || - nla_put_u8(skb, IFLA_BRPORT_PROXYARP, !!(p->flags & BR_PROXYARP))) + nla_put_u8(skb, IFLA_BRPORT_GUARD, + br_get_port_flag(p, IFLA_BRPORT_GUARD, BR_BPDU_GUARD)) || + nla_put_u8(skb, IFLA_BRPORT_PROTECT, + br_get_port_flag(p, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK)) || + nla_put_u8(skb, IFLA_BRPORT_FAST_LEAVE, + br_get_port_flag(p, IFLA_BRPORT_FAST_LEAVE, + BR_MULTICAST_FAST_LEAVE)) || + nla_put_u8(skb, IFLA_BRPORT_LEARNING, + br_get_port_flag(p, IFLA_BRPORT_LEARNING, BR_LEARNING)) || + nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD, + br_get_port_flag(p, IFLA_UNICAST_FLOOD, BR_FLOOD)) || + nla_put_u8(skb, IFLA_BRPORT_PROXYARP, + br_get_port_flag(p, IFLA_UNICAST_FLOOD, BR_PROXYARP))) return -EMSGSIZE; return 0; @@ -305,7 +312,9 @@ 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); + + if (BR_HW_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state); br_port_state_selection(p->br); return 0; } @@ -316,13 +325,34 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], { if (tb[attrtype]) { u8 flag = nla_get_u8(tb[attrtype]); - if (flag) - p->flags |= mask; - else + if (flag) { + flag_upper = flag & 0xf0 + if (!flag_upper || (flag_upper & BRPORT_KERNEL)) + p->flags |= mask; + if ((flag_upper & BRPORT_HW_OFFLOAD) || + (BR_HW_OFFLOAD(p->br))) + /* Also set the port flag in hw */ + netdev_sw_port_set_attr(p->dev, attrtype, 1); + } else { p->flags &= ~mask; + if (BR_HW_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, attrtype, 0); + } } } +/* Set/clear or port flags based on attribute */ +static u8 br_get_port_flag(struct net_bridge_port *p, + int attrtype, u8 flag) +{ + attrvalue = !!(p->flags & flag) + if (attrvalue) + attrvalue |= BRPORT_KERNEL + if (netdev_sw_port_get_flag(p->dev, attrtype)) + attrvalue |= BRPORT_HW_OFFLOAD + return attrvalue; +} + /* Process bridge protocol info on port */ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[]) { diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 8f3f081..3ebd196 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -41,6 +41,8 @@ /* Path to usermode spanning tree program */ #define BR_STP_PROG "/sbin/bridge-stp" +#define BR_HW_OFFLOAD(br) !!(br->dev->features & NETIF_F_HW_OFFLOAD) + typedef struct bridge_id bridge_id; typedef struct mac_addr mac_addr; typedef __u16 port_id; diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c index c00139b..bb61dc0 100644 --- a/net/bridge/br_stp.c +++ b/net/bridge/br_stp.c @@ -115,7 +115,8 @@ 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); + if (BR_HW_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state); br_ifinfo_notify(RTM_NEWLINK, p); if (br->forward_delay > 0) @@ -396,7 +397,8 @@ 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); + if (BR__HW_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state); br_ifinfo_notify(RTM_NEWLINK, p); del_timer(&p->forward_delay_timer); @@ -422,7 +424,8 @@ 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); + if (BR_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &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 91279f8..8435b4d 100644 --- a/net/bridge/br_stp_if.c +++ b/net/bridge/br_stp_if.c @@ -90,7 +90,8 @@ 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); + if (BR_HW_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state); br_ifinfo_notify(RTM_NEWLINK, p); } @@ -107,7 +108,8 @@ 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); + if (BR_HW_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_STATE, &p->state); br_ifinfo_notify(RTM_NEWLINK, p); del_timer(&p->message_age_timer); @@ -290,6 +292,8 @@ int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio) br_become_designated_port(p); br_port_state_selection(p->br); } + if (BR_HW_OFFLOAD(p->br)) + netdev_sw_port_set_attr(p->dev, IFLA_BRPORT_PRIORITY, &newprio) return 0; }