Re: [dpdk-dev] [PATCH 39/49] net/ice/base: slightly code update

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 6/4/19 7:42 AM, Leyi Rong wrote:
> Mainly update below functions:
> 
> ice_flow_proc_seg_hdrs
> ice_flow_find_prof_conds
> ice_dealloc_flow_entry
> ice_add_rule_internal


It seems that some of the changes are bug fixes.
So IMO, these changes should be in dedicated patches, with Fixes tag
in the commit message.

Overall, these changes should be split by kind of changes. There are
functions reworks, minor cleanups, robustness changes, ...

> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> ---
>   drivers/net/ice/base/ice_flex_pipe.c     | 13 +++----
>   drivers/net/ice/base/ice_flow.c          | 47 +++++++++++++++++-------
>   drivers/net/ice/base/ice_nvm.c           |  4 +-
>   drivers/net/ice/base/ice_protocol_type.h |  1 +
>   drivers/net/ice/base/ice_switch.c        | 24 +++++++-----
>   drivers/net/ice/base/ice_switch.h        | 14 +++----
>   drivers/net/ice/base/ice_type.h          | 13 ++++++-
>   7 files changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ice/base/ice_flex_pipe.c b/drivers/net/ice/base/ice_flex_pipe.c
> index 5864cbf3e..2a310b6e1 100644
> --- a/drivers/net/ice/base/ice_flex_pipe.c
> +++ b/drivers/net/ice/base/ice_flex_pipe.c
> @@ -134,7 +134,7 @@ static struct ice_buf_table *ice_find_buf_table(struct ice_seg *ice_seg)
>   	nvms = (struct ice_nvm_table *)(ice_seg->device_table +
>   		LE32_TO_CPU(ice_seg->device_table_count));
>   
> -	return (struct ice_buf_table *)
> +	return (_FORCE_ struct ice_buf_table *)
>   		(nvms->vers + LE32_TO_CPU(nvms->table_count));
>   }
>   
> @@ -1005,9 +1005,8 @@ ice_dwnld_cfg_bufs(struct ice_hw *hw, struct ice_buf *bufs, u32 count)
>   
>   		bh = (struct ice_buf_hdr *)(bufs + i);
>   
> -		status = ice_aq_download_pkg(hw, bh, LE16_TO_CPU(bh->data_end),
> -					     last, &offset, &info, NULL);
> -
> +		status = ice_aq_download_pkg(hw, bh, ICE_PKG_BUF_SIZE, last,
> +					     &offset, &info, NULL);
>   		if (status) {
>   			ice_debug(hw, ICE_DBG_PKG,
>   				  "Pkg download failed: err %d off %d inf %d\n",
> @@ -2937,7 +2936,7 @@ static void ice_fill_tbl(struct ice_hw *hw, enum ice_block block_id, u32 sid)
>   		case ICE_SID_XLT2_ACL:
>   		case ICE_SID_XLT2_PE:
>   			xlt2 = (struct ice_xlt2_section *)sect;
> -			src = (u8 *)xlt2->value;
> +			src = (_FORCE_ u8 *)xlt2->value;
>   			sect_len = LE16_TO_CPU(xlt2->count) *
>   				sizeof(*hw->blk[block_id].xlt2.t);
>   			dst = (u8 *)hw->blk[block_id].xlt2.t;
> @@ -3889,7 +3888,7 @@ ice_update_fd_swap(struct ice_hw *hw, u16 prof_id, struct ice_fv_word *es)
>   
>   	/* fill in the swap array */
>   	si = hw->blk[ICE_BLK_FD].es.fvw - 1;
> -	do {
> +	while (si >= 0) {
>   		u8 indexes_used = 1;
>   
>   		/* assume flat at this index */
> @@ -3921,7 +3920,7 @@ ice_update_fd_swap(struct ice_hw *hw, u16 prof_id, struct ice_fv_word *es)
>   		}
>   
>   		si -= indexes_used;
> -	} while (si >= 0);
> +	}
>   
>   	/* for each set of 4 swap indexes, write the appropriate register */
>   	for (j = 0; j < hw->blk[ICE_BLK_FD].es.fvw / 4; j++) {
> diff --git a/drivers/net/ice/base/ice_flow.c b/drivers/net/ice/base/ice_flow.c
> index 795abe98f..f31557eac 100644
> --- a/drivers/net/ice/base/ice_flow.c
> +++ b/drivers/net/ice/base/ice_flow.c
> @@ -415,9 +415,6 @@ ice_flow_proc_seg_hdrs(struct ice_flow_prof_params *params)
>   		const ice_bitmap_t *src;
>   		u32 hdrs;
>   
> -		if (i > 0 && (i + 1) < prof->segs_cnt)
> -			continue;
> -
>   		hdrs = prof->segs[i].hdrs;
>   
>   		if (hdrs & ICE_FLOW_SEG_HDR_ETH) {
> @@ -847,6 +844,7 @@ ice_flow_proc_segs(struct ice_hw *hw, struct ice_flow_prof_params *params)
>   
>   #define ICE_FLOW_FIND_PROF_CHK_FLDS	0x00000001
>   #define ICE_FLOW_FIND_PROF_CHK_VSI	0x00000002
> +#define ICE_FLOW_FIND_PROF_NOT_CHK_DIR	0x00000004
>   
>   /**
>    * ice_flow_find_prof_conds - Find a profile matching headers and conditions
> @@ -866,7 +864,8 @@ ice_flow_find_prof_conds(struct ice_hw *hw, enum ice_block blk,
>   	struct ice_flow_prof *p;
>   
>   	LIST_FOR_EACH_ENTRY(p, &hw->fl_profs[blk], ice_flow_prof, l_entry) {
> -		if (p->dir == dir && segs_cnt && segs_cnt == p->segs_cnt) {
> +		if ((p->dir == dir || conds & ICE_FLOW_FIND_PROF_NOT_CHK_DIR) &&
> +		    segs_cnt && segs_cnt == p->segs_cnt) {
>   			u8 i;
>   
>   			/* Check for profile-VSI association if specified */
> @@ -935,17 +934,15 @@ ice_flow_find_prof_id(struct ice_hw *hw, enum ice_block blk, u64 prof_id)
>   }
>   
>   /**
> - * ice_flow_rem_entry_sync - Remove a flow entry
> + * ice_dealloc_flow_entry - Deallocate flow entry memory
>    * @hw: pointer to the HW struct
>    * @entry: flow entry to be removed
>    */
> -static enum ice_status
> -ice_flow_rem_entry_sync(struct ice_hw *hw, struct ice_flow_entry *entry)
> +static void
> +ice_dealloc_flow_entry(struct ice_hw *hw, struct ice_flow_entry *entry)
>   {
>   	if (!entry)
> -		return ICE_ERR_BAD_PTR;
> -
> -	LIST_DEL(&entry->l_entry);
> +		return;
>   
>   	if (entry->entry)
>   		ice_free(hw, entry->entry);
> @@ -957,6 +954,22 @@ ice_flow_rem_entry_sync(struct ice_hw *hw, struct ice_flow_entry *entry)
>   	}
>   
>   	ice_free(hw, entry);
> +}
> +
> +/**
> + * ice_flow_rem_entry_sync - Remove a flow entry
> + * @hw: pointer to the HW struct
> + * @entry: flow entry to be removed
> + */
> +static enum ice_status
> +ice_flow_rem_entry_sync(struct ice_hw *hw, struct ice_flow_entry *entry)
> +{
> +	if (!entry)
> +		return ICE_ERR_BAD_PTR;
> +
> +	LIST_DEL(&entry->l_entry);
> +
> +	ice_dealloc_flow_entry(hw, entry);
>   
>   	return ICE_SUCCESS;
>   }
> @@ -1395,9 +1408,12 @@ ice_flow_add_entry(struct ice_hw *hw, enum ice_block blk, u64 prof_id,
>   		goto out;
>   	}
>   
> -	ice_acquire_lock(&prof->entries_lock);
> -	LIST_ADD(&e->l_entry, &prof->entries);
> -	ice_release_lock(&prof->entries_lock);
> +	if (blk != ICE_BLK_ACL) {
> +		/* ACL will handle the entry management */
> +		ice_acquire_lock(&prof->entries_lock);
> +		LIST_ADD(&e->l_entry, &prof->entries);
> +		ice_release_lock(&prof->entries_lock);
> +	}
>   
>   	*entry_h = ICE_FLOW_ENTRY_HNDL(e);
>   
> @@ -1425,7 +1441,7 @@ enum ice_status ice_flow_rem_entry(struct ice_hw *hw, u64 entry_h)
>   	if (entry_h == ICE_FLOW_ENTRY_HANDLE_INVAL)
>   		return ICE_ERR_PARAM;
>   
> -	entry = ICE_FLOW_ENTRY_PTR((unsigned long)entry_h);
> +	entry = ICE_FLOW_ENTRY_PTR(entry_h);
>   
>   	/* Retain the pointer to the flow profile as the entry will be freed */
>   	prof = entry->prof;
> @@ -1676,6 +1692,9 @@ enum ice_status ice_rem_vsi_rss_cfg(struct ice_hw *hw, u16 vsi_handle)
>   	if (!ice_is_vsi_valid(hw, vsi_handle))
>   		return ICE_ERR_PARAM;
>   
> +	if (LIST_EMPTY(&hw->fl_profs[blk]))
> +		return ICE_SUCCESS;
> +

It should be useless as LIST_FOR_EACH_ENTRY_SAFE handles that case
properly IIUC.

>   	ice_acquire_lock(&hw->fl_profs_locks[blk]);
>   	LIST_FOR_EACH_ENTRY_SAFE(p, t, &hw->fl_profs[blk], ice_flow_prof,
>   				 l_entry) {
> diff --git a/drivers/net/ice/base/ice_nvm.c b/drivers/net/ice/base/ice_nvm.c
> index fa9c348ce..76cfedb29 100644
> --- a/drivers/net/ice/base/ice_nvm.c
> +++ b/drivers/net/ice/base/ice_nvm.c
> @@ -127,7 +127,7 @@ ice_read_sr_word_aq(struct ice_hw *hw, u16 offset, u16 *data)
>   
>   	status = ice_read_sr_aq(hw, offset, 1, data, true);
>   	if (!status)
> -		*data = LE16_TO_CPU(*(__le16 *)data);
> +		*data = LE16_TO_CPU(*(_FORCE_ __le16 *)data);
>   
>   	return status;
>   }
> @@ -185,7 +185,7 @@ ice_read_sr_buf_aq(struct ice_hw *hw, u16 offset, u16 *words, u16 *data)
>   	} while (words_read < *words);
>   
>   	for (i = 0; i < *words; i++)
> -		data[i] = LE16_TO_CPU(((__le16 *)data)[i]);
> +		data[i] = LE16_TO_CPU(((_FORCE_ __le16 *)data)[i]);
>   
>   read_nvm_buf_aq_exit:
>   	*words = words_read;
> diff --git a/drivers/net/ice/base/ice_protocol_type.h b/drivers/net/ice/base/ice_protocol_type.h
> index e572dd320..82822fb74 100644
> --- a/drivers/net/ice/base/ice_protocol_type.h
> +++ b/drivers/net/ice/base/ice_protocol_type.h
> @@ -189,6 +189,7 @@ struct ice_udp_tnl_hdr {
>   	u16 field;
>   	u16 proto_type;
>   	u16 vni;
> +	u16 reserved;
>   };
>   
>   struct ice_nvgre {
> diff --git a/drivers/net/ice/base/ice_switch.c b/drivers/net/ice/base/ice_switch.c
> index faaedd4c8..373acb7a6 100644
> --- a/drivers/net/ice/base/ice_switch.c
> +++ b/drivers/net/ice/base/ice_switch.c
> @@ -279,6 +279,7 @@ enum ice_status ice_init_def_sw_recp(struct ice_hw *hw)
>   		recps[i].root_rid = i;
>   		INIT_LIST_HEAD(&recps[i].filt_rules);
>   		INIT_LIST_HEAD(&recps[i].filt_replay_rules);
> +		INIT_LIST_HEAD(&recps[i].rg_list);

That looks like a bug fix, isn't it?

>   		ice_init_lock(&recps[i].filt_rule_lock);
>   	}
>   
> @@ -859,7 +860,7 @@ ice_aq_add_update_mir_rule(struct ice_hw *hw, u16 rule_type, u16 dest_vsi,
>   			return ICE_ERR_PARAM;
>   
>   		buf_size = count * sizeof(__le16);
> -		mr_list = (__le16 *)ice_malloc(hw, buf_size);
> +		mr_list = (_FORCE_ __le16 *)ice_malloc(hw, buf_size);
>   		if (!mr_list)
>   			return ICE_ERR_NO_MEMORY;
>   		break;
> @@ -1459,7 +1460,6 @@ static int ice_ilog2(u64 n)
>   	return -1;
>   }
>   
> -
>   /**
>    * ice_fill_sw_rule - Helper function to fill switch rule structure
>    * @hw: pointer to the hardware structure
> @@ -1479,7 +1479,6 @@ ice_fill_sw_rule(struct ice_hw *hw, struct ice_fltr_info *f_info,
>   	__be16 *off;
>   	u8 q_rgn;
>   
> -
>   	if (opc == ice_aqc_opc_remove_sw_rules) {
>   		s_rule->pdata.lkup_tx_rx.act = 0;
>   		s_rule->pdata.lkup_tx_rx.index =
> @@ -1555,7 +1554,7 @@ ice_fill_sw_rule(struct ice_hw *hw, struct ice_fltr_info *f_info,
>   		daddr = f_info->l_data.ethertype_mac.mac_addr;
>   		/* fall-through */
>   	case ICE_SW_LKUP_ETHERTYPE:
> -		off = (__be16 *)(eth_hdr + ICE_ETH_ETHTYPE_OFFSET);
> +		off = (_FORCE_ __be16 *)(eth_hdr + ICE_ETH_ETHTYPE_OFFSET);
>   		*off = CPU_TO_BE16(f_info->l_data.ethertype_mac.ethertype);
>   		break;
>   	case ICE_SW_LKUP_MAC_VLAN:
> @@ -1586,7 +1585,7 @@ ice_fill_sw_rule(struct ice_hw *hw, struct ice_fltr_info *f_info,
>   			   ICE_NONDMA_TO_NONDMA);
>   
>   	if (!(vlan_id > ICE_MAX_VLAN_ID)) {
> -		off = (__be16 *)(eth_hdr + ICE_ETH_VLAN_TCI_OFFSET);
> +		off = (_FORCE_ __be16 *)(eth_hdr + ICE_ETH_VLAN_TCI_OFFSET);
>   		*off = CPU_TO_BE16(vlan_id);
>   	}
>   
> @@ -2289,14 +2288,15 @@ ice_add_rule_internal(struct ice_hw *hw, u8 recp_id,
>   
>   	m_entry = ice_find_rule_entry(hw, recp_id, new_fltr);
>   	if (!m_entry) {
> -		ice_release_lock(rule_lock);
> -		return ice_create_pkt_fwd_rule(hw, f_entry);
> +		status = ice_create_pkt_fwd_rule(hw, f_entry);
> +		goto exit_add_rule_internal;
>   	}
>   
>   	cur_fltr = &m_entry->fltr_info;
>   	status = ice_add_update_vsi_list(hw, m_entry, cur_fltr, new_fltr);
> -	ice_release_lock(rule_lock);
>   
> +exit_add_rule_internal:
> +	ice_release_lock(rule_lock);
>   	return status;
>   }
>   
> @@ -2975,12 +2975,19 @@ ice_add_mac_vlan(struct ice_hw *hw, struct LIST_HEAD_TYPE *mv_list)
>    * ice_add_eth_mac - Add ethertype and MAC based filter rule
>    * @hw: pointer to the hardware structure
>    * @em_list: list of ether type MAC filter, MAC is optional
> + *
> + * This function requires the caller to populate the entries in
> + * the filter list with the necessary fields (including flags to
> + * indicate Tx or Rx rules).
>    */
>   enum ice_status
>   ice_add_eth_mac(struct ice_hw *hw, struct LIST_HEAD_TYPE *em_list)
>   {
>   	struct ice_fltr_list_entry *em_list_itr;
>   
> +	if (!em_list || !hw)
> +		return ICE_ERR_PARAM;
> +
>   	LIST_FOR_EACH_ENTRY(em_list_itr, em_list, ice_fltr_list_entry,
>   			    list_entry) {
>   		enum ice_sw_lkup_type l_type =
> @@ -2990,7 +2997,6 @@ ice_add_eth_mac(struct ice_hw *hw, struct LIST_HEAD_TYPE *em_list)
>   		    l_type != ICE_SW_LKUP_ETHERTYPE)
>   			return ICE_ERR_PARAM;
>   
> -		em_list_itr->fltr_info.flag = ICE_FLTR_TX;
>   		em_list_itr->status = ice_add_rule_internal(hw, l_type,
>   							    em_list_itr);
>   		if (em_list_itr->status)
> diff --git a/drivers/net/ice/base/ice_switch.h b/drivers/net/ice/base/ice_switch.h
> index 2f140a86d..05b1170c9 100644
> --- a/drivers/net/ice/base/ice_switch.h
> +++ b/drivers/net/ice/base/ice_switch.h
> @@ -11,6 +11,9 @@
>   #define ICE_SW_CFG_MAX_BUF_LEN 2048
>   #define ICE_MAX_SW 256
>   #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)
>   
>   
>   /* Worst case buffer length for ice_aqc_opc_get_res_alloc */
> @@ -77,9 +80,6 @@ struct ice_fltr_info {
>   	/* rule ID returned by firmware once filter rule is created */
>   	u16 fltr_rule_id;
>   	u16 flag;
> -#define ICE_FLTR_RX		BIT(0)
> -#define ICE_FLTR_TX		BIT(1)
> -#define ICE_FLTR_TX_RX		(ICE_FLTR_RX | ICE_FLTR_TX)
>   
>   	/* Source VSI for LOOKUP_TX or source port for LOOKUP_RX */
>   	u16 src;
> @@ -145,10 +145,6 @@ struct ice_sw_act_ctrl {
>   	/* Source VSI for LOOKUP_TX or source port for LOOKUP_RX */
>   	u16 src;
>   	u16 flag;
> -#define ICE_FLTR_RX             BIT(0)
> -#define ICE_FLTR_TX             BIT(1)
> -#define ICE_FLTR_TX_RX (ICE_FLTR_RX | ICE_FLTR_TX)
> -
>   	enum ice_sw_fwd_act_type fltr_act;
>   	/* Depending on filter action */
>   	union {
> @@ -368,6 +364,8 @@ ice_aq_get_res_descs(struct ice_hw *hw, u16 num_entries,
>   		     struct ice_sq_cd *cd);
>   enum ice_status
>   ice_add_vlan(struct ice_hw *hw, struct LIST_HEAD_TYPE *m_list);
> +enum ice_status
> +ice_remove_vlan(struct ice_hw *hw, struct LIST_HEAD_TYPE *v_list);
>   void ice_rem_all_sw_rules_info(struct ice_hw *hw);
>   enum ice_status ice_add_mac(struct ice_hw *hw, struct LIST_HEAD_TYPE *m_lst);
>   enum ice_status ice_remove_mac(struct ice_hw *hw, struct LIST_HEAD_TYPE *m_lst);
> @@ -375,8 +373,6 @@ enum ice_status
>   ice_add_eth_mac(struct ice_hw *hw, struct LIST_HEAD_TYPE *em_list);
>   enum ice_status
>   ice_remove_eth_mac(struct ice_hw *hw, struct LIST_HEAD_TYPE *em_list);
> -enum ice_status
> -ice_remove_vlan(struct ice_hw *hw, struct LIST_HEAD_TYPE *v_list);
>   #ifndef NO_MACVLAN_SUPPORT
>   enum ice_status
>   ice_add_mac_vlan(struct ice_hw *hw, struct LIST_HEAD_TYPE *m_list);
> diff --git a/drivers/net/ice/base/ice_type.h b/drivers/net/ice/base/ice_type.h
> index 919ca7fa8..f4e151c55 100644
> --- a/drivers/net/ice/base/ice_type.h
> +++ b/drivers/net/ice/base/ice_type.h
> @@ -14,6 +14,10 @@
>   
>   #define BITS_PER_BYTE	8
>   
> +#ifndef _FORCE_
> +#define _FORCE_
> +#endif
> +
>   #define ICE_BYTES_PER_WORD	2
>   #define ICE_BYTES_PER_DWORD	4
>   #define ICE_MAX_TRAFFIC_CLASS	8
> @@ -35,7 +39,7 @@
>   #endif
>   
>   #ifndef IS_ASCII
> -#define IS_ASCII(_ch)  ((_ch) < 0x80)
> +#define IS_ASCII(_ch)	((_ch) < 0x80)
>   #endif
>   
>   #include "ice_status.h"
> @@ -80,6 +84,7 @@ static inline u32 ice_round_to_num(u32 N, u32 R)
>   #define ICE_HI_WORD(x)		((u16)(((x) >> 16) & 0xFFFF))
>   
>   /* debug masks - set these bits in hw->debug_mask to control output */
> +#define ICE_DBG_TRACE		BIT_ULL(0) /* for function-trace only */
>   #define ICE_DBG_INIT		BIT_ULL(1)
>   #define ICE_DBG_RELEASE		BIT_ULL(2)
>   #define ICE_DBG_FW_LOG		BIT_ULL(3)
> @@ -199,6 +204,7 @@ enum ice_vsi_type {
>   #ifdef ADQ_SUPPORT
>   	ICE_VSI_CHNL = 4,
>   #endif /* ADQ_SUPPORT */
> +	ICE_VSI_LB = 6,
>   };
>   
>   struct ice_link_status {
> @@ -718,6 +724,8 @@ struct ice_fw_log_cfg {
>   #define ICE_FW_LOG_EVNT_INIT	(ICE_AQC_FW_LOG_INIT_EN >> ICE_AQC_FW_LOG_EN_S)
>   #define ICE_FW_LOG_EVNT_FLOW	(ICE_AQC_FW_LOG_FLOW_EN >> ICE_AQC_FW_LOG_EN_S)
>   #define ICE_FW_LOG_EVNT_ERR	(ICE_AQC_FW_LOG_ERR_EN >> ICE_AQC_FW_LOG_EN_S)
> +#define ICE_FW_LOG_EVNT_ALL	(ICE_FW_LOG_EVNT_INFO | ICE_FW_LOG_EVNT_INIT | \
> +				 ICE_FW_LOG_EVNT_FLOW | ICE_FW_LOG_EVNT_ERR)
>   	struct ice_fw_log_evnt evnts[ICE_AQC_FW_LOG_ID_MAX];
>   };
>   
> @@ -745,6 +753,7 @@ struct ice_hw {
>   	u8 pf_id;		/* device profile info */
>   
>   	u16 max_burst_size;	/* driver sets this value */
> +
>   	/* Tx Scheduler values */
>   	u16 num_tx_sched_layers;
>   	u16 num_tx_sched_phys_layers;
> @@ -948,7 +957,6 @@ enum ice_sw_fwd_act_type {
>   #define ICE_SR_CSR_PROTECTED_LIST_PTR		0x0D
>   #define ICE_SR_MNG_CFG_PTR			0x0E
>   #define ICE_SR_EMP_MODULE_PTR			0x0F
> -#define ICE_SR_PBA_FLAGS			0x15
>   #define ICE_SR_PBA_BLOCK_PTR			0x16
>   #define ICE_SR_BOOT_CFG_PTR			0x17
>   #define ICE_SR_NVM_WOL_CFG			0x19
> @@ -994,6 +1002,7 @@ enum ice_sw_fwd_act_type {
>   #define ICE_SR_EMP_SR_SETTINGS_PTR		0x48
>   #define ICE_SR_CONFIGURATION_METADATA_PTR	0x4D
>   #define ICE_SR_IMMEDIATE_VALUES_PTR		0x4E
> +#define ICE_SR_POR_REGISTERS_AUTOLOAD_PTR	0x118
>   
>   /* Auxiliary field, mask and shift definition for Shadow RAM and NVM Flash */
>   #define ICE_SR_VPD_SIZE_WORDS		512
>