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: <AANLkTi=RU11ibzd3c9sqCLL0pNowvx1_ow7C=qWVoPMt@mail.gmail.com>
Date:	Wed, 26 Jan 2011 19:53:13 -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.

I think it is fine to adjust things, especially where the restrictions
are hardware specific and the user is less likely to know what
settings are related.  As long as it works, it doesn't matter too much
to me either way, so please do what you think is the most appropriate.

>
>>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.
>
> Sure, but I think a savvy user would always check the result of an ethtool command (ie. `ethtool -K` followed with `ethtool -k`, -A/-a, etc).

Probably, but it seems the less savviness required from the user the
better.  Regardless, it doesn't affect anything here, it would just be
a change to the userspace tool.
--
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