| Message ID | 20241028165922.7188-1-mateusz.polchlopek@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [Intel-wired-lan,iwl-net] ice: change q_index variable type to s16 to store -1 value | expand |
On Mon, Oct 28, 2024 at 12:59:22PM -0400, Mateusz Polchlopek wrote: > Fix Flow Director not allowing to re-map traffic to 0th queue when action > is configured to drop (and vice versa). > > The current implementation of ethtool callback in the ice driver forbids > change Flow Director action from 0 to -1 and from -1 to 0 with an error, > e.g: > > # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action 0 > # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action -1 > rmgr: Cannot insert RX class rule: Invalid argument > > We set the value of `u16 q_index = 0` at the beginning of the function > ice_set_fdir_input_set(). In case of "drop traffic" action (which is > equal to -1 in ethtool) we store the 0 value. Later, when want to change > traffic rule to redirect to queue with index 0 it returns an error > caused by duplicate found. > > Fix this behaviour by change of the type of field `q_index` from u16 to s16 > in `struct ice_fdir_fltr`. This allows to store -1 in the field in case > of "drop traffic" action. What is more, change the variable type in the > function ice_set_fdir_input_set() and assign at the beginning the new > `#define ICE_FDIR_NO_QUEUE_IDX` which is -1. Later, if the action is set > to another value (point specific queue index) the variable value is > overwritten in the function. > > Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> This looks good, although I am interested to know what the maximum value for q_index is. And, considering unsigned values are used elsewhere, if using 0xffff within this driver was considered instead of -1. That notwithstanding, Reviewed-by: Simon Horman <horms@kernel.org>
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Mateusz Polchlopek > Sent: 28 October 2024 22:29 > To: intel-wired-lan@lists.osuosl.org > Cc: Ertman, David M <david.m.ertman@intel.com>; netdev@vger.kernel.org; Polchlopek, Mateusz <mateusz.polchlopek@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> > Subject: [Intel-wired-lan] [PATCH iwl-net] ice: change q_index variable type to s16 to store -1 value > > Fix Flow Director not allowing to re-map traffic to 0th queue when action is configured to drop (and vice versa). > > The current implementation of ethtool callback in the ice driver forbids change Flow Director action from 0 to -1 and from -1 to 0 with an error, > e.g: > > # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action 0 # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action -1 > rmgr: Cannot insert RX class rule: Invalid argument > > We set the value of `u16 q_index = 0` at the beginning of the function ice_set_fdir_input_set(). In case of "drop traffic" action (which is equal to -1 in ethtool) we store the 0 value. Later, when want to change traffic rule to redirect to queue with index 0 it returns an error caused by duplicate found. > > Fix this behaviour by change of the type of field `q_index` from u16 to s16 in `struct ice_fdir_fltr`. This allows to store -1 in the field in case of "drop traffic" action. What is more, change the variable type > in the function ice_set_fdir_input_set() and assign at the beginning the new `#define ICE_FDIR_NO_QUEUE_IDX` which is -1. Later, if the action is set to another value (point specific queue index) the variable value is overwritten in the function. > > Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c | 3 ++- > drivers/net/ethernet/intel/ice/ice_fdir.h | 4 +++- > 2 files changed, 5 insertions(+), 2 deletions(-) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
On 11/2/2024 3:38 PM, Simon Horman wrote: > On Mon, Oct 28, 2024 at 12:59:22PM -0400, Mateusz Polchlopek wrote: >> Fix Flow Director not allowing to re-map traffic to 0th queue when action >> is configured to drop (and vice versa). >> >> The current implementation of ethtool callback in the ice driver forbids >> change Flow Director action from 0 to -1 and from -1 to 0 with an error, >> e.g: >> >> # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action 0 >> # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action -1 >> rmgr: Cannot insert RX class rule: Invalid argument >> >> We set the value of `u16 q_index = 0` at the beginning of the function >> ice_set_fdir_input_set(). In case of "drop traffic" action (which is >> equal to -1 in ethtool) we store the 0 value. Later, when want to change >> traffic rule to redirect to queue with index 0 it returns an error >> caused by duplicate found. >> >> Fix this behaviour by change of the type of field `q_index` from u16 to s16 >> in `struct ice_fdir_fltr`. This allows to store -1 in the field in case >> of "drop traffic" action. What is more, change the variable type in the >> function ice_set_fdir_input_set() and assign at the beginning the new >> `#define ICE_FDIR_NO_QUEUE_IDX` which is -1. Later, if the action is set >> to another value (point specific queue index) the variable value is >> overwritten in the function. >> >> Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") >> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > > This looks good, although I am interested to know what the maximum value > for q_index is. And, considering unsigned values are used elsewhere, if > using 0xffff within this driver was considered instead of -1. > > That notwithstanding, > > Reviewed-by: Simon Horman <horms@kernel.org> Hi Simon! Thanks for Your review. What is about q_index: it stores queue index which can be theoretically up to few thousands. So in this case s16 should be enough and will be able to hold all indexes. I didn't consider 0xffff as this may be misleading, I decided to stay with -1. Mateusz
On Mon, Nov 04, 2024 at 01:56:20PM +0100, Mateusz Polchlopek wrote: > > > On 11/2/2024 3:38 PM, Simon Horman wrote: > > On Mon, Oct 28, 2024 at 12:59:22PM -0400, Mateusz Polchlopek wrote: > > > Fix Flow Director not allowing to re-map traffic to 0th queue when action > > > is configured to drop (and vice versa). > > > > > > The current implementation of ethtool callback in the ice driver forbids > > > change Flow Director action from 0 to -1 and from -1 to 0 with an error, > > > e.g: > > > > > > # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action 0 > > > # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action -1 > > > rmgr: Cannot insert RX class rule: Invalid argument > > > > > > We set the value of `u16 q_index = 0` at the beginning of the function > > > ice_set_fdir_input_set(). In case of "drop traffic" action (which is > > > equal to -1 in ethtool) we store the 0 value. Later, when want to change > > > traffic rule to redirect to queue with index 0 it returns an error > > > caused by duplicate found. > > > > > > Fix this behaviour by change of the type of field `q_index` from u16 to s16 > > > in `struct ice_fdir_fltr`. This allows to store -1 in the field in case > > > of "drop traffic" action. What is more, change the variable type in the > > > function ice_set_fdir_input_set() and assign at the beginning the new > > > `#define ICE_FDIR_NO_QUEUE_IDX` which is -1. Later, if the action is set > > > to another value (point specific queue index) the variable value is > > > overwritten in the function. > > > > > > Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") > > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > > Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > > > > This looks good, although I am interested to know what the maximum value > > for q_index is. And, considering unsigned values are used elsewhere, if > > using 0xffff within this driver was considered instead of -1. > > > > That notwithstanding, > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > Hi Simon! > > Thanks for Your review. > What is about q_index: it stores queue index which can be theoretically > up to few thousands. So in this case s16 should be enough and will be > able to hold all indexes. I didn't consider 0xffff as this may be > misleading, I decided to stay with -1. Thanks. I agree that if we are expecting the maximum (positive) value to be a few thousand, then the s16-based approach you have taken is a good one.
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c index 5412eff8ef23..ee9862ddfe15 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c @@ -1830,11 +1830,12 @@ static int ice_set_fdir_input_set(struct ice_vsi *vsi, struct ethtool_rx_flow_spec *fsp, struct ice_fdir_fltr *input) { - u16 dest_vsi, q_index = 0; + s16 q_index = ICE_FDIR_NO_QUEUE_IDX; u16 orig_q_index = 0; struct ice_pf *pf; struct ice_hw *hw; int flow_type; + u16 dest_vsi; u8 dest_ctl; if (!vsi || !fsp || !input) diff --git a/drivers/net/ethernet/intel/ice/ice_fdir.h b/drivers/net/ethernet/intel/ice/ice_fdir.h index ab5b118daa2d..820023c0271f 100644 --- a/drivers/net/ethernet/intel/ice/ice_fdir.h +++ b/drivers/net/ethernet/intel/ice/ice_fdir.h @@ -53,6 +53,8 @@ */ #define ICE_FDIR_IPV4_PKT_FLAG_MF 0x20 +#define ICE_FDIR_NO_QUEUE_IDX -1 + enum ice_fltr_prgm_desc_dest { ICE_FLTR_PRGM_DESC_DEST_DROP_PKT, ICE_FLTR_PRGM_DESC_DEST_DIRECT_PKT_QINDEX, @@ -186,7 +188,7 @@ struct ice_fdir_fltr { u16 flex_fltr; /* filter control */ - u16 q_index; + s16 q_index; u16 orig_q_index; u16 dest_vsi; u8 dest_ctl;
Fix Flow Director not allowing to re-map traffic to 0th queue when action is configured to drop (and vice versa). The current implementation of ethtool callback in the ice driver forbids change Flow Director action from 0 to -1 and from -1 to 0 with an error, e.g: # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action 0 # ethtool -U eth2 flow-type tcp4 src-ip 1.1.1.1 loc 1 action -1 rmgr: Cannot insert RX class rule: Invalid argument We set the value of `u16 q_index = 0` at the beginning of the function ice_set_fdir_input_set(). In case of "drop traffic" action (which is equal to -1 in ethtool) we store the 0 value. Later, when want to change traffic rule to redirect to queue with index 0 it returns an error caused by duplicate found. Fix this behaviour by change of the type of field `q_index` from u16 to s16 in `struct ice_fdir_fltr`. This allows to store -1 in the field in case of "drop traffic" action. What is more, change the variable type in the function ice_set_fdir_input_set() and assign at the beginning the new `#define ICE_FDIR_NO_QUEUE_IDX` which is -1. Later, if the action is set to another value (point specific queue index) the variable value is overwritten in the function. Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters") Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> --- drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c | 3 ++- drivers/net/ethernet/intel/ice/ice_fdir.h | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) base-commit: 93e5920e5193241cb05caaa6421365fd8800f1b4