On 29/08/2022 23:22, Sevinj Aghayeva wrote: > Hi Nik, > > Please see below. > > I had to export call_netdev_notifiers_info. Also, I wanted to define the vlan-specific detail change struct in if_vlan.h and then include if_vlan.h from netdevice.h, where the netdev_notifier_change_details_info struct was defined, but I found out that I cannot include if_vlan.h in netdevice.h. After consulting with Andy I added a new file for the netdev_notifier_change_details_info. Again, I will send out a proper RFC patch once we sort out the details. > Hi, Please don't top post on netdev@. Sounds ok to me, just break it into separate logical pieces and please finally send a complete RFC series, reviewing pieces like this is not preferrable. A few comments below. > diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h > index 2be4dd7e90a9..ad0eddf1386f 100644 > --- a/include/linux/if_vlan.h > +++ b/include/linux/if_vlan.h > @@ -37,6 +37,10 @@ struct vlan_hdr { > __be16 h_vlan_encapsulated_proto; > }; > > +struct vlan_change_details { > + bool bridge_binding; > +}; > + > /** > * struct vlan_ethhdr - vlan ethernet header (ethhdr + vlan_hdr) > * @h_dest: destination ethernet address > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 2563d30736e9..ba4746f05d7f 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -2762,6 +2762,7 @@ enum netdev_cmd { > NETDEV_UNREGISTER, > NETDEV_CHANGEMTU, /* notify after mtu change happened */ > NETDEV_CHANGEADDR, /* notify after the address change */ > + NETDEV_CHANGE_DETAILS, > NETDEV_PRE_CHANGEADDR, /* notify before the address change */ > NETDEV_GOING_DOWN, > NETDEV_CHANGENAME, > @@ -2898,6 +2899,8 @@ netdev_notifier_info_to_extack(const struct netdev_notifier_info *info) > } > > int call_netdevice_notifiers(unsigned long val, struct net_device *dev); > +int call_netdevice_notifiers_info(unsigned long val, > + struct netdev_notifier_info *info); > > > extern rwlock_t dev_base_lock; /* Device list lock */ > diff --git a/include/linux/notifier_info.h b/include/linux/notifier_info.h > new file mode 100644 > index 000000000000..c103240588dc > --- /dev/null > +++ b/include/linux/notifier_info.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef _LINUX_NOTIFIER_INFO_H_ > +#define _LINUX_NOTIFIER_INFO_H_ > + > +#include <linux/netdevice.h> > +#include <linux/if_vlan.h> > + You can add a comment here about how the struct would be used > +struct netdev_notifier_change_details_info { > + struct netdev_notifier_info info; /* must be first */ > + union { > + struct vlan_change_details vlan; > + } details; union can be anonymous, no need to add another name here > +}; > + > +#endif /* !(_LINUX_NOTIFIER_INFO_H_) */ > diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h > index 5eaf38875554..71947cdcfaaa 100644 > --- a/net/8021q/vlan.h > +++ b/net/8021q/vlan.h > @@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct net_device *dev, > int vlan_dev_set_egress_priority(const struct net_device *dev, > u32 skb_prio, u16 vlan_prio); > void vlan_dev_free_egress_priority(const struct net_device *dev); > -int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask); > +int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask); > void vlan_dev_get_realdev_name(const struct net_device *dev, char *result, > size_t size); > > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > index 839f2020b015..d737e259e295 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -22,6 +22,7 @@ > #include <linux/skbuff.h> > #include <linux/netdevice.h> > #include <linux/net_tstamp.h> > +#include <linux/notifier_info.h> > #include <linux/etherdevice.h> > #include <linux/ethtool.h> > #include <linux/phy.h> > @@ -211,8 +212,9 @@ int vlan_dev_set_egress_priority(const struct net_device *dev, > /* Flags are defined in the vlan_flags enum in > * include/uapi/linux/if_vlan.h file. > */ > -int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask) > +int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask) > { > + struct netdev_notifier_change_details_info info; > struct vlan_dev_priv *vlan = vlan_dev_priv(dev); > u32 old_flags = vlan->flags; > > @@ -223,19 +225,31 @@ int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask) > > vlan->flags = (old_flags & ~mask) | (flags & mask); > > - if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) { > + if (!netif_running(dev)) > + return 0; > + > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) { > if (vlan->flags & VLAN_FLAG_GVRP) > vlan_gvrp_request_join(dev); > else > vlan_gvrp_request_leave(dev); > } > > - if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) { > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) { > if (vlan->flags & VLAN_FLAG_MVRP) > vlan_mvrp_request_join(dev); > else > vlan_mvrp_request_leave(dev); > } > + > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING && ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING) > + netif_is_bridge_master(vlan->real_dev)) { > + info.info.dev <http://info.info.dev> = dev; > + info.details.vlan.bridge_binding = > + !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING); > + call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info <http://info.info>); info.info?! > + } > + > return 0; > } > > diff --git a/net/bridge/br.c b/net/bridge/br.c > index 96e91d69a9a8..62e939c6a3f0 100644 > --- a/net/bridge/br.c > +++ b/net/bridge/br.c > @@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v > } > } > > + if (is_vlan_dev(dev)) { > + br_vlan_device_event(dev, event, ptr); > + return NOTIFY_DONE; > + } > + > /* not a port of a bridge */ > p = br_port_get_rtnl(dev); > if (!p) > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 06e5f6faa431..a9a08e49c76c 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -1470,6 +1470,8 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v, > void br_vlan_port_event(struct net_bridge_port *p, unsigned long event); > int br_vlan_bridge_event(struct net_device *dev, unsigned long event, > void *ptr); > +void br_vlan_device_event(struct net_device *dev, unsigned long event, > + void *ptr); > void br_vlan_rtnl_init(void); > void br_vlan_rtnl_uninit(void); > void br_vlan_notify(const struct net_bridge *br, > @@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct net_device *dev, > return 0; > } > > +static void br_vlan_device_event(struct net_device *dev, > + unsigned long event, void *ptr) > +{ > +} > + > static inline void br_vlan_rtnl_init(void) > { > } > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index 0f5e75ccac79..fe00efa46ce8 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-only > #include <linux/kernel.h> > #include <linux/netdevice.h> > +#include <linux/notifier_info.h> > #include <linux/rtnetlink.h> > #include <linux/slab.h> > #include <net/switchdev.h> > @@ -1768,6 +1769,23 @@ void br_vlan_port_event(struct net_bridge_port *p, unsigned long event) > } > } > > +void br_vlan_device_event(struct net_device *dev, unsigned long event, void *ptr) > +{ > + struct netdev_notifier_change_details_info *info; > + struct net_device *br_dev; > + > + switch (event) { > + case NETDEV_CHANGE_DETAILS: > + info = ptr; > + if (netif_is_bridge_master(vlan_dev_priv(dev)->real_dev)) { no need for the indentation here, just check the opposite and break if it's not > + br_dev = vlan_dev_priv(dev)->real_dev; > + br_vlan_upper_change(br_dev, dev, > + info->details.vlan.bridge_binding); > + } > + break; > + } > +} > + > static bool br_vlan_stats_fill(struct sk_buff *skb, > const struct net_bridge_vlan *v) > { > diff --git a/net/core/dev.c b/net/core/dev.c > index 30a1603a7225..2eed064e9529 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -160,7 +160,7 @@ struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly; > struct list_head ptype_all __read_mostly; /* Taps */ > > static int netif_rx_internal(struct sk_buff *skb); > -static int call_netdevice_notifiers_info(unsigned long val, > +int call_netdevice_notifiers_info(unsigned long val, > struct netdev_notifier_info *info); you have to move this to its proper header file > static int call_netdevice_notifiers_extack(unsigned long val, > struct net_device *dev, > @@ -1624,7 +1624,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd) > N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER) > N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO) > N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO) > - N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) > + N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS) > N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO) > N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO) > N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE) > @@ -1927,7 +1927,7 @@ static void move_netdevice_notifiers_dev_net(struct net_device *dev, > * are as for raw_notifier_call_chain(). > */ > > -static int call_netdevice_notifiers_info(unsigned long val, > +int call_netdevice_notifiers_info(unsigned long val, > struct netdev_notifier_info *info) > { > struct net *net = dev_net(info->dev); > @@ -1944,6 +1944,7 @@ static int call_netdevice_notifiers_info(unsigned long val, > return ret; > return raw_notifier_call_chain(&netdev_chain, val, info); > } > +EXPORT_SYMBOL(call_netdevice_notifiers_info); hmm, I see that call_netdevice_notifiers() is EXPORT_SYMBOL() only (not the _GPL) I wonder if this should instead be EXPORT_SYMBOL_GPL() as it's a new export > > /** > * call_netdevice_notifiers_info_robust - call per-netns notifier blocks > > On Mon, Aug 22, 2022 at 7:18 PM Sevinj Aghayeva <sevinj.aghayeva@xxxxxxxxx <mailto:sevinj.aghayeva@xxxxxxxxx>> wrote: > > > > On Mon, Aug 22, 2022 at 4:01 AM Nikolay Aleksandrov <razor@xxxxxxxxxxxxx <mailto:razor@xxxxxxxxxxxxx>> wrote: > > On 20/08/2022 14:33, Sevinj Aghayeva wrote: > > On Thu, Aug 18, 2022 at 8:00 AM Nikolay Aleksandrov <razor@xxxxxxxxxxxxx <mailto:razor@xxxxxxxxxxxxx>> wrote: > >> > >> On 18/08/2022 14:50, Sevinj Aghayeva wrote: > [snip] > >> > >> Hi, > >> Handling all vlan device-related changes in br_vlan_device_event() sounds good to me. > >> Please add it to br_vlan.c. > > > > Hi Nik, > > > > Can you please review this diff before I make it into a proper patchset? Thanks! > > > > Hi, > A few comments inline below, but in general when you prepare the rfc commit please > explain the motivation in detail why this way was chosen and a new notification type > is needed (e.g. why not use NETDEV_CHANGEINFODATA or extend NETDEV_CHANGE). > As I mentioned earlier it'd be nice to get feedback from others about adding this > new notification, so they should know the "why" in detail. > > > Sure, I will do that in the RFC patchset, but first I want to make sure I got the details right before making the RFC patchset. Thanks for the feedback. Please see inline. > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index 2563d30736e9..0ce3da42325e 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -2762,6 +2762,7 @@ enum netdev_cmd { > > NETDEV_UNREGISTER, > > NETDEV_CHANGEMTU, /* notify after mtu change happened */ > > NETDEV_CHANGEADDR, /* notify after the address change */ > > + NETDEV_CHANGE_DETAILS, > > NETDEV_PRE_CHANGEADDR, /* notify before the address change */ > > NETDEV_GOING_DOWN, > > NETDEV_CHANGENAME, > > @@ -2837,6 +2838,13 @@ struct netdev_notifier_changelowerstate_info { > > void *lower_state_info; /* is lower dev state */ > > }; > > > > +struct netdev_notifier_change_details_info { > > + struct netdev_notifier_info info; /* must be first */ > > + union { > > + bool bridge_binding; > > this should be in a vlan-specific structure, defined in if_vlan.h > every other link type which wants to use the notification would define its > own struct type > > > Okay, will move it there. > > > > > + } details; > > +}; > > + > > struct netdev_notifier_pre_changeaddr_info { > > struct netdev_notifier_info info; /* must be first */ > > const unsigned char *dev_addr; > > @@ -3836,6 +3844,8 @@ int __dev_set_mtu(struct net_device *, int); > > int dev_set_mtu(struct net_device *, int); > > int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr, > > struct netlink_ext_ack *extack); > > +int dev_change_details_notify(struct net_device *dev, bool bridge_binding, > > + struct netlink_ext_ack *extack); > > this helper is not needed > > > > int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, > > struct netlink_ext_ack *extack); > > int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa, > > diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h > > index 5eaf38875554..71947cdcfaaa 100644 > > --- a/net/8021q/vlan.h > > +++ b/net/8021q/vlan.h > > @@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct > > net_device *dev, > > int vlan_dev_set_egress_priority(const struct net_device *dev, > > u32 skb_prio, u16 vlan_prio); > > void vlan_dev_free_egress_priority(const struct net_device *dev); > > -int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask); > > +int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask); > > void vlan_dev_get_realdev_name(const struct net_device *dev, char *result, > > size_t size); > > > > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > > index 839f2020b015..489baa8435de 100644 > > --- a/net/8021q/vlan_dev.c > > +++ b/net/8021q/vlan_dev.c > > @@ -211,7 +211,7 @@ int vlan_dev_set_egress_priority(const struct > > net_device *dev, > > /* Flags are defined in the vlan_flags enum in > > * include/uapi/linux/if_vlan.h file. > > */ > > -int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask) > > +int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask) > > please don't remove the const, this function shouldn't change dev's struct > > > I tried not to remove const, but it seems impossible because call_netdevice_notifiers_info that we eventually call from vlan_dev_change_flags takes a non-const struct info that has a dev field being set from the const vlan_dev_change_flags. > > > > > { > > struct vlan_dev_priv *vlan = vlan_dev_priv(dev); > > u32 old_flags = vlan->flags; > > @@ -223,19 +223,29 @@ int vlan_dev_change_flags(const struct > > net_device *dev, u32 flags, u32 mask) > > > > vlan->flags = (old_flags & ~mask) | (flags & mask); > > > > - if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) { > > + if (!netif_running(dev)) > > + return 0; > > + > > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) { > > if (vlan->flags & VLAN_FLAG_GVRP) > > vlan_gvrp_request_join(dev); > > else > > vlan_gvrp_request_leave(dev); > > } > > > > - if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) { > > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) { > > if (vlan->flags & VLAN_FLAG_MVRP) > > vlan_mvrp_request_join(dev); > > else > > vlan_mvrp_request_leave(dev); > > } > > + > > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING && > > + netif_is_bridge_master(vlan->real_dev)) { > > + dev_change_details_notify(dev, > > + !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING), NULL); > > this helper is not needed, just fill in the details here and send the notification > > > Okay, in that case I will need to export call_netdevice_notifiers_info so that I can call it from here. > > > + } > > + > > return 0; > > } > > > > diff --git a/net/bridge/br.c b/net/bridge/br.c > > index 96e91d69a9a8..62e939c6a3f0 100644 > > --- a/net/bridge/br.c > > +++ b/net/bridge/br.c > > @@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block > > *unused, unsigned long event, v > > } > > } > > > > + if (is_vlan_dev(dev)) { > > + br_vlan_device_event(dev, event, ptr); > > + return NOTIFY_DONE; > > + } > > + > > /* not a port of a bridge */ > > p = br_port_get_rtnl(dev); > > if (!p) > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > > index 06e5f6faa431..a9a08e49c76c 100644 > > --- a/net/bridge/br_private.h > > +++ b/net/bridge/br_private.h > > @@ -1470,6 +1470,8 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v, > > void br_vlan_port_event(struct net_bridge_port *p, unsigned long event); > > int br_vlan_bridge_event(struct net_device *dev, unsigned long event, > > void *ptr); > > +void br_vlan_device_event(struct net_device *dev, unsigned long event, > > + void *ptr); > > void br_vlan_rtnl_init(void); > > void br_vlan_rtnl_uninit(void); > > void br_vlan_notify(const struct net_bridge *br, > > @@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct > > net_device *dev, > > return 0; > > } > > > > +static void br_vlan_device_event(struct net_device *dev, > > + unsigned long event, void *ptr) > > +{ > > +} > > + > > static inline void br_vlan_rtnl_init(void) > > { > > } > > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > > index 0f5e75ccac79..70a9950df175 100644 > > --- a/net/bridge/br_vlan.c > > +++ b/net/bridge/br_vlan.c > > @@ -1768,6 +1768,20 @@ void br_vlan_port_event(struct net_bridge_port > > *p, unsigned long event) > > } > > } > > > > +void br_vlan_device_event(struct net_device *dev, unsigned long > > event, void *ptr) > > +{ > > + struct netdev_notifier_change_details_info *info; > > + struct net_device *br_dev; > > + > > + switch (event) { > > + case NETDEV_CHANGE_DETAILS: > > + info = ptr; > > + br_dev = vlan_dev_priv(dev)->real_dev; > > you're not guaranteed to have a bridge device as its real_dev, so you should > validate that the vlan's real dev is a bridge > > > Okay, will do. > > > > > + br_vlan_upper_change(br_dev, dev, info->details.bridge_binding); > > + break; > > + } > > +} > > + > > static bool br_vlan_stats_fill(struct sk_buff *skb, > > const struct net_bridge_vlan *v) > > { > > diff --git a/net/core/dev.c b/net/core/dev.c > > index 30a1603a7225..dcdbc625585d 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -1624,7 +1624,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd) > > N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER) > > N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO) > > N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO) > > - N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) > > + N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS) > > N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO) > > N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO) > > N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE) > > @@ -8767,6 +8767,27 @@ int dev_pre_changeaddr_notify(struct net_device > > *dev, const char *addr, > > } > > EXPORT_SYMBOL(dev_pre_changeaddr_notify); > > > > +/** > > + * dev_change_details_notify - Call NETDEV_PRE_CHANGE_DETAILS. > > + * @dev: device > > + * @bridge_binding: bridge binding setting > > + * @extack: netlink extended ack > > + */ > > +int dev_change_details_notify(struct net_device *dev, bool bridge_binding, > > + struct netlink_ext_ack *extack) > > +{ > > + struct netdev_notifier_change_details_info info = { > > + .info.dev <http://info.dev> = dev, > > + .info.extack = extack, > > + .details.bridge_binding = bridge_binding, > > + }; > > + int rc; > > + > > + rc = call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info <http://info.info>); > > + return notifier_to_errno(rc); > > +} > > +EXPORT_SYMBOL(dev_change_details_notify); > > + > > this helper is unnecessary, just fill in the struct at the caller site and > send the notification directly > > > Okay, will remove it. > > Thanks for the review! I will send the updated patch soon. > > > > /** > > * dev_set_mac_address - Change Media Access Control Address > > * @dev: device > > > > > >> > >> Thanks, > >> Nik > >> > >> > > Cheers, > Nik > > > > > -- > > Sevinj.Aghayeva > > > > -- > > Sevinj.Aghayeva