From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Gross Subject: Re: [net-next 08/12] ixgb: convert to new VLAN model Date: Tue, 25 Jan 2011 09:22:50 -0800 Message-ID: References: <1294360199-9860-1-git-send-email-jeffrey.t.kirsher@intel.com> <1294360199-9860-9-git-send-email-jeffrey.t.kirsher@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "Kirsher, Jeffrey T" , "davem@davemloft.net" , "netdev@vger.kernel.org" , "bphilips@novell.com" , "Pieper, Jeffrey E" To: "Tantilov, Emil S" Return-path: Received: from mail-iw0-f174.google.com ([209.85.214.174]:44097 "EHLO mail-iw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753726Ab1AYRWv convert rfc822-to-8bit (ORCPT ); Tue, 25 Jan 2011 12:22:51 -0500 Received: by iwn9 with SMTP id 9so8686iwn.19 for ; Tue, 25 Jan 2011 09:22:50 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Jan 23, 2011 at 4:25 PM, Tantilov, Emil S wrote: > Jesse Gross wrote: >> On Thu, Jan 6, 2011 at 7:29 PM, =A0 wro= te: >>> +static int ixgb_set_flags(struct net_device *netdev, u32 data) +{ >>> + =A0 =A0 =A0 struct ixgb_adapter *adapter =3D netdev_priv(netdev);= + >>> bool need_reset; + =A0 =A0 =A0 int rc; >>> + >>> + =A0 =A0 =A0 /* >>> + =A0 =A0 =A0 =A0* TX vlan insertion does not work per HW design wh= en Rx >>> stripping is + =A0 =A0 =A0 =A0* disabled. =A0Disable txvlan when rx= vlan is >>> off. + =A0 =A0 =A0 =A0*/ + =A0 =A0 =A0 if ((data & ETH_FLAG_RXVLAN)= !=3D >>> (netdev->features & NETIF_F_HW_VLAN_RX)) + =A0 =A0 =A0 =A0 =A0 =A0 = =A0 data ^=3D >>> ETH_FLAG_TXVLAN; >> >> Does this really do the right thing? =A0If the RX vlan setting is >> changed, it will do the opposite of what the user requested for TX >> vlan? >> >> So if I start with both on (the default) and turn them both off in o= ne >> command (a valid setting), I will get RX off and TX on (an invalid >> setting). >> >> Why not: >> >> if (!(data & ETH_FLAG_RXVLAN)) >> =A0 =A0 =A0 =A0 data &=3D ~ETH_FLAG_TXVLAN; > > Yeah that works for disabling rxvlan, but what if rxvlan is disabled,= and the user attempts to enable txvlan? At least our validation argued= that we should make it work both ways. Perhaps something like the foll= owing? > > =A0 =A0 =A0 =A0if (!(data & ETH_FLAG_RXVLAN) && > =A0 =A0 =A0 =A0 =A0 (netdev->features & NETIF_F_HW_VLAN_TX)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0data &=3D ~ETH_FLAG_TXVLAN; > =A0 =A0 =A0 =A0else if (data & ETH_FLAG_TXVLAN) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0data |=3D ETH_FLAG_RXVLAN; I think the logic above does what you describe and will always result in a consistent state. Turning dependent features on when needed is a little bit inconsistent with the rest of Ethtool (for example, turning on TSO when checksum offloading is off will not enable checksum offloading, it will produce an error). However, I know that drivers aren't completely consistent here and the most important part is that it enforces valid states, so I don't have a strong opinion. Ben's previous suggestion of Ethtool querying again after the operation and reporting any flags that were automatically changed would help a lot here.