| Message ID | 20190226184556.16082-3-ivan.khoronzhuk@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: add individual virtual device filtering | expand |
On 2/26/2019 10:45 AM, Ivan Khoronzhuk wrote: > Update vlan mc and uc addresses with VID tag while propagating > addresses to lower devices, do this only if address is not synced. > It allows at end driver level to distinguish addresses belonging > to vlan devices. > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > --- [snip] > > +u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr) Having some kernel doc comment here would also be nice. > +{ > + u16 vid = 0; > + > + if (dev->vid_len != NET_8021Q_VID_TSIZE) > + return vid; > + > + vid = addr[dev->addr_len]; > + vid |= (addr[dev->addr_len + 1] & 0xf) << 8; This uses knowledge of the maximum VLAN ID is 4095, which is fine, might be a good idea to add a check on VID not exceeding the maximum VLAN ID number instead of doing a silent truncation? [snip] > +static void vlan_dev_align_addr_vid(struct net_device *vlan_dev) > +{ > + struct net_device *real_dev = vlan_dev_real_dev(vlan_dev); > + struct netdev_hw_addr *ha; > + > + if (!real_dev->vid_len) > + return; Should not this check be moved to dev_{mc,uc}_sync? It does not seem to me like this would scale really well across different stacked devices (VLAN, bond, macvlan) as well as underlying drivers (cpsw, dsa, etc.). Or maybe the check should be if vlan_dev->vid_len > real_dev->vid_len -> error, right?
On Wed, Feb 27, 2019 at 08:09:44PM -0800, Florian Fainelli wrote: > > >On 2/26/2019 10:45 AM, Ivan Khoronzhuk wrote: >> Update vlan mc and uc addresses with VID tag while propagating >> addresses to lower devices, do this only if address is not synced. >> It allows at end driver level to distinguish addresses belonging >> to vlan devices. >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> --- > >[snip] > >> >> +u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr) > >Having some kernel doc comment here would also be nice. yes can be: vlan_dev_get_addr_vid - get vlan id the address belongs to > >> +{ >> + u16 vid = 0; >> + >> + if (dev->vid_len != NET_8021Q_VID_TSIZE) >> + return vid; >> + >> + vid = addr[dev->addr_len]; >> + vid |= (addr[dev->addr_len + 1] & 0xf) << 8; > >This uses knowledge of the maximum VLAN ID is 4095, which is fine, might >be a good idea to add a check on VID not exceeding the maximum VLAN ID >number instead of doing a silent truncation? and then return -1, not sure, just because it's 0 or directly set by vlan layer and is verified anyway. But no harm to verify even it looks like redundancy. > >[snip] > >> +static void vlan_dev_align_addr_vid(struct net_device *vlan_dev) >> +{ >> + struct net_device *real_dev = vlan_dev_real_dev(vlan_dev); >> + struct netdev_hw_addr *ha; >> + >> + if (!real_dev->vid_len) >> + return; > >Should not this check be moved to dev_{mc,uc}_sync? It does not seem to >me like this would scale really well across different stacked devices >(VLAN, bond, macvlan) as well as underlying drivers (cpsw, dsa, etc.). >Or maybe the check should be if vlan_dev->vid_len > real_dev->vid_len -> >error, right? It shouldn't be part of netdev addr module, no any vlan_dev_vlan_id(vlan_dev) should be there. It's scaled becouse bond/team ...etc, are ethernet devices and have IVDF enabled while configuration. Address propagation always is from leafs to real root device, every underneeth device knows nothing about above, so check is only in one direction.
On 3/1/2019 4:24 AM, Ivan Khoronzhuk wrote: > On Wed, Feb 27, 2019 at 08:09:44PM -0800, Florian Fainelli wrote: >> >> >> On 2/26/2019 10:45 AM, Ivan Khoronzhuk wrote: >>> Update vlan mc and uc addresses with VID tag while propagating >>> addresses to lower devices, do this only if address is not synced. >>> It allows at end driver level to distinguish addresses belonging >>> to vlan devices. >>> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>> --- >> >> [snip] >> >>> >>> +u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr) >> >> Having some kernel doc comment here would also be nice. > > yes can be: vlan_dev_get_addr_vid - get vlan id the address belongs to > > >> >>> +{ >>> + u16 vid = 0; >>> + >>> + if (dev->vid_len != NET_8021Q_VID_TSIZE) >>> + return vid; >>> + >>> + vid = addr[dev->addr_len]; >>> + vid |= (addr[dev->addr_len + 1] & 0xf) << 8; >> >> This uses knowledge of the maximum VLAN ID is 4095, which is fine, might >> be a good idea to add a check on VID not exceeding the maximum VLAN ID >> number instead of doing a silent truncation? > > and then return -1, not sure, just because it's 0 or directly set by vlan > layer and is verified anyway. But no harm to verify even it looks like > redundancy. I would have thought that there would be an existing helper function to put/get a VLAN identifier into/from two bytes but that is fine as-is. > >> >> [snip] >> >>> +static void vlan_dev_align_addr_vid(struct net_device *vlan_dev) >>> +{ >>> + struct net_device *real_dev = vlan_dev_real_dev(vlan_dev); >>> + struct netdev_hw_addr *ha; >>> + >>> + if (!real_dev->vid_len) >>> + return; >> >> Should not this check be moved to dev_{mc,uc}_sync? It does not seem to >> me like this would scale really well across different stacked devices >> (VLAN, bond, macvlan) as well as underlying drivers (cpsw, dsa, etc.). >> Or maybe the check should be if vlan_dev->vid_len > real_dev->vid_len -> >> error, right? > > It shouldn't be part of netdev addr module, no any > vlan_dev_vlan_id(vlan_dev) > should be there. > > It's scaled becouse bond/team ...etc, are ethernet devices and have IVDF > enabled while configuration. Address propagation always is from leafs to > real root device, every underneeth device knows nothing about above, so > check is only in one direction. > OK, indeed stacked devices would lead to that, thanks for explaining!
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h index 4cca4da7a6de..94657f3c483a 100644 --- a/include/linux/if_vlan.h +++ b/include/linux/if_vlan.h @@ -136,6 +136,7 @@ extern struct net_device *__vlan_find_dev_deep_rcu(struct net_device *real_dev, extern int vlan_for_each(struct net_device *dev, int (*action)(struct net_device *dev, int vid, void *arg), void *arg); +extern u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr); extern struct net_device *vlan_dev_real_dev(const struct net_device *dev); extern u16 vlan_dev_vlan_id(const struct net_device *dev); extern __be16 vlan_dev_vlan_proto(const struct net_device *dev); diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h index c46daf09a501..f083c43c508f 100644 --- a/net/8021q/vlan.h +++ b/net/8021q/vlan.h @@ -6,6 +6,8 @@ #include <linux/u64_stats_sync.h> #include <linux/list.h> +#define NET_8021Q_VID_TSIZE 2 + /* if this changes, algorithm will have to be reworked because this * depends on completely exhausting the VLAN identifier space. Thus * it gives constant time look-up, but in many cases it wastes memory. diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c index a313165e7a67..fe2ac64c13f8 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -454,6 +454,19 @@ bool vlan_uses_dev(const struct net_device *dev) } EXPORT_SYMBOL(vlan_uses_dev); +u16 vlan_dev_get_addr_vid(struct net_device *dev, const u8 *addr) +{ + u16 vid = 0; + + if (dev->vid_len != NET_8021Q_VID_TSIZE) + return vid; + + vid = addr[dev->addr_len]; + vid |= (addr[dev->addr_len + 1] & 0xf) << 8; + return vid; +} +EXPORT_SYMBOL(vlan_dev_get_addr_vid); + static struct sk_buff *vlan_gro_receive(struct list_head *head, struct sk_buff *skb) { diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index 15293c2a5dd8..93d20b1f4916 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -249,6 +249,14 @@ void vlan_dev_get_realdev_name(const struct net_device *dev, char *result) strncpy(result, vlan_dev_priv(dev)->real_dev->name, 23); } +static void vlan_dev_set_addr_vid(struct net_device *vlan_dev, u8 *addr) +{ + u16 vid = vlan_dev_vlan_id(vlan_dev); + + addr[vlan_dev->addr_len] = vid & 0xff; + addr[vlan_dev->addr_len + 1] = (vid >> 8) & 0xf; +} + bool vlan_dev_inherit_address(struct net_device *dev, struct net_device *real_dev) { @@ -480,8 +488,26 @@ static void vlan_dev_change_rx_flags(struct net_device *dev, int change) } } +static void vlan_dev_align_addr_vid(struct net_device *vlan_dev) +{ + struct net_device *real_dev = vlan_dev_real_dev(vlan_dev); + struct netdev_hw_addr *ha; + + if (!real_dev->vid_len) + return; + + netdev_for_each_mc_addr(ha, vlan_dev) + if (!ha->sync_cnt) + vlan_dev_set_addr_vid(vlan_dev, ha->addr); + + netdev_for_each_uc_addr(ha, vlan_dev) + if (!ha->sync_cnt) + vlan_dev_set_addr_vid(vlan_dev, ha->addr); +} + static void vlan_dev_set_rx_mode(struct net_device *vlan_dev) { + vlan_dev_align_addr_vid(vlan_dev); dev_mc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev); dev_uc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev); }
Update vlan mc and uc addresses with VID tag while propagating addresses to lower devices, do this only if address is not synced. It allows at end driver level to distinguish addresses belonging to vlan devices. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- include/linux/if_vlan.h | 1 + net/8021q/vlan.h | 2 ++ net/8021q/vlan_core.c | 13 +++++++++++++ net/8021q/vlan_dev.c | 26 ++++++++++++++++++++++++++ 4 files changed, 42 insertions(+)