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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <AANLkTikkMTv4uCbpOFfdgHzt2jMa5u5ipCOQ3OOh3KLs@mail.gmail.com>
Date:	Mon, 7 Mar 2011 17:31:10 -0800
From:	Jesse Gross <jesse@...ira.com>
To:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
Cc:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...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 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html