Message-ID: <ZCwMs/FqlD3/ygF1@localhost.localdomain>
Date:   Tue, 4 Apr 2023 13:40:35 +0200
From:   Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To:     Alexander Lobakin <aleksander.lobakin@...el.com>
Cc:     intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        Simon Horman <simon.horman@...igine.com>
Subject: Re: [Intel-wired-lan] [PATCH net-next v2 2/4] ice: remove redundant
 Rx field from rule info

On Tue, Apr 04, 2023 at 12:07:47PM +0200, Alexander Lobakin wrote:
> From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> Date: Tue,  4 Apr 2023 09:28:31 +0200
> 
> > Information about the direction is currently stored in sw_act.flag.
> > There is no need to duplicate it in another field.
> > 
> > Setting direction flag doesn't mean that there is a match criteria for
> > direction in rule. It is only a information for HW from where switch id
> > should be collected (VSI or port). In current implementation of advance
> > rule handling, without matching for direction meta data, we can always
> > set one the same flag and everything will work the same.
> > 
> > Ability to match on direction matadata will be added in follow up
> > patches.
> > 
> > Recipe 0, 3 and 9 loaded from package has direction match
> > criteria, but they are handled in other function.
> > 
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> > Reviewed-by: Piotr Raczynski <piotr.raczynski@...el.com>
> > Reviewed-by: Simon Horman <simon.horman@...igine.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_eswitch.c |  1 -
> >  drivers/net/ethernet/intel/ice/ice_switch.c  | 22 ++++++++++----------
> >  drivers/net/ethernet/intel/ice/ice_switch.h  |  2 --
> >  drivers/net/ethernet/intel/ice/ice_tc_lib.c  |  5 -----
> >  4 files changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > index f6dd3f8fd936..2c80d57331d0 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > @@ -39,7 +39,6 @@ ice_eswitch_add_vf_mac_rule(struct ice_pf *pf, struct ice_vf *vf, const u8 *mac)
> >  	rule_info.sw_act.flag |= ICE_FLTR_TX;
> >  	rule_info.sw_act.vsi_handle = ctrl_vsi->idx;
> >  	rule_info.sw_act.fltr_act = ICE_FWD_TO_Q;
> > -	rule_info.rx = false;
> >  	rule_info.sw_act.fwd_id.q_id = hw->func_caps.common_cap.rxq_first_id +
> >  				       ctrl_vsi->rxq_map[vf->vf_id];
> >  	rule_info.flags_info.act |= ICE_SINGLE_ACT_LB_ENABLE;
> > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> > index 5c3f266fa80f..4d3a92e0c61f 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> > @@ -6121,8 +6121,7 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> >  	if (rinfo->sw_act.fltr_act == ICE_FWD_TO_VSI)
> >  		rinfo->sw_act.fwd_id.hw_vsi_id =
> >  			ice_get_hw_vsi_num(hw, vsi_handle);
> > -	if (rinfo->sw_act.flag & ICE_FLTR_TX)
> > -		rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
> > +	rinfo->sw_act.src = ice_get_hw_vsi_num(hw, vsi_handle);
> >  
> >  	status = ice_add_adv_recipe(hw, lkups, lkups_cnt, rinfo, &rid);
> >  	if (status)
> > @@ -6190,19 +6189,20 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> >  		goto err_ice_add_adv_rule;
> >  	}
> >  
> > -	/* set the rule LOOKUP type based on caller specified 'Rx'
> > -	 * instead of hardcoding it to be either LOOKUP_TX/RX
> > +	/* If there is no matching criteria for direction there
> > +	 * is only one difference between Rx and Tx:
> > +	 * - get switch id base on VSI number from source field (Tx)
> > +	 * - get switch id base on port number (Rx)
> >  	 *
> > -	 * for 'Rx' set the source to be the port number
> > -	 * for 'Tx' set the source to be the source HW VSI number (determined
> > -	 * by caller)
> > +	 * If matching on direction metadata is chose rule direction is
> > +	 * extracted from type value set here.
> >  	 */
> > -	if (rinfo->rx) {
> > -		s_rule->hdr.type = cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_RX);
> > -		s_rule->src = cpu_to_le16(hw->port_info->lport);
> > -	} else {
> > +	if (rinfo->sw_act.flag & ICE_FLTR_TX) {
> >  		s_rule->hdr.type = cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_TX);
> >  		s_rule->src = cpu_to_le16(rinfo->sw_act.src);
> > +	} else {
> > +		s_rule->hdr.type = cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_RX);
> > +		s_rule->src = cpu_to_le16(hw->port_info->lport);
> >  	}
> >  
> >  	s_rule->recipe_id = cpu_to_le16(rid);
> > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
> > index 68d8e8a6a189..44aa37b80111 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> > @@ -10,7 +10,6 @@
> >  #define ICE_DFLT_VSI_INVAL 0xff
> >  #define ICE_FLTR_RX BIT(0)
> >  #define ICE_FLTR_TX BIT(1)
> > -#define ICE_FLTR_TX_RX (ICE_FLTR_RX | ICE_FLTR_TX)
> >  #define ICE_VSI_INVAL_ID 0xffff
> >  #define ICE_INVAL_Q_HANDLE 0xFFFF
> >  
> > @@ -190,7 +189,6 @@ struct ice_adv_rule_info {
> >  	enum ice_sw_tunnel_type tun_type;
> >  	struct ice_sw_act_ctrl sw_act;
> >  	u32 priority;
> > -	u8 rx; /* true means LOOKUP_RX otherwise LOOKUP_TX */
> >  	u16 fltr_rule_id;
> >  	u16 vlan_type;
> >  	struct ice_adv_rule_flags_info flags_info;
> 
> That u8 here was really off, was introducing at least 1 byte hole. Good
> thing you dropped it.
> Have you checked whether there are any holes left, maybe move fields
> around a bit?
> 

As You pointed in patch 3 there are, I will move it to avoid holes.

> > diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > index 76f29a5bf8d7..b5af6cd5592b 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > @@ -697,11 +697,9 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> >  	if (fltr->direction == ICE_ESWITCH_FLTR_INGRESS) {
> >  		rule_info.sw_act.flag |= ICE_FLTR_RX;
> >  		rule_info.sw_act.src = hw->pf_id;
> > -		rule_info.rx = true;
> >  	} else {
> >  		rule_info.sw_act.flag |= ICE_FLTR_TX;
> >  		rule_info.sw_act.src = vsi->idx;
> > -		rule_info.rx = false;
> >  		rule_info.flags_info.act = ICE_SINGLE_ACT_LAN_ENABLE;
> >  		rule_info.flags_info.act_valid = true;
> >  	}
> > @@ -909,7 +907,6 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
> >  		rule_info.sw_act.vsi_handle = dest_vsi->idx;
> >  		rule_info.priority = ICE_SWITCH_FLTR_PRIO_VSI;
> >  		rule_info.sw_act.src = hw->pf_id;
> > -		rule_info.rx = true;
> >  		dev_dbg(dev, "add switch rule for TC:%u vsi_idx:%u, lkups_cnt:%u\n",
> >  			tc_fltr->action.fwd.tc.tc_class,
> >  			rule_info.sw_act.vsi_handle, lkups_cnt);
> > @@ -920,7 +917,6 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
> >  		rule_info.sw_act.vsi_handle = dest_vsi->idx;
> >  		rule_info.priority = ICE_SWITCH_FLTR_PRIO_QUEUE;
> >  		rule_info.sw_act.src = hw->pf_id;
> > -		rule_info.rx = true;
> >  		dev_dbg(dev, "add switch rule action to forward to queue:%u (HW queue %u), lkups_cnt:%u\n",
> >  			tc_fltr->action.fwd.q.queue,
> >  			tc_fltr->action.fwd.q.hw_queue, lkups_cnt);
> > @@ -928,7 +924,6 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
> >  	case ICE_DROP_PACKET:
> >  		rule_info.sw_act.flag |= ICE_FLTR_RX;
> >  		rule_info.sw_act.src = hw->pf_id;
> > -		rule_info.rx = true;
> >  		rule_info.priority = ICE_SWITCH_FLTR_PRIO_VSI;
> >  		break;
> >  	default:
> 
> Thanks,
> Olek

Powered by blists - more mailing lists