Re: [net-next 08/12] ixgb: convert to new VLAN model

[Jeff Kirsher <jeffrey.t.kirsher@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3131 bytes --]

On Mon, 2011-03-07 at 17:31 -0800, Jesse Gross wrote:
> On Tue, Jan 25, 2011 at 10:20 AM, Tantilov, Emil S
> <emil.s.tantilov@intel.com> 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
> >><emil.s.tantilov@intel.com> wrote:
> >>> Jesse Gross wrote:
> >>>> On Thu, Jan 6, 2011 at 7:29 PM,  <jeffrey.t.kirsher@intel.com> wrote:
> >>>>> +static int ixgb_set_flags(struct net_device *netdev, u32 data) +{
> >>>>> +       struct ixgb_adapter *adapter = netdev_priv(netdev); +
> >>>>> bool need_reset; +       int rc;
> >>>>> +
> >>>>> +       /*
> >>>>> +        * TX vlan insertion does not work per HW design when Rx
> >>>>> stripping is +        * disabled.  Disable txvlan when rxvlan is
> >>>>> off. +        */ +       if ((data & ETH_FLAG_RXVLAN) !=
> >>>>> (netdev->features & NETIF_F_HW_VLAN_RX)) +               data ^=
> >>>>> ETH_FLAG_TXVLAN;
> >>>>
> >>>> Does this really do the right thing?  If 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))
> >>>>         data &= ~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 following?
> >>>
> >>>        if (!(data & ETH_FLAG_RXVLAN) &&
> >>>           (netdev->features & NETIF_F_HW_VLAN_TX))
> >>>                data &= ~ETH_FLAG_TXVLAN;
> >>>        else if (data & ETH_FLAG_TXVLAN)
> >>>                data |= 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
> >
> > That is the reason I asked, as I don't want to keep bouncing the patch back and forth. Personally I like the idea of helping the user and adjusting the flags to something that works rather than a generic error message.
> 
> 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.

I have a patch which has passed testing from Emil, I just need to check
with Emil that the current patch I have ready to push is the "latest"
version and does not need any more tweaks.

Cheers,
Jeff

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]