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.kirs...@intel.com> >> Date: Tue, 26 Apr 2016 13:55:41 -0700 >> >>> From: Kiran Patil <kiran.pa...@intel.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.pa...@intel.com> >>> Tested-by: Andrew Bowers <andrewx.bow...@intel.com> >>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.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
