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! -- Florian