| Message ID | 20181203184023.3430-3-ivan.khoronzhuk@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: allow hw addresses for virtual devices | expand |
On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: > Update vlan mc and uc addresses with VID tag while propagating address > set to lower devices, do this only if address is not synched. It allows > on end driver level to distinguish address belonging to vlans. Underlying driver for the real device would be able to properly identify that you are attempting to add an address to a virtual device, which happens to be of VLAN kind so I am really not sure this is the right approach here. From there, it seems to me that we have two situations: - each of your network devices expose VLAN devices directly on top of the real device, in which case your driver should support ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN devices are create and maintain a VLAN device to VID correspondence if it needs to when being called while setting the addresses - you are setting up a bridge that is VLAN aware on one of your bridge ports, and there you can use switchdev to learn about such events and know about both addresses as well as VIDs that must be programmed into your real device It seems to me that what you need may be something like either: - notifications on slave devices when addresses are added via ndo_set_rxmode() or - dev_{uc,mc}_sync() should be augmented with a "source net_device" argument which allows you to differentiate which network device is the source of the address programming. That way, no need to "hash" the MAC address with a VID, any network device specific information can be provided and in the real device driver you can do: if (netif_is_vlan()... etc.) Hopefully someone else will chime in. > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > --- > include/linux/if_vlan.h | 1 + > net/8021q/vlan_core.c | 10 ++++++++++ > net/8021q/vlan_dev.c | 26 ++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+) > > 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_core.c b/net/8021q/vlan_core.c > index a313165e7a67..5d17947d6988 100644 > --- a/net/8021q/vlan_core.c > +++ b/net/8021q/vlan_core.c > @@ -454,6 +454,16 @@ 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; > + > + 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 b2d9c8f27cd7..c05b313314b7 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -250,6 +250,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) > { > @@ -481,8 +489,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); > } >
On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote: >On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: >> Update vlan mc and uc addresses with VID tag while propagating address >> set to lower devices, do this only if address is not synched. It allows >> on end driver level to distinguish address belonging to vlans. > >Underlying driver for the real device would be able to properly identify >that you are attempting to add an address to a virtual device, which >happens to be of VLAN kind so I am really not sure this is the right >approach here. > >From there, it seems to me that we have two situations: > >- each of your network devices expose VLAN devices directly on top of >the real device, in which case your driver should support >ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN devices >are create and maintain a VLAN device to VID correspondence if it needs >to when being called while setting the addresses > >- you are setting up a bridge that is VLAN aware on one of your bridge >ports, and there you can use switchdev to learn about such events and >know about both addresses as well as VIDs that must be programmed into >your real device No limits to have any "middle" device between real end device and virtual one, not only a bridge, but also other kind. And as it's generic change, it should cover all such cases, the simplest example is: real_dev/macvlan/vlan. > >It seems to me that what you need may be something like either: > >- notifications on slave devices when addresses are added via >ndo_set_rxmode() > >or > >- dev_{uc,mc}_sync() should be augmented with a "source net_device" >argument which allows you to differentiate which network device is the >source of the address programming. That way, no need to "hash" the MAC >address with a VID, any network device specific information can be >provided and in the real device driver you can do: if >(netif_is_vlan()... etc.) No issue to retrieve vlan dev if it's directly on top of real dev. Issue is to get it when it's not directly connected as it's not in vlan_info group list. Who knows what else can be "structed" on top of real dev till the vlan device. Please look on reply for cover letter, as it seems requires similar response. > >Hopefully someone else will chime in. > >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> --- >> include/linux/if_vlan.h | 1 + >> net/8021q/vlan_core.c | 10 ++++++++++ >> net/8021q/vlan_dev.c | 26 ++++++++++++++++++++++++++ >> 3 files changed, 37 insertions(+) >> >> 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_core.c b/net/8021q/vlan_core.c >> index a313165e7a67..5d17947d6988 100644 >> --- a/net/8021q/vlan_core.c >> +++ b/net/8021q/vlan_core.c >> @@ -454,6 +454,16 @@ 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; >> + >> + 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 b2d9c8f27cd7..c05b313314b7 100644 >> --- a/net/8021q/vlan_dev.c >> +++ b/net/8021q/vlan_dev.c >> @@ -250,6 +250,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) >> { >> @@ -481,8 +489,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); >> } >> > > >-- >Florian
On 12/3/18 3:51 PM, Ivan Khoronzhuk wrote: > On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote: >> On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: >>> Update vlan mc and uc addresses with VID tag while propagating address >>> set to lower devices, do this only if address is not synched. It allows >>> on end driver level to distinguish address belonging to vlans. >> >> Underlying driver for the real device would be able to properly identify >> that you are attempting to add an address to a virtual device, which >> happens to be of VLAN kind so I am really not sure this is the right >> approach here. >> >> From there, it seems to me that we have two situations: >> >> - each of your network devices expose VLAN devices directly on top of >> the real device, in which case your driver should support >> ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN devices >> are create and maintain a VLAN device to VID correspondence if it needs >> to when being called while setting the addresses >> >> - you are setting up a bridge that is VLAN aware on one of your bridge >> ports, and there you can use switchdev to learn about such events and >> know about both addresses as well as VIDs that must be programmed into >> your real device > No limits to have any "middle" device between real end device and > virtual one, not only a bridge, but also other kind. And as it's generic > change, it should cover all such cases, the simplest example is: > real_dev/macvlan/vlan. It is not generic if the additional information is a VLAN ID, that construct does not apply to all types of virtual devices, that is part of my issue with the extra VID that is being added. If this was a void * priv and any virtual device could pass up/down information that might be more acceptable. > >> >> It seems to me that what you need may be something like either: >> >> - notifications on slave devices when addresses are added via >> ndo_set_rxmode() >> >> or >> >> - dev_{uc,mc}_sync() should be augmented with a "source net_device" >> argument which allows you to differentiate which network device is the >> source of the address programming. That way, no need to "hash" the MAC >> address with a VID, any network device specific information can be >> provided and in the real device driver you can do: if >> (netif_is_vlan()... etc.) > No issue to retrieve vlan dev if it's directly on top of real dev. > Issue is to get it when it's not directly connected as it's not in > vlan_info group list. Who knows what else can be "structed" on top of > real dev till the vlan device. Please look on reply for cover letter, > as it seems requires similar response. In that case, there are notifications generated that you must be listening to determine whether you have something like a VLAN device on top of a bond, which is a port member of a bridge, on which one of your real device port is enslaved (yes, it can be that type of stacking). > >> >> Hopefully someone else will chime in. >> >>> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>> --- >>> include/linux/if_vlan.h | 1 + >>> net/8021q/vlan_core.c | 10 ++++++++++ >>> net/8021q/vlan_dev.c | 26 ++++++++++++++++++++++++++ >>> 3 files changed, 37 insertions(+) >>> >>> 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_core.c b/net/8021q/vlan_core.c >>> index a313165e7a67..5d17947d6988 100644 >>> --- a/net/8021q/vlan_core.c >>> +++ b/net/8021q/vlan_core.c >>> @@ -454,6 +454,16 @@ 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; >>> + >>> + 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 b2d9c8f27cd7..c05b313314b7 100644 >>> --- a/net/8021q/vlan_dev.c >>> +++ b/net/8021q/vlan_dev.c >>> @@ -250,6 +250,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) >>> { >>> @@ -481,8 +489,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); >>> } >>> >> >> >> -- >> Florian >
On Mon, Dec 03, 2018 at 03:57:03PM -0800, Florian Fainelli wrote: >On 12/3/18 3:51 PM, Ivan Khoronzhuk wrote: >> On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote: >>> On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: >>>> Update vlan mc and uc addresses with VID tag while propagating address >>>> set to lower devices, do this only if address is not synched. It allows >>>> on end driver level to distinguish address belonging to vlans. >>> >>> Underlying driver for the real device would be able to properly identify >>> that you are attempting to add an address to a virtual device, which >>> happens to be of VLAN kind so I am really not sure this is the right >>> approach here. >>> >>> From there, it seems to me that we have two situations: >>> >>> - each of your network devices expose VLAN devices directly on top of >>> the real device, in which case your driver should support >>> ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN devices >>> are create and maintain a VLAN device to VID correspondence if it needs >>> to when being called while setting the addresses >>> >>> - you are setting up a bridge that is VLAN aware on one of your bridge >>> ports, and there you can use switchdev to learn about such events and >>> know about both addresses as well as VIDs that must be programmed into >>> your real device >> No limits to have any "middle" device between real end device and >> virtual one, not only a bridge, but also other kind. And as it's generic >> change, it should cover all such cases, the simplest example is: >> real_dev/macvlan/vlan. > >It is not generic if the additional information is a VLAN ID, that >construct does not apply to all types of virtual devices, that is part >of my issue with the extra VID that is being added. If this was a void * >priv and any virtual device could pass up/down information that might be >more acceptable. I tractate it as virtual identification not relating it to vlans only. VID just uses for eth devices part of already reserved address. I was also thinking about passing smth like pointer on vlan device, frankly even don't remember what stopped me, probably size of pointer or so, maybe it looked harder... but it still seems possible and at least for ethernet address is enough space: 32 - 6 = 26, 26 > 8. Despite it only combines methods with extended address space and passing link to vlan device, it adds ability to apply allmulti and promisc modes per vlans. > >> >>> >>> It seems to me that what you need may be something like either: >>> >>> - notifications on slave devices when addresses are added via >>> ndo_set_rxmode() >>> >>> or >>> >>> - dev_{uc,mc}_sync() should be augmented with a "source net_device" >>> argument which allows you to differentiate which network device is the >>> source of the address programming. That way, no need to "hash" the MAC >>> address with a VID, any network device specific information can be >>> provided and in the real device driver you can do: if >>> (netif_is_vlan()... etc.) >> No issue to retrieve vlan dev if it's directly on top of real dev. >> Issue is to get it when it's not directly connected as it's not in >> vlan_info group list. Who knows what else can be "structed" on top of >> real dev till the vlan device. Please look on reply for cover letter, >> as it seems requires similar response. > >In that case, there are notifications generated that you must be >listening to determine whether you have something like a VLAN device on >top of a bond, which is a port member of a bridge, on which one of your >real device port is enslaved (yes, it can be that type of stacking). It becomes even harder, traversing all structure of devices, seems not very simple task. > >> >>> >>> Hopefully someone else will chime in. >>> >>>> >>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>>> --- >>>> include/linux/if_vlan.h | 1 + >>>> net/8021q/vlan_core.c | 10 ++++++++++ >>>> net/8021q/vlan_dev.c | 26 ++++++++++++++++++++++++++ >>>> 3 files changed, 37 insertions(+) >>>> >>>> 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_core.c b/net/8021q/vlan_core.c >>>> index a313165e7a67..5d17947d6988 100644 >>>> --- a/net/8021q/vlan_core.c >>>> +++ b/net/8021q/vlan_core.c >>>> @@ -454,6 +454,16 @@ 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; >>>> + >>>> + 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 b2d9c8f27cd7..c05b313314b7 100644 >>>> --- a/net/8021q/vlan_dev.c >>>> +++ b/net/8021q/vlan_dev.c >>>> @@ -250,6 +250,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) >>>> { >>>> @@ -481,8 +489,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); >>>> } >>>> >>> >>> >>> -- >>> Florian >> > > >-- >Florian
On Mon, Dec 03, 2018 at 03:57:03PM -0800, Florian Fainelli wrote: >On 12/3/18 3:51 PM, Ivan Khoronzhuk wrote: >> On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote: >>> On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: >>>> Update vlan mc and uc addresses with VID tag while propagating address >>>> set to lower devices, do this only if address is not synched. It allows >>>> on end driver level to distinguish address belonging to vlans. >>> >>> Underlying driver for the real device would be able to properly identify >>> that you are attempting to add an address to a virtual device, which >>> happens to be of VLAN kind so I am really not sure this is the right >>> approach here. >>> >>> From there, it seems to me that we have two situations: >>> >>> - each of your network devices expose VLAN devices directly on top of >>> the real device, in which case your driver should support >>> ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN devices >>> are create and maintain a VLAN device to VID correspondence if it needs >>> to when being called while setting the addresses >>> >>> - you are setting up a bridge that is VLAN aware on one of your bridge >>> ports, and there you can use switchdev to learn about such events and >>> know about both addresses as well as VIDs that must be programmed into >>> your real device >> No limits to have any "middle" device between real end device and >> virtual one, not only a bridge, but also other kind. And as it's generic >> change, it should cover all such cases, the simplest example is: >> real_dev/macvlan/vlan. > >It is not generic if the additional information is a VLAN ID, that >construct does not apply to all types of virtual devices, that is part >of my issue with the extra VID that is being added. If this was a void * >priv and any virtual device could pass up/down information that might be >more acceptable. You mean to create smth like common struct pinned to "an address" and pass information not only like vid, but in parallel what ever user wanted. Even if pass vlan device pointer it still considered like an address continuation and same sync method is used w/o modification. And here vid is considered as part of address, by a big account address+vid it's a separate address, same happens with the pointer, address+pointer it's still separate address. I was thinking also about pinned list of vlans to the address, but in this case this information also has to be synced by members of device chain, because it can be modified on any device level and it looks not very friendly, and at the end address space has addresses with pinned lists of vlans with their pointers. But keeping this stuff in sync is not simplest decision.
On 12/4/18 10:57 AM, Ivan Khoronzhuk wrote: > On Mon, Dec 03, 2018 at 03:57:03PM -0800, Florian Fainelli wrote: >> On 12/3/18 3:51 PM, Ivan Khoronzhuk wrote: >>> On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote: >>>> On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: >>>>> Update vlan mc and uc addresses with VID tag while propagating address >>>>> set to lower devices, do this only if address is not synched. It >>>>> allows >>>>> on end driver level to distinguish address belonging to vlans. >>>> >>>> Underlying driver for the real device would be able to properly >>>> identify >>>> that you are attempting to add an address to a virtual device, which >>>> happens to be of VLAN kind so I am really not sure this is the right >>>> approach here. >>>> >>>> From there, it seems to me that we have two situations: >>>> >>>> - each of your network devices expose VLAN devices directly on top of >>>> the real device, in which case your driver should support >>>> ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN devices >>>> are create and maintain a VLAN device to VID correspondence if it needs >>>> to when being called while setting the addresses >>>> >>>> - you are setting up a bridge that is VLAN aware on one of your bridge >>>> ports, and there you can use switchdev to learn about such events and >>>> know about both addresses as well as VIDs that must be programmed into >>>> your real device >>> No limits to have any "middle" device between real end device and >>> virtual one, not only a bridge, but also other kind. And as it's generic >>> change, it should cover all such cases, the simplest example is: >>> real_dev/macvlan/vlan. >> >> It is not generic if the additional information is a VLAN ID, that >> construct does not apply to all types of virtual devices, that is part >> of my issue with the extra VID that is being added. If this was a void * >> priv and any virtual device could pass up/down information that might be >> more acceptable. > > You mean to create smth like common struct pinned to "an address" and > pass information not only like vid, but in parallel what ever user wanted. > Even if pass vlan device pointer it still considered like an address > continuation and same sync method is used w/o modification. And here vid > is considered as part of address, by a big account address+vid it's a > separate address, same happens with the pointer, address+pointer it's > still separate address. That depends on the HW implementation, some switches do individual VLAN learning (IVL) and some do shared VLAN learning (SVL) so whether the VID becomes part of the address resolution logic is HW dependent, obviously the more capable, the better (IVL). > > I was thinking also about pinned list of vlans to the address, but in > this case this information also has to be synced by members of device > chain, > because it can be modified on any device level and it looks not very > friendly, > and at the end address space has addresses with pinned lists of vlans with > their pointers. But keeping this stuff in sync is not simplest decision. > > I really think we are not communicating properly, it really seems to me that if you had the information about the upper device trying to add an address to the lower device filter's either through notification or call to ndo_set_rxmode, you could be solving your problems. What are we missing here? -- Florian
On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: >On 12/4/18 10:57 AM, Ivan Khoronzhuk wrote: >> On Mon, Dec 03, 2018 at 03:57:03PM -0800, Florian Fainelli wrote: >>> On 12/3/18 3:51 PM, Ivan Khoronzhuk wrote: >>>> On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote: >>>>> On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: >>>>>> Update vlan mc and uc addresses with VID tag while propagating address >>>>>> set to lower devices, do this only if address is not synched. It >>>>>> allows >>>>>> on end driver level to distinguish address belonging to vlans. >>>>> >>>>> Underlying driver for the real device would be able to properly >>>>> identify >>>>> that you are attempting to add an address to a virtual device, which >>>>> happens to be of VLAN kind so I am really not sure this is the right >>>>> approach here. >>>>> >>>>> From there, it seems to me that we have two situations: >>>>> >>>>> - each of your network devices expose VLAN devices directly on top of >>>>> the real device, in which case your driver should support >>>>> ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN devices >>>>> are create and maintain a VLAN device to VID correspondence if it needs >>>>> to when being called while setting the addresses >>>>> >>>>> - you are setting up a bridge that is VLAN aware on one of your bridge >>>>> ports, and there you can use switchdev to learn about such events and >>>>> know about both addresses as well as VIDs that must be programmed into >>>>> your real device >>>> No limits to have any "middle" device between real end device and >>>> virtual one, not only a bridge, but also other kind. And as it's generic >>>> change, it should cover all such cases, the simplest example is: >>>> real_dev/macvlan/vlan. >>> >>> It is not generic if the additional information is a VLAN ID, that >>> construct does not apply to all types of virtual devices, that is part >>> of my issue with the extra VID that is being added. If this was a void * >>> priv and any virtual device could pass up/down information that might be >>> more acceptable. >> >> You mean to create smth like common struct pinned to "an address" and >> pass information not only like vid, but in parallel what ever user wanted. >> Even if pass vlan device pointer it still considered like an address >> continuation and same sync method is used w/o modification. And here vid >> is considered as part of address, by a big account address+vid it's a >> separate address, same happens with the pointer, address+pointer it's >> still separate address. > >That depends on the HW implementation, some switches do individual VLAN >learning (IVL) and some do shared VLAN learning (SVL) so whether the VID >becomes part of the address resolution logic is HW dependent, obviously >the more capable, the better (IVL). In my case IVL is only choice, as SVL is rather imitation, as each vlan has it's own address table anyway. And I mean not only vlan configuration above the bridge but also any simple configuration above real device. There is proposition to add smth like additional list of entries pinned to the each address as you proposed, but in a little bit different way. Pin the context pointers to each address if IVL config is enabled. Smth like +struct ctx_entry { + void *info; + unsigned type; + int synced; + int sync_cnt; + int refcount; +} Then in hw_addr struct add a ctx_list: struct netdev_hw_addr { struct list_head list; + struct list_head ctx_list; unsigned char addr[MAX_ADDR_LEN]; unsigned char type; ..... } Each ctx_entry contains pointer to some structure, in my case it could be pointer on vlan net_dev, and it can be marked with type VLAN_DEVICE_POINTER or else. In some other invisible cases it could be another one. Main difference between each of them is its pointer and type. And once each net dev calls mc/uc_sync these entires, if not synced, are synced along with addresses. But main difference that these ctx entires are pinned to the address, when addresses are pinned to the device. It can allow to bring information any new abstraction can apply. For real device the list can be empty or contain special entry to differ it from the vlan device entries, as could happen only some vlan is address owner. Not sure it can be much simpler but it definitely can introduce more capabilities, and potentially cover some other cases including your. Probably I need rename the series on smth like: "make addressing scheme to be IVL capable" and send RFCv2 Thanks for your comment, it's really valuable. > >> >> I was thinking also about pinned list of vlans to the address, but in >> this case this information also has to be synced by members of device >> chain, >> because it can be modified on any device level and it looks not very >> friendly, >> and at the end address space has addresses with pinned lists of vlans with >> their pointers. But keeping this stuff in sync is not simplest decision. >> >> > >I really think we are not communicating properly, it really seems to me >that if you had the information about the upper device trying to add an >address to the lower device filter's either through notification or call >to ndo_set_rxmode, you could be solving your problems. What are we >missing here? >-- >Florian
On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: ... >> >> I was thinking also about pinned list of vlans to the address, but in >> this case this information also has to be synced by members of device >> chain, >> because it can be modified on any device level and it looks not very >> friendly, >> and at the end address space has addresses with pinned lists of vlans with >> their pointers. But keeping this stuff in sync is not simplest decision. >> >> > >I really think we are not communicating properly, it really seems to me >that if you had the information about the upper device trying to add an >address to the lower device filter's either through notification or call >to ndo_set_rxmode, you could be solving your problems. What are we >missing here? Sry, missed this one. The problem in getting the owner of address. Just simple case: vlan/macvlan/real_dev or vlan/.../.../real_dev The real dev hasn't simple way to get vid the address belong to, or it has? >-- >Florian
Le 12/4/18 à 4:04 PM, Ivan Khoronzhuk a écrit : > On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: > > ... > >>> >>> I was thinking also about pinned list of vlans to the address, but in >>> this case this information also has to be synced by members of device >>> chain, >>> because it can be modified on any device level and it looks not very >>> friendly, >>> and at the end address space has addresses with pinned lists of vlans >>> with >>> their pointers. But keeping this stuff in sync is not simplest decision. >>> >>> >> >> I really think we are not communicating properly, it really seems to me >> that if you had the information about the upper device trying to add an >> address to the lower device filter's either through notification or call >> to ndo_set_rxmode, you could be solving your problems. What are we >> missing here? > > Sry, missed this one. The problem in getting the owner of address. > Just simple case: vlan/macvlan/real_dev or vlan/.../.../real_dev > > The real dev hasn't simple way to get vid the address belong to, or it has? Humm looks like your right, by the time the address lists are synchronized (e.g: from = vlan_dev, to = real_dev), we lost that information. It looks like I just managed to find such an use case myself with VLAN filtering enabled on a bridge (so switch is VLAN aware) and a VLAN device created on the bridge (br0.42) but with IGMP snooping turned off (so we don't get HOST_MDB notifications with correct VLAN ID). Maybe keeping the "from" net_device within the address list that is processed by ndo_set_rx_mode() will do the job though? Then you can do things like: if (is_vlan_dev(ha->dev) && ha->dev != dev) vid = vlan_dev_vlan_id(ha->dev); and it should scale to any type of stacked device, regardless of VID or something else that we need? Can you remind me of your use case again? Is it because your switch has VLAN filtering enabled and you need to make sure that MC addresses on VLAN device get programmed into the switch's multicast database with correct VID?
On 1/7/19 9:01 PM, Florian Fainelli wrote: > Le 12/4/18 à 4:04 PM, Ivan Khoronzhuk a écrit : >> On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: >> >> ... >> >>>> >>>> I was thinking also about pinned list of vlans to the address, but in >>>> this case this information also has to be synced by members of device >>>> chain, >>>> because it can be modified on any device level and it looks not very >>>> friendly, >>>> and at the end address space has addresses with pinned lists of vlans >>>> with >>>> their pointers. But keeping this stuff in sync is not simplest decision. >>>> >>>> >>> >>> I really think we are not communicating properly, it really seems to me >>> that if you had the information about the upper device trying to add an >>> address to the lower device filter's either through notification or call >>> to ndo_set_rxmode, you could be solving your problems. What are we >>> missing here? >> >> Sry, missed this one. The problem in getting the owner of address. >> Just simple case: vlan/macvlan/real_dev or vlan/.../.../real_dev >> >> The real dev hasn't simple way to get vid the address belong to, or it has? > > Humm looks like your right, by the time the address lists are > synchronized (e.g: from = vlan_dev, to = real_dev), we lost that > information. It looks like I just managed to find such an use case > myself with VLAN filtering enabled on a bridge (so switch is VLAN aware) > and a VLAN device created on the bridge (br0.42) but with IGMP snooping > turned off (so we don't get HOST_MDB notifications with correct VLAN ID). > > Maybe keeping the "from" net_device within the address list that is > processed by ndo_set_rx_mode() will do the job though? > > Then you can do things like: > > if (is_vlan_dev(ha->dev) && ha->dev != dev) > vid = vlan_dev_vlan_id(ha->dev); > > and it should scale to any type of stacked device, regardless of VID or > something else that we need? > > Can you remind me of your use case again? Is it because your switch has > VLAN filtering enabled and you need to make sure that MC addresses on > VLAN device get programmed into the switch's multicast database with > correct VID? Ivan, can you see if the following would work for you: https://github.com/ffainelli/linux/commit/19e173ebdcdd32f5f5b5ef29049e35d33ad058f2 this should be more scalable approach in that we can support HOST_MDB notifications from the bridge, the same way we would get notified about IGMP snooping from the bridge and this does not impact any other driver than those that elect to receive switchdev object notifications, which cpsw should really implement by now...
On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote: > On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: >> On 12/4/18 10:57 AM, Ivan Khoronzhuk wrote: >>> On Mon, Dec 03, 2018 at 03:57:03PM -0800, Florian Fainelli wrote: >>>> On 12/3/18 3:51 PM, Ivan Khoronzhuk wrote: >>>>> On Mon, Dec 03, 2018 at 02:17:00PM -0800, Florian Fainelli wrote: >>>>>> On 12/3/18 10:40 AM, Ivan Khoronzhuk wrote: >>>>>>> Update vlan mc and uc addresses with VID tag while propagating >>>>>>> address >>>>>>> set to lower devices, do this only if address is not synched. It >>>>>>> allows >>>>>>> on end driver level to distinguish address belonging to vlans. >>>>>> >>>>>> Underlying driver for the real device would be able to properly >>>>>> identify >>>>>> that you are attempting to add an address to a virtual device, which >>>>>> happens to be of VLAN kind so I am really not sure this is the right >>>>>> approach here. >>>>>> >>>>>> From there, it seems to me that we have two situations: >>>>>> >>>>>> - each of your network devices expose VLAN devices directly on top of >>>>>> the real device, in which case your driver should support >>>>>> ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid to know when VLAN >>>>>> devices >>>>>> are create and maintain a VLAN device to VID correspondence if it >>>>>> needs >>>>>> to when being called while setting the addresses >>>>>> >>>>>> - you are setting up a bridge that is VLAN aware on one of your >>>>>> bridge >>>>>> ports, and there you can use switchdev to learn about such events and >>>>>> know about both addresses as well as VIDs that must be programmed >>>>>> into >>>>>> your real device >>>>> No limits to have any "middle" device between real end device and >>>>> virtual one, not only a bridge, but also other kind. And as it's >>>>> generic >>>>> change, it should cover all such cases, the simplest example is: >>>>> real_dev/macvlan/vlan. >>>> >>>> It is not generic if the additional information is a VLAN ID, that >>>> construct does not apply to all types of virtual devices, that is part >>>> of my issue with the extra VID that is being added. If this was a >>>> void * >>>> priv and any virtual device could pass up/down information that >>>> might be >>>> more acceptable. >>> >>> You mean to create smth like common struct pinned to "an address" and >>> pass information not only like vid, but in parallel what ever user >>> wanted. >>> Even if pass vlan device pointer it still considered like an address >>> continuation and same sync method is used w/o modification. And here vid >>> is considered as part of address, by a big account address+vid it's a >>> separate address, same happens with the pointer, address+pointer it's >>> still separate address. >> >> That depends on the HW implementation, some switches do individual VLAN >> learning (IVL) and some do shared VLAN learning (SVL) so whether the VID >> becomes part of the address resolution logic is HW dependent, obviously >> the more capable, the better (IVL). > > In my case IVL is only choice, as SVL is rather imitation, as each vlan > has it's own address table anyway. And I mean not only vlan configuration > above the bridge but also any simple configuration above real device. > > There is proposition to add smth like additional list of entries pinned > to the each address as you proposed, but in a little bit different way. > > Pin the context pointers to each address if IVL config is enabled. > Smth like > > +struct ctx_entry { > + void *info; > + unsigned type; > + int synced; > + int sync_cnt; > + int refcount; > +} > > Then in hw_addr struct add a ctx_list: > > struct netdev_hw_addr { > struct list_head list; > + struct list_head ctx_list; > unsigned char addr[MAX_ADDR_LEN]; > unsigned char type; > ..... > } > > Each ctx_entry contains pointer to some structure, in my case it could be > pointer on vlan net_dev, and it can be marked with type > VLAN_DEVICE_POINTER or > else. In some other invisible cases it could be another one. Main > difference > between each of them is its pointer and type. > > And once each net dev calls mc/uc_sync these entires, if not synced, are > synced > along with addresses. But main difference that these ctx entires are > pinned to > the address, when addresses are pinned to the device. > > It can allow to bring information any new abstraction can apply. > For real device the list can be empty or contain special entry to differ > it from the vlan device entries, as could happen only some vlan is address > owner. > > Not sure it can be much simpler but it definitely can introduce more > capabilities, and potentially cover some other cases including your. > > Probably I need rename the series on smth like: > "make addressing scheme to be IVL capable" > > and send RFCv2 > > Thanks for your comment, it's really valuable. Ivan, based on the recent submission I copied you on [1], it sounds like we want to move ahead with your proposal to extend netdev_hw_addr with a vid member. On second thought, your approach is good and if we enclose the vid member within an #if IS_ENABLED(CONFIG_VLAN)8021Q) we should be good for most foreseeable use cases, if not, we can always introduce a variable size/defined context in the future. Can you resubmit this patch series as non-RFC in the next few days so I can also repost mine [1] and take advantage of these changes for multicast over VLAN when VLAN filtering is globally enabled on the device. [1]: https://www.spinics.net/lists/netdev/msg544722.html Thanks!
On Mon, Jan 21, 2019 at 03:37:41PM -0800, Florian Fainelli wrote: >On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote: >> On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: [...] > >Ivan, based on the recent submission I copied you on [1], it sounds like >we want to move ahead with your proposal to extend netdev_hw_addr with a >vid member. > >On second thought, your approach is good and if we enclose the vid >member within an #if IS_ENABLED(CONFIG_VLAN)8021Q) we should be good for >most foreseeable use cases, if not, we can always introduce a variable >size/defined context in the future. > >Can you resubmit this patch series as non-RFC in the next few days so I >can also repost mine [1] and take advantage of these changes for >multicast over VLAN when VLAN filtering is globally enabled on the device. > >[1]: https://www.spinics.net/lists/netdev/msg544722.html > >Thanks! Yes, sure. I can start to do that in several days. Just a little busy right now. Just before doing this, maybe some comments could be added as it has more attention now. Meanwhile I can send alternative variant but based on real dev splitting addresses between vlans. In this approach it leaves address space w/o vid extension but requires more changes to vlan core. Drawback here that to change one address alg traverses all related vlan addresses, it can be cpu/time wasteful, if it's done regularly, but saves memory.... Basically it's implemented locally in cpsw and requires more changes to move it as some vlan core auxiliary functions to be reused. But it can work only with vlans directly on top of real dev, which is fixable. Core function here: __hw_addr_ref_sync_dev it is called only for address the link of which was increased/decreased, thus update made only on one address, comparing it for every vlan dev. It was added with this patch: [1] net: core: dev_addr_lists: add auxiliary func to handle reference address update e7946760de5852f32 And used by this patch: [2] net: ethernet: ti: cpsw: fix vlan mcast 15180eca569bfe1d4d So, idea is to move [2] to be vlan core auxiliary function to be reused by NIC drivers. But potentially it can bring a little more changes I assume: 1) add priv_flag |= IFF_IV_FLT (independent vlan filtering). It allows to reuse this flag for farther changes, probably for per vlan allmulti or so. 2) real dev has to have complete list for vlans, not only their vids, but also all vlandevs in device chain above it. So changes in add_vid can be required. Vlan core can assign vlan dev pointer to real device only after it's completely initialized. And for propagation reasons it requires every device in infrastructure to be aware. That seems doable, but depends not only on me. 3) Move code from [2] to be auxiliary vlan core API for setting mc and uc. From this patch only one function is cpsw specific: cpsw_set_mc(). The rest can be applicable on every NIC supporting IFF_IV_FLT. 4) Move code from link below to do the same but for uc addresses: https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/commit/?h=ucast_vlan_fix&id=ebc88a7d8758759322d9ff88f25f8bac51ce7219 here only one func cpsw specific: cpsw_set_uc() the rest can be generic. As third alternative, we can think about how to reduce memory for addresses by reusing them or else, but this is as continuation of addr+vid approach, and API probably would be the same. Then all this can be compared for proper decision. >-- >Florian
On Tue, Jan 08, 2019 at 10:21:25AM -0800, Florian Fainelli wrote: >On 1/7/19 9:01 PM, Florian Fainelli wrote: >> Le 12/4/18 à 4:04 PM, Ivan Khoronzhuk a écrit : >>> On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: >>> >>> ... >>> >>>>> >>>>> I was thinking also about pinned list of vlans to the address, but in >>>>> this case this information also has to be synced by members of device >>>>> chain, >>>>> because it can be modified on any device level and it looks not very >>>>> friendly, >>>>> and at the end address space has addresses with pinned lists of vlans >>>>> with >>>>> their pointers. But keeping this stuff in sync is not simplest decision. >>>>> >>>>> >>>> >>>> I really think we are not communicating properly, it really seems to me >>>> that if you had the information about the upper device trying to add an >>>> address to the lower device filter's either through notification or call >>>> to ndo_set_rxmode, you could be solving your problems. What are we >>>> missing here? >>> >>> Sry, missed this one. The problem in getting the owner of address. >>> Just simple case: vlan/macvlan/real_dev or vlan/.../.../real_dev >>> >>> The real dev hasn't simple way to get vid the address belong to, or it has? >> >> Humm looks like your right, by the time the address lists are >> synchronized (e.g: from = vlan_dev, to = real_dev), we lost that >> information. It looks like I just managed to find such an use case >> myself with VLAN filtering enabled on a bridge (so switch is VLAN aware) >> and a VLAN device created on the bridge (br0.42) but with IGMP snooping >> turned off (so we don't get HOST_MDB notifications with correct VLAN ID). >> >> Maybe keeping the "from" net_device within the address list that is >> processed by ndo_set_rx_mode() will do the job though? >> >> Then you can do things like: >> >> if (is_vlan_dev(ha->dev) && ha->dev != dev) >> vid = vlan_dev_vlan_id(ha->dev); >> >> and it should scale to any type of stacked device, regardless of VID or >> something else that we need? >> >> Can you remind me of your use case again? Is it because your switch has >> VLAN filtering enabled and you need to make sure that MC addresses on >> VLAN device get programmed into the switch's multicast database with >> correct VID? > >Ivan, can you see if the following would work for you: > >https://github.com/ffainelli/linux/commit/19e173ebdcdd32f5f5b5ef29049e35d33ad058f2 > >this should be more scalable approach in that we can support HOST_MDB >notifications from the bridge, the same way we would get notified about >IGMP snooping from the bridge and this does not impact any other driver >than those that elect to receive switchdev object notifications, which >cpsw should really implement by now... Sorry missed 2 last comments and seems like it's clear now. I need to be in TO or CC list my filters can parse it, probably I need to create more smart filters. >-- >Florian
On Tue, Jan 22, 2019 at 03:12:41PM +0200, Ivan Khoronzhuk wrote: >On Mon, Jan 21, 2019 at 03:37:41PM -0800, Florian Fainelli wrote: >>On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote: >>>On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: > >[...] > >> >>Ivan, based on the recent submission I copied you on [1], it sounds like >>we want to move ahead with your proposal to extend netdev_hw_addr with a >>vid member. >> >>On second thought, your approach is good and if we enclose the vid >>member within an #if IS_ENABLED(CONFIG_VLAN)8021Q) we should be good for >>most foreseeable use cases, if not, we can always introduce a variable >>size/defined context in the future. >> >>Can you resubmit this patch series as non-RFC in the next few days so I >>can also repost mine [1] and take advantage of these changes for >>multicast over VLAN when VLAN filtering is globally enabled on the device. >> >>[1]: https://www.spinics.net/lists/netdev/msg544722.html >> >>Thanks! > >Yes, sure. I can start to do that in several days. >Just a little busy right now. > >Just before doing this, maybe some comments could be added as it has more >attention now. Meanwhile I can send alternative variant but based on >real dev splitting addresses between vlans. In this approach it leaves address >space w/o vid extension but requires more changes to vlan core. Drawback here >that to change one address alg traverses all related vlan addresses, it can be >cpu/time wasteful, if it's done regularly, but saves memory.... > >Basically it's implemented locally in cpsw and requires more changes to move >it as some vlan core auxiliary functions to be reused. But it can work only >with vlans directly on top of real dev, which is fixable. > >Core function here: >__hw_addr_ref_sync_dev >it is called only for address the link of which was increased/decreased, thus >update made only on one address, comparing it for every vlan dev. > >It was added with this patch: >[1] net: core: dev_addr_lists: add auxiliary func to handle reference >address update e7946760de5852f32 > >And used by this patch: >[2] net: ethernet: ti: cpsw: fix vlan mcast 15180eca569bfe1d4d > >So, idea is to move [2] to be vlan core auxiliary function to be reused >by NIC drivers. > >But potentially it can bring a little more changes I assume: > >1) add priv_flag |= IFF_IV_FLT (independent vlan filtering). It allows to reuse >this flag for farther changes, probably for per vlan allmulti or so. > >2) real dev has to have complete list for vlans, not only their vids, but also >all vlandevs in device chain above it. So changes in add_vid can be required. >Vlan core can assign vlan dev pointer to real device only after it's completely >initialized. And for propagation reasons it requires every device in >infrastructure to be aware. That seems doable, but depends not only on me. > >3) Move code from [2] to be auxiliary vlan core API for setting mc and uc. >From this patch only one function is cpsw specific: cpsw_set_mc(). The rest can >be applicable on every NIC supporting IFF_IV_FLT. > >4) Move code from link below to do the same but for uc addresses: >https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/commit/?h=ucast_vlan_fix&id=ebc88a7d8758759322d9ff88f25f8bac51ce7219 >here only one func cpsw specific: cpsw_set_uc() >the rest can be generic. > >As third alternative, we can think about how to reduce memory for addresses by >reusing them or else, but this is as continuation of addr+vid approach, and API >probably would be the same. > >Then all this can be compared for proper decision. Hi Florian, After several more investigations and tries probably better left this idea as is. Here actually several explanations for this: 1) If even assume that we can get access to vlan devices in the above ndev tree (we can) that doesn't guarantee that receive vlan filters are set replicating this structure. For example bond device can have one active slave but both of them in the tree having vid set, in this case addresses are syched only with active slave, no filters should be applied to not active slave. this can be achieved only each address has vid context. 2) According to 1) rx filters device structure can be created while mc_sync() in each rx_mode(), and then used as orthogonal info. I've tried and it looks not cool and consumes anyway memory and even if it's less it's still not very scalable. (+ no normal signal "in complex structure case" when address should be undated to avoid redundant cpu cycles). Not sure it can have practical results and be universal enouph. 3) Assuming that every device in the tree (bond, team or else) is legal to modify its own address space, the real end device cannot be sure the vlan device address spaces reflects vid addresses that device tree want's from him. According to this each address in address space must hold its own context at every device and this context is comparable with address size. >-- Regards, >Ivan Khoronzhuk
On February 13, 2019 8:17:16 AM PST, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: >On Tue, Jan 22, 2019 at 03:12:41PM +0200, Ivan Khoronzhuk wrote: >>On Mon, Jan 21, 2019 at 03:37:41PM -0800, Florian Fainelli wrote: >>>On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote: >>>>On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: >> >>[...] >> >>> >>>Ivan, based on the recent submission I copied you on [1], it sounds >like >>>we want to move ahead with your proposal to extend netdev_hw_addr >with a >>>vid member. >>> >>>On second thought, your approach is good and if we enclose the vid >>>member within an #if IS_ENABLED(CONFIG_VLAN)8021Q) we should be good >for >>>most foreseeable use cases, if not, we can always introduce a >variable >>>size/defined context in the future. >>> >>>Can you resubmit this patch series as non-RFC in the next few days so >I >>>can also repost mine [1] and take advantage of these changes for >>>multicast over VLAN when VLAN filtering is globally enabled on the >device. >>> >>>[1]: https://www.spinics.net/lists/netdev/msg544722.html >>> >>>Thanks! >> >>Yes, sure. I can start to do that in several days. >>Just a little busy right now. >> >>Just before doing this, maybe some comments could be added as it has >more >>attention now. Meanwhile I can send alternative variant but based on >>real dev splitting addresses between vlans. In this approach it leaves >address >>space w/o vid extension but requires more changes to vlan core. >Drawback here >>that to change one address alg traverses all related vlan addresses, >it can be >>cpu/time wasteful, if it's done regularly, but saves memory.... >> >>Basically it's implemented locally in cpsw and requires more changes >to move >>it as some vlan core auxiliary functions to be reused. But it can work >only >>with vlans directly on top of real dev, which is fixable. >> >>Core function here: >>__hw_addr_ref_sync_dev >>it is called only for address the link of which was >increased/decreased, thus >>update made only on one address, comparing it for every vlan dev. >> >>It was added with this patch: >>[1] net: core: dev_addr_lists: add auxiliary func to handle reference >>address update e7946760de5852f32 >> >>And used by this patch: >>[2] net: ethernet: ti: cpsw: fix vlan mcast 15180eca569bfe1d4d >> >>So, idea is to move [2] to be vlan core auxiliary function to be >reused >>by NIC drivers. >> >>But potentially it can bring a little more changes I assume: >> >>1) add priv_flag |= IFF_IV_FLT (independent vlan filtering). It allows >to reuse >>this flag for farther changes, probably for per vlan allmulti or so. >> >>2) real dev has to have complete list for vlans, not only their vids, >but also >>all vlandevs in device chain above it. So changes in add_vid can be >required. >>Vlan core can assign vlan dev pointer to real device only after it's >completely >>initialized. And for propagation reasons it requires every device in >>infrastructure to be aware. That seems doable, but depends not only on >me. >> >>3) Move code from [2] to be auxiliary vlan core API for setting mc and >uc. >>From this patch only one function is cpsw specific: cpsw_set_mc(). The >rest can >>be applicable on every NIC supporting IFF_IV_FLT. >> >>4) Move code from link below to do the same but for uc addresses: >>https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/commit/?h=ucast_vlan_fix&id=ebc88a7d8758759322d9ff88f25f8bac51ce7219 >>here only one func cpsw specific: cpsw_set_uc() >>the rest can be generic. >> >>As third alternative, we can think about how to reduce memory for >addresses by >>reusing them or else, but this is as continuation of addr+vid >approach, and API >>probably would be the same. >> >>Then all this can be compared for proper decision. > > >Hi Florian, > >After several more investigations and tries probably better left this >idea as is. Thank you for keeping the thread alive, does that mean you are going to resubmit this patch series as-is (rebased) or are you saying that you are abandoning the idea and leaving the situation the way it is in cpsw? > >Here actually several explanations for this: >1) If even assume that we can get access to vlan devices in the above >ndev >tree (we can) that doesn't guarantee that receive vlan filters are set >replicating this structure. For example bond device can have one active >slave >but both of them in the tree having vid set, in this case addresses are >syched only with active slave, no filters should be applied to not >active slave. >this can be achieved only each address has vid context. > >2) According to 1) rx filters device structure can be created while >mc_sync() >in each rx_mode(), and then used as orthogonal info. I've tried and it >looks >not cool and consumes anyway memory and even if it's less it's still >not very >scalable. (+ no normal signal "in complex structure case" when address >should >be undated to avoid redundant cpu cycles). Not sure it can have >practical >results and be universal enouph. > >3) Assuming that every device in the tree (bond, team or else) is legal >to >modify its own address space, the real end device cannot be sure the >vlan device >address spaces reflects vid addresses that device tree want's from him. >According to this each address in address space must hold its own >context at >every device and this context is comparable with address size. > >>-- Regards, >>Ivan Khoronzhuk
On Wed, Feb 13, 2019 at 08:49:39PM -0800, Florian Fainelli wrote: > > >On February 13, 2019 8:17:16 AM PST, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: >>On Tue, Jan 22, 2019 at 03:12:41PM +0200, Ivan Khoronzhuk wrote: >>>On Mon, Jan 21, 2019 at 03:37:41PM -0800, Florian Fainelli wrote: >>>>On 12/4/18 3:42 PM, Ivan Khoronzhuk wrote: >>>>>On Tue, Dec 04, 2018 at 11:49:27AM -0800, Florian Fainelli wrote: >>> >>>[...] >>> >>>> >>>>Ivan, based on the recent submission I copied you on [1], it sounds >>like >>>>we want to move ahead with your proposal to extend netdev_hw_addr >>with a >>>>vid member. >>>> >>>>On second thought, your approach is good and if we enclose the vid >>>>member within an #if IS_ENABLED(CONFIG_VLAN)8021Q) we should be good >>for >>>>most foreseeable use cases, if not, we can always introduce a >>variable >>>>size/defined context in the future. >>>> >>>>Can you resubmit this patch series as non-RFC in the next few days so >>I >>>>can also repost mine [1] and take advantage of these changes for >>>>multicast over VLAN when VLAN filtering is globally enabled on the >>device. >>>> >>>>[1]: https://www.spinics.net/lists/netdev/msg544722.html >>>> >>>>Thanks! >>> >>>Yes, sure. I can start to do that in several days. >>>Just a little busy right now. >>> >>>Just before doing this, maybe some comments could be added as it has >>more >>>attention now. Meanwhile I can send alternative variant but based on >>>real dev splitting addresses between vlans. In this approach it leaves >>address >>>space w/o vid extension but requires more changes to vlan core. >>Drawback here >>>that to change one address alg traverses all related vlan addresses, >>it can be >>>cpu/time wasteful, if it's done regularly, but saves memory.... >>> >>>Basically it's implemented locally in cpsw and requires more changes >>to move >>>it as some vlan core auxiliary functions to be reused. But it can work >>only >>>with vlans directly on top of real dev, which is fixable. >>> >>>Core function here: >>>__hw_addr_ref_sync_dev >>>it is called only for address the link of which was >>increased/decreased, thus >>>update made only on one address, comparing it for every vlan dev. >>> >>>It was added with this patch: >>>[1] net: core: dev_addr_lists: add auxiliary func to handle reference >>>address update e7946760de5852f32 >>> >>>And used by this patch: >>>[2] net: ethernet: ti: cpsw: fix vlan mcast 15180eca569bfe1d4d >>> >>>So, idea is to move [2] to be vlan core auxiliary function to be >>reused >>>by NIC drivers. >>> >>>But potentially it can bring a little more changes I assume: >>> >>>1) add priv_flag |= IFF_IV_FLT (independent vlan filtering). It allows >>to reuse >>>this flag for farther changes, probably for per vlan allmulti or so. >>> >>>2) real dev has to have complete list for vlans, not only their vids, >>but also >>>all vlandevs in device chain above it. So changes in add_vid can be >>required. >>>Vlan core can assign vlan dev pointer to real device only after it's >>completely >>>initialized. And for propagation reasons it requires every device in >>>infrastructure to be aware. That seems doable, but depends not only on >>me. >>> >>>3) Move code from [2] to be auxiliary vlan core API for setting mc and >>uc. >>>From this patch only one function is cpsw specific: cpsw_set_mc(). The >>rest can >>>be applicable on every NIC supporting IFF_IV_FLT. >>> >>>4) Move code from link below to do the same but for uc addresses: >>>https://git.linaro.org/people/ivan.khoronzhuk/tsn_kernel.git/commit/?h=ucast_vlan_fix&id=ebc88a7d8758759322d9ff88f25f8bac51ce7219 >>>here only one func cpsw specific: cpsw_set_uc() >>>the rest can be generic. >>> >>>As third alternative, we can think about how to reduce memory for >>addresses by >>>reusing them or else, but this is as continuation of addr+vid >>approach, and API >>>probably would be the same. >>> >>>Then all this can be compared for proper decision. >> >> >>Hi Florian, >> >>After several more investigations and tries probably better left this >>idea as is. > >Thank you for keeping the thread alive, does that mean you are going to resubmit this patch series as-is (rebased) or are you saying that you are abandoning the idea and leaving the situation the way it is in cpsw? I will resubmit this one. But: I have to try one more approach before this. The idea is to create simple rx flt device tree while mc/us sync. Then use it at real device to dispatch addresses. It increases hw_addr struct a little and code base, But: - no need to keep linearly all vlan addresses in one address space. - replicates RX filtering struct of net devices, (but not logical tree of netdevs) - keeps devs info per address. - no need to change addr lenth and modify existent API - access at any net dev to above rx flt device structure per address - potentially can be use not only for vlan devs identification but for other rx path offloads. Idea is simple but not completely verified it yet, need a little bit more time to prove/clean ...or drop it. > >> >>Here actually several explanations for this: >>1) If even assume that we can get access to vlan devices in the above >>ndev >>tree (we can) that doesn't guarantee that receive vlan filters are set >>replicating this structure. For example bond device can have one active >>slave >>but both of them in the tree having vid set, in this case addresses are >>syched only with active slave, no filters should be applied to not >>active slave. >>this can be achieved only each address has vid context. >> >>2) According to 1) rx filters device structure can be created while >>mc_sync() >>in each rx_mode(), and then used as orthogonal info. I've tried and it >>looks >>not cool and consumes anyway memory and even if it's less it's still >>not very >>scalable. (+ no normal signal "in complex structure case" when address >>should >>be undated to avoid redundant cpu cycles). Not sure it can have >>practical >>results and be universal enouph. >> >>3) Assuming that every device in the tree (bond, team or else) is legal >>to >>modify its own address space, the real end device cannot be sure the >>vlan device >>address spaces reflects vid addresses that device tree want's from him. >>According to this each address in address space must hold its own >>context at >>every device and this context is comparable with address size. >> >>>-- Regards, >>>Ivan Khoronzhuk > >-- >Florian
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_core.c b/net/8021q/vlan_core.c index a313165e7a67..5d17947d6988 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -454,6 +454,16 @@ 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; + + 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 b2d9c8f27cd7..c05b313314b7 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -250,6 +250,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) { @@ -481,8 +489,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 address set to lower devices, do this only if address is not synched. It allows on end driver level to distinguish address belonging to vlans. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- include/linux/if_vlan.h | 1 + net/8021q/vlan_core.c | 10 ++++++++++ net/8021q/vlan_dev.c | 26 ++++++++++++++++++++++++++ 3 files changed, 37 insertions(+)