From a21605993dd5dfd15edfa7f06705ede17b519026 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Fri, 16 Feb 2024 14:06:35 -0800 Subject: [PATCH 1/9] ice: pass VSI pointer into ice_vc_isvalid_q_id The ice_vc_isvalid_q_id() function takes a VSI index and a queue ID. It looks up the VSI from its index, and then validates that the queue number is valid for that VSI. The VSI ID passed is typically a VSI index from the VF. This VSI number is validated by the PF to ensure that it matches the VSI associated with the VF already. In every flow where ice_vc_isvalid_q_id() is called, the PF driver already has a pointer to the VSI associated with the VF. This pointer is obtained using ice_get_vf_vsi(), rather than looking up the VSI using the index sent by the VF. Since we already know which VSI to operate on, we can modify ice_vc_isvalid_q_id() to take a VSI pointer instead of a VSI index. Pass the VSI we found from ice_get_vf_vsi() instead of re-doing the lookup. This removes some unnecessary computation and scanning of the VSI list. It also removes the last place where the driver directly used the VSI number from the VF. This will pave the way for refactoring to communicate relative VSI numbers to the VF instead of absolute numbers from the PF space. Signed-off-by: Jacob Keller Reviewed-by: Przemek Kitszel Tested-by: Rafal Romanowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_virtchnl.c | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c index c925813ec9ca..3dd4e5af0b6a 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c @@ -562,17 +562,15 @@ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id) /** * ice_vc_isvalid_q_id - * @vf: pointer to the VF info - * @vsi_id: VSI ID + * @vsi: VSI to check queue ID against * @qid: VSI relative queue ID * * check for the valid queue ID */ -static bool ice_vc_isvalid_q_id(struct ice_vf *vf, u16 vsi_id, u8 qid) +static bool ice_vc_isvalid_q_id(struct ice_vsi *vsi, u8 qid) { - struct ice_vsi *vsi = ice_find_vsi(vf->pf, vsi_id); /* allocated Tx and Rx queues should be always equal for VF VSI */ - return (vsi && (qid < vsi->alloc_txq)); + return qid < vsi->alloc_txq; } /** @@ -1330,7 +1328,7 @@ static int ice_vc_ena_qs_msg(struct ice_vf *vf, u8 *msg) */ q_map = vqs->rx_queues; for_each_set_bit(vf_q_id, &q_map, ICE_MAX_RSS_QS_PER_VF) { - if (!ice_vc_isvalid_q_id(vf, vqs->vsi_id, vf_q_id)) { + if (!ice_vc_isvalid_q_id(vsi, vf_q_id)) { v_ret = VIRTCHNL_STATUS_ERR_PARAM; goto error_param; } @@ -1352,7 +1350,7 @@ static int ice_vc_ena_qs_msg(struct ice_vf *vf, u8 *msg) q_map = vqs->tx_queues; for_each_set_bit(vf_q_id, &q_map, ICE_MAX_RSS_QS_PER_VF) { - if (!ice_vc_isvalid_q_id(vf, vqs->vsi_id, vf_q_id)) { + if (!ice_vc_isvalid_q_id(vsi, vf_q_id)) { v_ret = VIRTCHNL_STATUS_ERR_PARAM; goto error_param; } @@ -1457,7 +1455,7 @@ static int ice_vc_dis_qs_msg(struct ice_vf *vf, u8 *msg) q_map = vqs->tx_queues; for_each_set_bit(vf_q_id, &q_map, ICE_MAX_RSS_QS_PER_VF) { - if (!ice_vc_isvalid_q_id(vf, vqs->vsi_id, vf_q_id)) { + if (!ice_vc_isvalid_q_id(vsi, vf_q_id)) { v_ret = VIRTCHNL_STATUS_ERR_PARAM; goto error_param; } @@ -1483,7 +1481,7 @@ static int ice_vc_dis_qs_msg(struct ice_vf *vf, u8 *msg) bitmap_zero(vf->rxq_ena, ICE_MAX_RSS_QS_PER_VF); } else if (q_map) { for_each_set_bit(vf_q_id, &q_map, ICE_MAX_RSS_QS_PER_VF) { - if (!ice_vc_isvalid_q_id(vf, vqs->vsi_id, vf_q_id)) { + if (!ice_vc_isvalid_q_id(vsi, vf_q_id)) { v_ret = VIRTCHNL_STATUS_ERR_PARAM; goto error_param; } @@ -1539,7 +1537,7 @@ ice_cfg_interrupt(struct ice_vf *vf, struct ice_vsi *vsi, u16 vector_id, for_each_set_bit(vsi_q_id_idx, &qmap, ICE_MAX_RSS_QS_PER_VF) { vsi_q_id = vsi_q_id_idx; - if (!ice_vc_isvalid_q_id(vf, vsi->vsi_num, vsi_q_id)) + if (!ice_vc_isvalid_q_id(vsi, vsi_q_id)) return VIRTCHNL_STATUS_ERR_PARAM; q_vector->num_ring_rx++; @@ -1553,7 +1551,7 @@ ice_cfg_interrupt(struct ice_vf *vf, struct ice_vsi *vsi, u16 vector_id, for_each_set_bit(vsi_q_id_idx, &qmap, ICE_MAX_RSS_QS_PER_VF) { vsi_q_id = vsi_q_id_idx; - if (!ice_vc_isvalid_q_id(vf, vsi->vsi_num, vsi_q_id)) + if (!ice_vc_isvalid_q_id(vsi, vsi_q_id)) return VIRTCHNL_STATUS_ERR_PARAM; q_vector->num_ring_tx++; @@ -1710,7 +1708,7 @@ static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg) qpi->txq.headwb_enabled || !ice_vc_isvalid_ring_len(qpi->txq.ring_len) || !ice_vc_isvalid_ring_len(qpi->rxq.ring_len) || - !ice_vc_isvalid_q_id(vf, qci->vsi_id, qpi->txq.queue_id)) { + !ice_vc_isvalid_q_id(vsi, qpi->txq.queue_id)) { goto error_param; } From 363f689600dd010703ce6391bcfc729a97d21840 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Fri, 16 Feb 2024 14:06:36 -0800 Subject: [PATCH 2/9] ice: remove unnecessary duplicate checks for VF VSI ID The ice_vc_fdir_param_check() function validates that the VSI ID of the virtchnl flow director command matches the VSI number of the VF. This is already checked by the call to ice_vc_isvalid_vsi_id() immediately following this. This check is unnecessary since ice_vc_isvalid_vsi_id() already confirms this by checking that the VSI ID can locate the VSI associated with the VF structure. Furthermore, a following change is going to refactor the ice driver to report VSI IDs using a relative index for each VF instead of reporting the PF VSI number. This additional check would break that logic since it enforces that the VSI ID matches the VSI number. Since this check duplicates the logic in ice_vc_isvalid_vsi_id() and gets in the way of refactoring that logic, remove it. Signed-off-by: Jacob Keller Reviewed-by: Przemek Kitszel Tested-by: Rafal Romanowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c index f001553e1a1a..8e4ff3af86c6 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c @@ -94,9 +94,6 @@ ice_vc_fdir_param_check(struct ice_vf *vf, u16 vsi_id) if (!(vf->driver_caps & VIRTCHNL_VF_OFFLOAD_FDIR_PF)) return -EINVAL; - if (vsi_id != vf->lan_vsi_num) - return -EINVAL; - if (!ice_vc_isvalid_vsi_id(vf, vsi_id)) return -EINVAL; From 11fbb1bfb5bc8c98b2d7db9da332b5e568f4aaab Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Fri, 16 Feb 2024 14:06:37 -0800 Subject: [PATCH 3/9] ice: use relative VSI index for VFs instead of PF VSI number When initializing over virtchnl, the PF is required to pass a VSI ID to the VF as part of its capabilities exchange. The VF driver reports this value back to the PF in a variety of commands. The PF driver validates that this value matches the value it sent to the VF. Some hardware families such as the E700 series could use this value when reading RSS registers or communicating directly with firmware over the Admin Queue. However, E800 series hardware does not support any of these interfaces and the VF's only use for this value is to report it back to the PF. Thus, there is no requirement that this value be an actual VSI ID value of any kind. The PF driver already does not trust that the VF sends it a real VSI ID. The VSI structure is always looked up from the VF structure. The PF does validate that the VSI ID provided matches a VSI associated with the VF, but otherwise does not use the VSI ID for any purpose. Instead of reporting the VSI number relative to the PF space, report a fixed value of 1. When communicating with the VF over virtchnl, validate that the VSI number is returned appropriately. This avoids leaking information about the firmware of the PF state. Currently the ice driver only supplies a VF with a single VSI. However, it appears that virtchnl has some support for allowing multiple VSIs. I did not attempt to implement this. However, space is left open to allow further relative indexes if additional VSIs are provided in future feature development. For this reason, keep the ice_vc_isvalid_vsi_id function in place to allow extending it for multiple VSIs in the future. This change will also simplify handling of live migration in a future series. Since we no longer will provide a real VSI number to the VF, there will be no need to keep track of this number when migrating to a new host. Signed-off-by: Jacob Keller Reviewed-by: Przemek Kitszel Tested-by: Rafal Romanowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_virtchnl.c | 9 ++------- drivers/net/ethernet/intel/ice/ice_virtchnl.h | 9 +++++++++ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c index 3dd4e5af0b6a..6b88b9bfb51e 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c @@ -506,7 +506,7 @@ static int ice_vc_get_vf_res_msg(struct ice_vf *vf, u8 *msg) vfres->rss_lut_size = ICE_LUT_VSI_SIZE; vfres->max_mtu = ice_vc_get_max_frame_size(vf); - vfres->vsi_res[0].vsi_id = vf->lan_vsi_num; + vfres->vsi_res[0].vsi_id = ICE_VF_VSI_ID; vfres->vsi_res[0].vsi_type = VIRTCHNL_VSI_SRIOV; vfres->vsi_res[0].num_queue_pairs = vsi->num_txq; ether_addr_copy(vfres->vsi_res[0].default_mac_addr, @@ -552,12 +552,7 @@ static void ice_vc_reset_vf_msg(struct ice_vf *vf) */ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id) { - struct ice_pf *pf = vf->pf; - struct ice_vsi *vsi; - - vsi = ice_find_vsi(pf, vsi_id); - - return (vsi && (vsi->vf == vf)); + return vsi_id == ICE_VF_VSI_ID; } /** diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.h b/drivers/net/ethernet/intel/ice/ice_virtchnl.h index 60dfbe05980a..3a4115869153 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.h +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.h @@ -19,6 +19,15 @@ #define ICE_MAX_MACADDR_PER_VF 18 #define ICE_FLEX_DESC_RXDID_MAX_NUM 64 +/* VFs only get a single VSI. For ice hardware, the VF does not need to know + * its VSI index. However, the virtchnl interface requires a VSI number, + * mainly due to legacy hardware. + * + * Since the VF doesn't need this information, report a static value to the VF + * instead of leaking any information about the PF or hardware setup. + */ +#define ICE_VF_VSI_ID 1 + struct ice_virtchnl_ops { int (*get_ver_msg)(struct ice_vf *vf, u8 *msg); int (*get_vf_res_msg)(struct ice_vf *vf, u8 *msg); From 1cf94cbfc61bac89cddeb075fbc100ebd3aea81b Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Fri, 16 Feb 2024 14:06:38 -0800 Subject: [PATCH 4/9] ice: remove vf->lan_vsi_num field The lan_vsi_num field of the VF structure is no longer used for any purpose. Remove it. Signed-off-by: Jacob Keller Reviewed-by: Przemek Kitszel Tested-by: Rafal Romanowski Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_sriov.c | 1 - drivers/net/ethernet/intel/ice/ice_vf_lib.c | 10 +--------- drivers/net/ethernet/intel/ice/ice_vf_lib.h | 5 ----- 3 files changed, 1 insertion(+), 15 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c index a94a1c48c3de..a2f8dbe3cb82 100644 --- a/drivers/net/ethernet/intel/ice/ice_sriov.c +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c @@ -240,7 +240,6 @@ static struct ice_vsi *ice_vf_vsi_setup(struct ice_vf *vf) } vf->lan_vsi_idx = vsi->idx; - vf->lan_vsi_num = vsi->vsi_num; return vsi; } diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c index 2ffdae9a82df..21d26e19338a 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c @@ -280,12 +280,6 @@ int ice_vf_reconfig_vsi(struct ice_vf *vf) return err; } - /* Update the lan_vsi_num field since it might have been changed. The - * PF lan_vsi_idx number remains the same so we don't need to change - * that. - */ - vf->lan_vsi_num = vsi->vsi_num; - return 0; } @@ -315,7 +309,6 @@ static int ice_vf_rebuild_vsi(struct ice_vf *vf) * vf->lan_vsi_idx */ vsi->vsi_num = ice_get_hw_vsi_num(&pf->hw, vsi->idx); - vf->lan_vsi_num = vsi->vsi_num; return 0; } @@ -1315,13 +1308,12 @@ int ice_vf_init_host_cfg(struct ice_vf *vf, struct ice_vsi *vsi) } /** - * ice_vf_invalidate_vsi - invalidate vsi_idx/vsi_num to remove VSI access + * ice_vf_invalidate_vsi - invalidate vsi_idx to remove VSI access * @vf: VF to remove access to VSI for */ void ice_vf_invalidate_vsi(struct ice_vf *vf) { vf->lan_vsi_idx = ICE_NO_VSI; - vf->lan_vsi_num = ICE_NO_VSI; } /** diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h index 0cc9034065c5..fec16919ec19 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h @@ -109,11 +109,6 @@ struct ice_vf { u8 spoofchk:1; u8 link_forced:1; u8 link_up:1; /* only valid if VF link is forced */ - /* VSI indices - actual VSI pointers are maintained in the PF structure - * When assigned, these will be non-zero, because VSI 0 is always - * the main LAN VSI for the PF. - */ - u16 lan_vsi_num; /* ID as used by firmware */ unsigned int min_tx_rate; /* Minimum Tx bandwidth limit in Mbps */ unsigned int max_tx_rate; /* Maximum Tx bandwidth limit in Mbps */ DECLARE_BITMAP(vf_states, ICE_VF_STATES_NBITS); /* VF runtime states */ From 1260b45dbe2dbc415f3bc1e841c6c098083bcfb8 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 26 Feb 2024 16:14:54 -0800 Subject: [PATCH 5/9] ice: rename ice_write_* functions to ice_pack_ctx_* In ice_common.c there are 4 functions used for converting the unpacked software Tx and Rx context structure data into the packed format used by hardware. These functions have extremely generic names: * ice_write_byte * ice_write_word * ice_write_dword * ice_write_qword When I saw these function names my first thought was "write what? to where?". Understanding what these functions do requires looking at the implementation details. The functions take bits from an unpacked structure and copy them into the packed layout used by hardware. As part of live migration, we will want functions which perform the inverse operation of reading bits from the packed layout and copying them into the unpacked format. Naming these as "ice_read_byte", etc would be very confusing since they appear to write data. In preparation for adding this new inverse operation, rename the existing functions to use the prefix "ice_pack_ctx_". This makes it clear that they perform the bit packing while copying from the unpacked software context structure to the packed hardware context. The inverse operations can then neatly be named ice_unpack_ctx_*, clearly indicating they perform the bit unpacking while copying from the packed hardware context to the unpacked software context structure. Signed-off-by: Jacob Keller Reviewed-by: Przemek Kitszel Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_common.c | 56 ++++++++++----------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 9266f25a9978..3622079cb506 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -4362,13 +4362,13 @@ ice_aq_add_rdma_qsets(struct ice_hw *hw, u8 num_qset_grps, /* End of FW Admin Queue command wrappers */ /** - * ice_write_byte - write a byte to a packed context structure - * @src_ctx: the context structure to read from - * @dest_ctx: the context to be written to - * @ce_info: a description of the struct to be filled + * ice_pack_ctx_byte - write a byte to a packed context structure + * @src_ctx: unpacked source context structure + * @dest_ctx: packed destination context data + * @ce_info: context element description */ -static void -ice_write_byte(u8 *src_ctx, u8 *dest_ctx, const struct ice_ctx_ele *ce_info) +static void ice_pack_ctx_byte(u8 *src_ctx, u8 *dest_ctx, + const struct ice_ctx_ele *ce_info) { u8 src_byte, dest_byte, mask; u8 *from, *dest; @@ -4401,13 +4401,13 @@ ice_write_byte(u8 *src_ctx, u8 *dest_ctx, const struct ice_ctx_ele *ce_info) } /** - * ice_write_word - write a word to a packed context structure - * @src_ctx: the context structure to read from - * @dest_ctx: the context to be written to - * @ce_info: a description of the struct to be filled + * ice_pack_ctx_word - write a word to a packed context structure + * @src_ctx: unpacked source context structure + * @dest_ctx: packed destination context data + * @ce_info: context element description */ -static void -ice_write_word(u8 *src_ctx, u8 *dest_ctx, const struct ice_ctx_ele *ce_info) +static void ice_pack_ctx_word(u8 *src_ctx, u8 *dest_ctx, + const struct ice_ctx_ele *ce_info) { u16 src_word, mask; __le16 dest_word; @@ -4444,13 +4444,13 @@ ice_write_word(u8 *src_ctx, u8 *dest_ctx, const struct ice_ctx_ele *ce_info) } /** - * ice_write_dword - write a dword to a packed context structure - * @src_ctx: the context structure to read from - * @dest_ctx: the context to be written to - * @ce_info: a description of the struct to be filled + * ice_pack_ctx_dword - write a dword to a packed context structure + * @src_ctx: unpacked source context structure + * @dest_ctx: packed destination context data + * @ce_info: context element description */ -static void -ice_write_dword(u8 *src_ctx, u8 *dest_ctx, const struct ice_ctx_ele *ce_info) +static void ice_pack_ctx_dword(u8 *src_ctx, u8 *dest_ctx, + const struct ice_ctx_ele *ce_info) { u32 src_dword, mask; __le32 dest_dword; @@ -4495,13 +4495,13 @@ ice_write_dword(u8 *src_ctx, u8 *dest_ctx, const struct ice_ctx_ele *ce_info) } /** - * ice_write_qword - write a qword to a packed context structure - * @src_ctx: the context structure to read from - * @dest_ctx: the context to be written to - * @ce_info: a description of the struct to be filled + * ice_pack_ctx_qword - write a qword to a packed context structure + * @src_ctx: unpacked source context structure + * @dest_ctx: packed destination context data + * @ce_info: context element description */ -static void -ice_write_qword(u8 *src_ctx, u8 *dest_ctx, const struct ice_ctx_ele *ce_info) +static void ice_pack_ctx_qword(u8 *src_ctx, u8 *dest_ctx, + const struct ice_ctx_ele *ce_info) { u64 src_qword, mask; __le64 dest_qword; @@ -4570,16 +4570,16 @@ ice_set_ctx(struct ice_hw *hw, u8 *src_ctx, u8 *dest_ctx, } switch (ce_info[f].size_of) { case sizeof(u8): - ice_write_byte(src_ctx, dest_ctx, &ce_info[f]); + ice_pack_ctx_byte(src_ctx, dest_ctx, &ce_info[f]); break; case sizeof(u16): - ice_write_word(src_ctx, dest_ctx, &ce_info[f]); + ice_pack_ctx_word(src_ctx, dest_ctx, &ce_info[f]); break; case sizeof(u32): - ice_write_dword(src_ctx, dest_ctx, &ce_info[f]); + ice_pack_ctx_dword(src_ctx, dest_ctx, &ce_info[f]); break; case sizeof(u64): - ice_write_qword(src_ctx, dest_ctx, &ce_info[f]); + ice_pack_ctx_qword(src_ctx, dest_ctx, &ce_info[f]); break; default: return -EINVAL; From a45d1bf516c097bb7ae4983d3128ebf139be952c Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 26 Feb 2024 16:14:55 -0800 Subject: [PATCH 6/9] ice: use GENMASK instead of BIT(n) - 1 in pack functions The functions used to pack the Tx and Rx context into the hardware format rely on using BIT() and then subtracting 1 to get a bitmask. These functions even have a comment about how x86 machines can't use this method for certain widths because the SHL instructions will not work properly. The Linux kernel already provides the GENMASK macro for generating a suitable bitmask. Further, GENMASK is capable of generating the mask including the shift_width. Since width is the total field width, take care to subtract one to get the final bit position. Since we now include the shifted bits as part of the mask, shift the source value first before applying the mask. Signed-off-by: Jacob Keller Reviewed-by: Przemek Kitszel Tested-by: Pucha Himasekhar Reddy (A Contingent worker at Intel) Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_common.c | 44 ++++----------------- 1 file changed, 8 insertions(+), 36 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 3622079cb506..e9e4a8ef7904 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -4379,14 +4379,11 @@ static void ice_pack_ctx_byte(u8 *src_ctx, u8 *dest_ctx, /* prepare the bits and mask */ shift_width = ce_info->lsb % 8; - mask = (u8)(BIT(ce_info->width) - 1); + mask = GENMASK(ce_info->width - 1 + shift_width, shift_width); src_byte = *from; - src_byte &= mask; - - /* shift to correct alignment */ - mask <<= shift_width; src_byte <<= shift_width; + src_byte &= mask; /* get the current bits from the target bit string */ dest = dest_ctx + (ce_info->lsb / 8); @@ -4419,17 +4416,14 @@ static void ice_pack_ctx_word(u8 *src_ctx, u8 *dest_ctx, /* prepare the bits and mask */ shift_width = ce_info->lsb % 8; - mask = BIT(ce_info->width) - 1; + mask = GENMASK(ce_info->width - 1 + shift_width, shift_width); /* don't swizzle the bits until after the mask because the mask bits * will be in a different bit position on big endian machines */ src_word = *(u16 *)from; - src_word &= mask; - - /* shift to correct alignment */ - mask <<= shift_width; src_word <<= shift_width; + src_word &= mask; /* get the current bits from the target bit string */ dest = dest_ctx + (ce_info->lsb / 8); @@ -4462,25 +4456,14 @@ static void ice_pack_ctx_dword(u8 *src_ctx, u8 *dest_ctx, /* prepare the bits and mask */ shift_width = ce_info->lsb % 8; - - /* if the field width is exactly 32 on an x86 machine, then the shift - * operation will not work because the SHL instructions count is masked - * to 5 bits so the shift will do nothing - */ - if (ce_info->width < 32) - mask = BIT(ce_info->width) - 1; - else - mask = (u32)~0; + mask = GENMASK(ce_info->width - 1 + shift_width, shift_width); /* don't swizzle the bits until after the mask because the mask bits * will be in a different bit position on big endian machines */ src_dword = *(u32 *)from; - src_dword &= mask; - - /* shift to correct alignment */ - mask <<= shift_width; src_dword <<= shift_width; + src_dword &= mask; /* get the current bits from the target bit string */ dest = dest_ctx + (ce_info->lsb / 8); @@ -4513,25 +4496,14 @@ static void ice_pack_ctx_qword(u8 *src_ctx, u8 *dest_ctx, /* prepare the bits and mask */ shift_width = ce_info->lsb % 8; - - /* if the field width is exactly 64 on an x86 machine, then the shift - * operation will not work because the SHL instructions count is masked - * to 6 bits so the shift will do nothing - */ - if (ce_info->width < 64) - mask = BIT_ULL(ce_info->width) - 1; - else - mask = (u64)~0; + mask = GENMASK_ULL(ce_info->width - 1 + shift_width, shift_width); /* don't swizzle the bits until after the mask because the mask bits * will be in a different bit position on big endian machines */ src_qword = *(u64 *)from; - src_qword &= mask; - - /* shift to correct alignment */ - mask <<= shift_width; src_qword <<= shift_width; + src_qword &= mask; /* get the current bits from the target bit string */ dest = dest_ctx + (ce_info->lsb / 8); From 979c2c049fbea107ce9f8d31f3ba9dba83ddb0a2 Mon Sep 17 00:00:00 2001 From: Jacob Keller Date: Mon, 26 Feb 2024 16:14:56 -0800 Subject: [PATCH 7/9] ice: cleanup line splitting for context set functions The indentation for ice_set_ctx and ice_write_rxq_ctx breaks the function name after the return type. This style of breaking is used a lot throughout the ice driver, even in cases where its not actually helpful for readability. We no longer prefer this style of line splitting in the driver, and new code is avoiding it. Normally, I would leave this alone unless the actual function contents or description needed updating. However, a future change is going to add inverse functions for converting packed context to unpacked context structures. To keep this code uniform with the existing set functions, fix up the style to the modern format of keeping the type on the same line. Signed-off-by: Jacob Keller Reviewed-by: Przemek Kitszel Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_common.c | 12 +++++------- drivers/net/ethernet/intel/ice/ice_common.h | 10 ++++------ 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index e9e4a8ef7904..7d9315ffae57 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -1399,9 +1399,8 @@ static const struct ice_ctx_ele ice_rlan_ctx_info[] = { * it to HW register space and enables the hardware to prefetch descriptors * instead of only fetching them on demand */ -int -ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx, - u32 rxq_index) +int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx, + u32 rxq_index) { u8 ctx_buf[ICE_RXQ_CTX_SZ] = { 0 }; @@ -4522,11 +4521,10 @@ static void ice_pack_ctx_qword(u8 *src_ctx, u8 *dest_ctx, * @hw: pointer to the hardware structure * @src_ctx: pointer to a generic non-packed context structure * @dest_ctx: pointer to memory for the packed structure - * @ce_info: a description of the structure to be transformed + * @ce_info: List of Rx context elements */ -int -ice_set_ctx(struct ice_hw *hw, u8 *src_ctx, u8 *dest_ctx, - const struct ice_ctx_ele *ce_info) +int ice_set_ctx(struct ice_hw *hw, u8 *src_ctx, u8 *dest_ctx, + const struct ice_ctx_ele *ce_info) { int f; diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h index 32fd10de620c..ffb22c7ce28b 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.h +++ b/drivers/net/ethernet/intel/ice/ice_common.h @@ -53,9 +53,8 @@ int ice_get_caps(struct ice_hw *hw); void ice_set_safe_mode_caps(struct ice_hw *hw); -int -ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx, - u32 rxq_index); +int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx, + u32 rxq_index); int ice_aq_get_rss_lut(struct ice_hw *hw, struct ice_aq_get_set_rss_lut_params *get_params); @@ -72,9 +71,8 @@ bool ice_check_sq_alive(struct ice_hw *hw, struct ice_ctl_q_info *cq); int ice_aq_q_shutdown(struct ice_hw *hw, bool unloading); void ice_fill_dflt_direct_cmd_desc(struct ice_aq_desc *desc, u16 opcode); extern const struct ice_ctx_ele ice_tlan_ctx_info[]; -int -ice_set_ctx(struct ice_hw *hw, u8 *src_ctx, u8 *dest_ctx, - const struct ice_ctx_ele *ce_info); +int ice_set_ctx(struct ice_hw *hw, u8 *src_ctx, u8 *dest_ctx, + const struct ice_ctx_ele *ce_info); extern struct mutex ice_global_cfg_lock_sw; From d5926e01e3739542bb047b77f850d7f641eaa7bc Mon Sep 17 00:00:00 2001 From: Maciej Fijalkowski Date: Fri, 23 Feb 2024 17:06:27 +0100 Subject: [PATCH 8/9] ice: do not disable Tx queues twice in ice_down() ice_down() clears QINT_TQCTL_CAUSE_ENA_M bit twice, which is not necessary. First clearing happens in ice_vsi_dis_irq() and second in ice_vsi_stop_tx_ring() - remove the first one. While at it, make ice_vsi_dis_irq() static as ice_down() is the only current caller of it. Signed-off-by: Maciej Fijalkowski Tested-by: Pucha Himasekhar Reddy (A Contingent worker at Intel) Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_lib.c | 55 ----------------------- drivers/net/ethernet/intel/ice/ice_lib.h | 2 - drivers/net/ethernet/intel/ice/ice_main.c | 44 ++++++++++++++++++ 3 files changed, 44 insertions(+), 57 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index 59e8a2572985..48e4c33a3550 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -2719,61 +2719,6 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) } } -/** - * ice_vsi_dis_irq - Mask off queue interrupt generation on the VSI - * @vsi: the VSI being un-configured - */ -void ice_vsi_dis_irq(struct ice_vsi *vsi) -{ - struct ice_pf *pf = vsi->back; - struct ice_hw *hw = &pf->hw; - u32 val; - int i; - - /* disable interrupt causation from each queue */ - if (vsi->tx_rings) { - ice_for_each_txq(vsi, i) { - if (vsi->tx_rings[i]) { - u16 reg; - - reg = vsi->tx_rings[i]->reg_idx; - val = rd32(hw, QINT_TQCTL(reg)); - val &= ~QINT_TQCTL_CAUSE_ENA_M; - wr32(hw, QINT_TQCTL(reg), val); - } - } - } - - if (vsi->rx_rings) { - ice_for_each_rxq(vsi, i) { - if (vsi->rx_rings[i]) { - u16 reg; - - reg = vsi->rx_rings[i]->reg_idx; - val = rd32(hw, QINT_RQCTL(reg)); - val &= ~QINT_RQCTL_CAUSE_ENA_M; - wr32(hw, QINT_RQCTL(reg), val); - } - } - } - - /* disable each interrupt */ - ice_for_each_q_vector(vsi, i) { - if (!vsi->q_vectors[i]) - continue; - wr32(hw, GLINT_DYN_CTL(vsi->q_vectors[i]->reg_idx), 0); - } - - ice_flush(hw); - - /* don't call synchronize_irq() for VF's from the host */ - if (vsi->type == ICE_VSI_VF) - return; - - ice_for_each_q_vector(vsi, i) - synchronize_irq(vsi->q_vectors[i]->irq.virq); -} - /** * __ice_queue_set_napi - Set the napi instance for the queue * @dev: device to which NAPI and queue belong diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h index b5a1ed7cc4b1..9cd23afe5f15 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_lib.h @@ -110,8 +110,6 @@ void ice_write_qrxflxp_cntxt(struct ice_hw *hw, u16 pf_q, u32 rxdid, u32 prio, bool ena_ts); -void ice_vsi_dis_irq(struct ice_vsi *vsi); - void ice_vsi_free_irq(struct ice_vsi *vsi); void ice_vsi_free_rx_rings(struct ice_vsi *vsi); diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 8f73ba77e835..062b4ea8e2c3 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -7001,6 +7001,50 @@ static void ice_napi_disable_all(struct ice_vsi *vsi) } } +/** + * ice_vsi_dis_irq - Mask off queue interrupt generation on the VSI + * @vsi: the VSI being un-configured + */ +static void ice_vsi_dis_irq(struct ice_vsi *vsi) +{ + struct ice_pf *pf = vsi->back; + struct ice_hw *hw = &pf->hw; + u32 val; + int i; + + /* disable interrupt causation from each Rx queue; Tx queues are + * handled in ice_vsi_stop_tx_ring() + */ + if (vsi->rx_rings) { + ice_for_each_rxq(vsi, i) { + if (vsi->rx_rings[i]) { + u16 reg; + + reg = vsi->rx_rings[i]->reg_idx; + val = rd32(hw, QINT_RQCTL(reg)); + val &= ~QINT_RQCTL_CAUSE_ENA_M; + wr32(hw, QINT_RQCTL(reg), val); + } + } + } + + /* disable each interrupt */ + ice_for_each_q_vector(vsi, i) { + if (!vsi->q_vectors[i]) + continue; + wr32(hw, GLINT_DYN_CTL(vsi->q_vectors[i]->reg_idx), 0); + } + + ice_flush(hw); + + /* don't call synchronize_irq() for VF's from the host */ + if (vsi->type == ICE_VSI_VF) + return; + + ice_for_each_q_vector(vsi, i) + synchronize_irq(vsi->q_vectors[i]->irq.virq); +} + /** * ice_down - Shutdown the connection * @vsi: The VSI being stopped From 90f821d72e112d5e4e4214a3704935283cc53375 Mon Sep 17 00:00:00 2001 From: Maciej Fijalkowski Date: Fri, 23 Feb 2024 17:06:28 +0100 Subject: [PATCH 9/9] ice: avoid unnecessary devm_ usage 1. pcaps are free'd right after AQ routines are done, no need for devm_'s 2. a test frame for loopback test in ethtool -t is destroyed at the end of the test so we don't need devm_ here either. Signed-off-by: Maciej Fijalkowski Reviewed-by: Przemek Kitszel Tested-by: Pucha Himasekhar Reddy (A Contingent worker at Intel) Signed-off-by: Tony Nguyen --- drivers/net/ethernet/intel/ice/ice_common.c | 34 +++++++------------- drivers/net/ethernet/intel/ice/ice_ethtool.c | 10 ++---- 2 files changed, 14 insertions(+), 30 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 7d9315ffae57..4d8111aeb0ff 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -1002,9 +1002,9 @@ static void ice_get_itr_intrl_gran(struct ice_hw *hw) */ int ice_init_hw(struct ice_hw *hw) { - struct ice_aqc_get_phy_caps_data *pcaps; + struct ice_aqc_get_phy_caps_data *pcaps __free(kfree); + void *mac_buf __free(kfree); u16 mac_buf_len; - void *mac_buf; int status; /* Set MAC type based on DeviceID */ @@ -1082,7 +1082,7 @@ int ice_init_hw(struct ice_hw *hw) if (status) goto err_unroll_sched; - pcaps = devm_kzalloc(ice_hw_to_dev(hw), sizeof(*pcaps), GFP_KERNEL); + pcaps = kzalloc(sizeof(*pcaps), GFP_KERNEL); if (!pcaps) { status = -ENOMEM; goto err_unroll_sched; @@ -1092,7 +1092,6 @@ int ice_init_hw(struct ice_hw *hw) status = ice_aq_get_phy_caps(hw->port_info, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA, pcaps, NULL); - devm_kfree(ice_hw_to_dev(hw), pcaps); if (status) dev_warn(ice_hw_to_dev(hw), "Get PHY capabilities failed status = %d, continuing anyway\n", status); @@ -1119,18 +1118,15 @@ int ice_init_hw(struct ice_hw *hw) /* Get MAC information */ /* A single port can report up to two (LAN and WoL) addresses */ - mac_buf = devm_kcalloc(ice_hw_to_dev(hw), 2, - sizeof(struct ice_aqc_manage_mac_read_resp), - GFP_KERNEL); - mac_buf_len = 2 * sizeof(struct ice_aqc_manage_mac_read_resp); - + mac_buf = kcalloc(2, sizeof(struct ice_aqc_manage_mac_read_resp), + GFP_KERNEL); if (!mac_buf) { status = -ENOMEM; goto err_unroll_fltr_mgmt_struct; } + mac_buf_len = 2 * sizeof(struct ice_aqc_manage_mac_read_resp); status = ice_aq_manage_mac_read(hw, mac_buf, mac_buf_len, NULL); - devm_kfree(ice_hw_to_dev(hw), mac_buf); if (status) goto err_unroll_fltr_mgmt_struct; @@ -3276,19 +3272,14 @@ int ice_update_link_info(struct ice_port_info *pi) return status; if (li->link_info & ICE_AQ_MEDIA_AVAILABLE) { - struct ice_aqc_get_phy_caps_data *pcaps; - struct ice_hw *hw; + struct ice_aqc_get_phy_caps_data *pcaps __free(kfree); - hw = pi->hw; - pcaps = devm_kzalloc(ice_hw_to_dev(hw), sizeof(*pcaps), - GFP_KERNEL); + pcaps = kzalloc(sizeof(*pcaps), GFP_KERNEL); if (!pcaps) return -ENOMEM; status = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA, pcaps, NULL); - - devm_kfree(ice_hw_to_dev(hw), pcaps); } return status; @@ -3429,8 +3420,8 @@ ice_cfg_phy_fc(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg, int ice_set_fc(struct ice_port_info *pi, u8 *aq_failures, bool ena_auto_link_update) { + struct ice_aqc_get_phy_caps_data *pcaps __free(kfree); struct ice_aqc_set_phy_cfg_data cfg = { 0 }; - struct ice_aqc_get_phy_caps_data *pcaps; struct ice_hw *hw; int status; @@ -3440,7 +3431,7 @@ ice_set_fc(struct ice_port_info *pi, u8 *aq_failures, bool ena_auto_link_update) *aq_failures = 0; hw = pi->hw; - pcaps = devm_kzalloc(ice_hw_to_dev(hw), sizeof(*pcaps), GFP_KERNEL); + pcaps = kzalloc(sizeof(*pcaps), GFP_KERNEL); if (!pcaps) return -ENOMEM; @@ -3492,7 +3483,6 @@ ice_set_fc(struct ice_port_info *pi, u8 *aq_failures, bool ena_auto_link_update) } out: - devm_kfree(ice_hw_to_dev(hw), pcaps); return status; } @@ -3571,7 +3561,7 @@ int ice_cfg_phy_fec(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg, enum ice_fec_mode fec) { - struct ice_aqc_get_phy_caps_data *pcaps; + struct ice_aqc_get_phy_caps_data *pcaps __free(kfree); struct ice_hw *hw; int status; @@ -3640,8 +3630,6 @@ ice_cfg_phy_fec(struct ice_port_info *pi, struct ice_aqc_set_phy_cfg_data *cfg, } out: - kfree(pcaps); - return status; } diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c index 3cc364a4d682..bb912155676e 100644 --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c @@ -802,7 +802,7 @@ static int ice_lbtest_create_frame(struct ice_pf *pf, u8 **ret_data, u16 size) if (!pf) return -EINVAL; - data = devm_kzalloc(ice_pf_to_dev(pf), size, GFP_KERNEL); + data = kzalloc(size, GFP_KERNEL); if (!data) return -ENOMEM; @@ -945,11 +945,9 @@ static u64 ice_loopback_test(struct net_device *netdev) int num_frames, valid_frames; struct ice_tx_ring *tx_ring; struct ice_rx_ring *rx_ring; - struct device *dev; - u8 *tx_frame; + u8 *tx_frame __free(kfree); int i; - dev = ice_pf_to_dev(pf); netdev_info(netdev, "loopback test\n"); test_vsi = ice_lb_vsi_setup(pf, pf->hw.port_info); @@ -994,7 +992,7 @@ static u64 ice_loopback_test(struct net_device *netdev) for (i = 0; i < num_frames; i++) { if (ice_diag_send(tx_ring, tx_frame, ICE_LB_FRAME_SIZE)) { ret = 8; - goto lbtest_free_frame; + goto remove_mac_filters; } } @@ -1004,8 +1002,6 @@ static u64 ice_loopback_test(struct net_device *netdev) else if (valid_frames != num_frames) ret = 10; -lbtest_free_frame: - devm_kfree(dev, tx_frame); remove_mac_filters: if (ice_fltr_remove_mac(test_vsi, broadcast, ICE_FWD_TO_VSI)) netdev_err(netdev, "Could not remove MAC filter for the test VSI\n");