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: Mon, 7 Mar 2011 17:31:10 -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" , Ben Hutchings To: "Tantilov, Emil S" Return-path: Received: from mail-vx0-f174.google.com ([209.85.220.174]:38901 "EHLO mail-vx0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750754Ab1CHBbL convert rfc822-to-8bit (ORCPT ); Mon, 7 Mar 2011 20:31:11 -0500 Received: by vxi39 with SMTP id 39so4300150vxi.19 for ; Mon, 07 Mar 2011 17:31:11 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Jan 25, 2011 at 10:20 AM, Tantilov, Emil S wrote: >>-----Original Message----- >>From: Jesse Gross [mailto:jesse@nicira.com] >>Sent: Tuesday, January 25, 2011 9:23 AM >>To: Tantilov, Emil S >>Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org; >>bphilips@novell.com; Pieper, Jeffrey E >>Subject: Re: [net-next 08/12] ixgb: convert to new VLAN model >> >>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 w= rote: >>>>> +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 = when Rx >>>>> stripping is + =A0 =A0 =A0 =A0* disabled. =A0Disable txvlan when = rxvlan is >>>>> off. + =A0 =A0 =A0 =A0*/ + =A0 =A0 =A0 if ((data & ETH_FLAG_RXVLA= N) !=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= one >>>> 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 disable= d, and >>the user attempts to enable txvlan? At least our validation argued th= at we >>should make it work both ways. Perhaps something like the following? >>> >>> =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. =A0Turning dependent features on when needed i= s a >>little bit inconsistent with the rest of Ethtool (for example, turnin= g >>on TSO when checksum offloading is off will not enable checksum >>offloading, it will produce an error). =A0However, I know that driver= s > > That is the reason I asked, as I don't want to keep bouncing the patc= h back and forth. Personally I like the idea of helping the user and ad= justing the flags to something that works rather than a generic error m= essage. Were you able to take a look at this? I think that we have something that is pretty close, so it would be nice to wrap it up.