From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ido Schimmel Subject: Re: [PATCH v3,net-next] vlan: implement vlan id and protocol changes Date: Tue, 26 Jun 2018 18:29:06 +0300 Message-ID: <20180626152906.GA18460@splinter.mtl.com> References: <20180610231912.1543-1-3chas3@gmail.com> <20180625103043.25384-1-3chas3@gmail.com> <20180626103216.GA11722@splinter.mtl.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dsa@cumulusnetworks.com, "David S. Miller" , netdev , Roopa Prabhu , idosch@mellanox.com To: Chas Williams <3chas3@gmail.com> Return-path: Received: from out2-smtp.messagingengine.com ([66.111.4.26]:44423 "EHLO out2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751334AbeFZP3I (ORCPT ); Tue, 26 Jun 2018 11:29:08 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Jun 26, 2018 at 09:33:40AM -0400, Chas Williams wrote: > On Tue, Jun 26, 2018 at 6:32 AM Ido Schimmel wrote: > > > On Mon, Jun 25, 2018 at 02:45:24PM -0600, David Ahern wrote: > > > On 6/25/18 4:30 AM, Chas Williams wrote: > > > > vlan_changelink silently ignores attempts to change the vlan id > > > > or protocol id of an existing vlan interface. Implement by adding > > > > the new vlan id and protocol to the interface's vlan group and then > > > > removing the old vlan id and protocol from the vlan group. > > > > > > > > Signed-off-by: Chas Williams <3chas3@gmail.com> > > > > --- > > > > include/linux/netdevice.h | 1 + > > > > net/8021q/vlan.c | 4 ++-- > > > > net/8021q/vlan.h | 2 ++ > > > > net/8021q/vlan_netlink.c | 38 ++++++++++++++++++++++++++++++++++++++ > > > > net/core/dev.c | 1 + > > > > 5 files changed, 44 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > > > index 3ec9850c7936..a95ae238addf 100644 > > > > --- a/include/linux/netdevice.h > > > > +++ b/include/linux/netdevice.h > > > > @@ -2409,6 +2409,7 @@ enum netdev_cmd { > > > > NETDEV_CVLAN_FILTER_DROP_INFO, > > > > NETDEV_SVLAN_FILTER_PUSH_INFO, > > > > NETDEV_SVLAN_FILTER_DROP_INFO, > > > > + NETDEV_CHANGEVLAN, > > > > }; > > > > const char *netdev_cmd_to_name(enum netdev_cmd cmd); > > > > > > > > > > you add the new notifier, but do not add any hooks to catch and process > > it. > > > > > > Personally, I think it is a bit sketchy to change the vlan id on an > > > existing device and I suspect it will cause latent errors. > > > > +1 > > > > > > > > What's your use case for trying to implement the change versus causing > > > it to generate an unsupported error? > > > > > > If this patch does get accepted, I believe the mlxsw switchdev driver > > > will be impacted. > > > > Yes, at minimum we need to return an error for NETDEV_CHANGEVLAN, but > > looking at the code it seems that there's no proper rollback. > > > > I would prefer not to bother with error handling on the notification. If > something misses the notification, something misses the notification. > It happens. The notification is used so that relevant users in the kernel can potentially veto the operation and refuse it. See other notifications such as NETDEV_PRECHANGEUPPER. The driver David mentioned is one existing user that needs to refuse the VLAN change as it can't support it.