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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 07 Mar 2011 18:30:54 -0800
From:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
To:	Jesse Gross <jesse@...ira.com>
Cc:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"bphilips@...ell.com" <bphilips@...ell.com>,
	"Pieper, Jeffrey E" <jeffrey.e.pieper@...el.com>,
	Ben Hutchings <bhutchings@...arflare.com>
Subject: Re: [net-next 08/12] ixgb: convert to new VLAN model

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@...el.com> wrote:
> >>-----Original Message-----
> >>From: Jesse Gross [mailto:jesse@...ira.com]
> >>Sent: Tuesday, January 25, 2011 9:23 AM
> >>To: Tantilov, Emil S
> >>Cc: Kirsher, Jeffrey T; davem@...emloft.net; netdev@...r.kernel.org;
> >>bphilips@...ell.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@...el.com> wrote:
> >>> Jesse Gross wrote:
> >>>> On Thu, Jan 6, 2011 at 7:29 PM,  <jeffrey.t.kirsher@...el.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

Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)