netdev - Re: [net-next 08/15] i40e: Allow user to change input set mask for flow director

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]

Date:	Wed, 4 May 2016 20:22:30 -0700
From:	John Fastabend <john.fastabend@...il.com>
To:	"Patil, Kiran" <kiran.patil@...el.com>,
	David Miller <davem@...emloft.net>, jeffrey.t.kirsher@...el.com
Cc:	netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
	jogreene@...hat.com
Subject: Re: [net-next 08/15] i40e: Allow user to change input set mask for
 flow director

On 16-05-04 07:47 PM, Patil, Kiran wrote:
> On 4/26/2016 8:48 PM, David Miller wrote:
>> From: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>> Date: Tue, 26 Apr 2016 13:55:41 -0700
>>
>>> From: Kiran Patil <kiran.patil@...el.com>
>>>
>>> This patch implements feature, which allows user to change
>>> input set mask for flow director using side-band channel.
>>> This patch adds definition of FLOW_TYPE_MASK into the header file.
>>> With this patch, user can now specify less than 4 tuple(src ip, dsp ip,
>>> src port, dst port) for flow type TCP4/UDP4.
>>>
>>> Change-Id: I90052508f8c172c0da5a4b78b949704b4a59ea78
>>> Signed-off-by: Kiran Patil <kiran.patil@...el.com>
>>> Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>>
>> If you want to do this, you have to define the semantics generically
>> in include/uapi/linux/ethtool.h so that other drivers can implement
>> this too.
>>
>> Please don't ever implement private, driver specific, interpretations
>> of the generic ethtool facilitites.
>>
>> The semantics and interpretations of the values must absolutely be
>> consistent across all drivers in the tree.
>>
>> Otherwise the user experience is terrible.
>>
>> Thanks.
>>
> This is not new feature implemented in i40e driver. This is the original
> feature of ethtool which allows user to specify subset of tuple for
> setting up flow director.
> 
> i40e driver using it same way as ixgbe.
> 
> Please let us know if I misinterpreted your response.
> 
> Would you recommend that we re-submit patch with better patch
> description (indicating that it is not new feature but just enabling
> feature).
> 
> Thanks,
> -- Kiran P.

At least define FLOW_TYPE_MASK in ethtool.h then its not
sort of hobbled together in the driver and others can use it
instead of the normal ~FLOW_EXT which I see other drivers used.
Another benefit if its near the definition of the flow types
you have a chance of someone seeing it if they add a flag
past 0xff.

And maybe do it as a separate patch. So you aren't adding normal
driver ethtool implementation and a new #define for all drivers
in the same patch.

.John