| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7596497f-394d-20f2-d517-064674d38b5e@intel.com>
Date: Thu, 26 Oct 2023 11:10:12 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Ivan Vecera <ivecera@...hat.com>, <netdev@...r.kernel.org>
CC: Jesse Brandeburg <jesse.brandeburg@...el.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>,
"David S. Miller" <davem@...emloft.net>,
"Eric Dumazet" <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
<intel-wired-lan@...ts.osuosl.org>, <linux-kernel@...r.kernel.org>,
Jacob Keller <jacob.e.keller@...el.com>,
Wojciech Drewek <wojciech.drewek@...el.com>
Subject: Re: [PATCH iwl-next] iavf: Remove queue tracking fields from
iavf_adminq_ring
On 10/26/23 10:39, Ivan Vecera wrote:
> Fields 'head', 'tail', 'len', 'bah' and 'bal' in iavf_adminq_ring
> are used to store register offsets. These offsets are initialized
> and remains constant so there is no need to store them in the
> iavf_adminq_ring structure.
>
> Remove these fields from iavf_adminq_ring and use register offset
> constants instead. Remove iavf_adminq_init_regs() that originally
> stores these constants into these fields.
>
> Finally improve iavf_check_asq_alive() that assumes that
> non-zero value of hw->aq.asq.len indicates fully initialized
> AdminQ send queue. Replace it by check for non-zero value
> of field hw->aq.asq.count that is non-zero when the sending
> queue is initialized and is zeroed during shutdown of
> the queue.
That part could be a separate commit, perhaps even for -net
>
> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
> ---
> drivers/net/ethernet/intel/iavf/iavf_adminq.c | 86 +++++++------------
> drivers/net/ethernet/intel/iavf/iavf_adminq.h | 7 --
> drivers/net/ethernet/intel/iavf/iavf_common.c | 8 +-
> drivers/net/ethernet/intel/iavf/iavf_main.c | 8 +-
> 4 files changed, 39 insertions(+), 70 deletions(-)
anyway, this is fine, so
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_adminq.c b/drivers/net/ethernet/intel/iavf/iavf_adminq.c
> index 9ffbd24d83cb..82fcd18ad660 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_adminq.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_adminq.c
> @@ -7,27 +7,6 @@
> #include "iavf_adminq.h"
> #include "iavf_prototype.h"
>
> -/**
> - * iavf_adminq_init_regs - Initialize AdminQ registers
> - * @hw: pointer to the hardware structure
> - *
> - * This assumes the alloc_asq and alloc_arq functions have already been called
> - **/
> -static void iavf_adminq_init_regs(struct iavf_hw *hw)
> -{
> - /* set head and tail registers in our local struct */
> - hw->aq.asq.tail = IAVF_VF_ATQT1;
> - hw->aq.asq.head = IAVF_VF_ATQH1;
> - hw->aq.asq.len = IAVF_VF_ATQLEN1;
> - hw->aq.asq.bal = IAVF_VF_ATQBAL1;
> - hw->aq.asq.bah = IAVF_VF_ATQBAH1;
> - hw->aq.arq.tail = IAVF_VF_ARQT1;
> - hw->aq.arq.head = IAVF_VF_ARQH1;
> - hw->aq.arq.len = IAVF_VF_ARQLEN1;
> - hw->aq.arq.bal = IAVF_VF_ARQBAL1;
> - hw->aq.arq.bah = IAVF_VF_ARQBAH1;
> -}
> -
> /**
> * iavf_alloc_adminq_asq_ring - Allocate Admin Queue send rings
> * @hw: pointer to the hardware structure
> @@ -259,17 +238,17 @@ static enum iavf_status iavf_config_asq_regs(struct iavf_hw *hw)
> u32 reg = 0;
>
> /* Clear Head and Tail */
> - wr32(hw, hw->aq.asq.head, 0);
> - wr32(hw, hw->aq.asq.tail, 0);
> + wr32(hw, IAVF_VF_ATQH1, 0);
> + wr32(hw, IAVF_VF_ATQT1, 0);
>
> /* set starting point */
> - wr32(hw, hw->aq.asq.len, (hw->aq.num_asq_entries |
> + wr32(hw, IAVF_VF_ATQLEN1, (hw->aq.num_asq_entries |
> IAVF_VF_ATQLEN1_ATQENABLE_MASK));
> - wr32(hw, hw->aq.asq.bal, lower_32_bits(hw->aq.asq.desc_buf.pa));
> - wr32(hw, hw->aq.asq.bah, upper_32_bits(hw->aq.asq.desc_buf.pa));
> + wr32(hw, IAVF_VF_ATQBAL1, lower_32_bits(hw->aq.asq.desc_buf.pa));
> + wr32(hw, IAVF_VF_ATQBAH1, upper_32_bits(hw->aq.asq.desc_buf.pa));
>
> /* Check one register to verify that config was applied */
> - reg = rd32(hw, hw->aq.asq.bal);
> + reg = rd32(hw, IAVF_VF_ATQBAL1);
> if (reg != lower_32_bits(hw->aq.asq.desc_buf.pa))
> ret_code = IAVF_ERR_ADMIN_QUEUE_ERROR;
>
> @@ -288,20 +267,20 @@ static enum iavf_status iavf_config_arq_regs(struct iavf_hw *hw)
> u32 reg = 0;
>
> /* Clear Head and Tail */
> - wr32(hw, hw->aq.arq.head, 0);
> - wr32(hw, hw->aq.arq.tail, 0);
> + wr32(hw, IAVF_VF_ARQH1, 0);
> + wr32(hw, IAVF_VF_ARQT1, 0);
>
> /* set starting point */
> - wr32(hw, hw->aq.arq.len, (hw->aq.num_arq_entries |
> + wr32(hw, IAVF_VF_ARQLEN1, (hw->aq.num_arq_entries |
> IAVF_VF_ARQLEN1_ARQENABLE_MASK));
> - wr32(hw, hw->aq.arq.bal, lower_32_bits(hw->aq.arq.desc_buf.pa));
> - wr32(hw, hw->aq.arq.bah, upper_32_bits(hw->aq.arq.desc_buf.pa));
> + wr32(hw, IAVF_VF_ARQBAL1, lower_32_bits(hw->aq.arq.desc_buf.pa));
> + wr32(hw, IAVF_VF_ARQBAH1, upper_32_bits(hw->aq.arq.desc_buf.pa));
>
> /* Update tail in the HW to post pre-allocated buffers */
> - wr32(hw, hw->aq.arq.tail, hw->aq.num_arq_entries - 1);
> + wr32(hw, IAVF_VF_ARQT1, hw->aq.num_arq_entries - 1);
>
> /* Check one register to verify that config was applied */
> - reg = rd32(hw, hw->aq.arq.bal);
> + reg = rd32(hw, IAVF_VF_ARQBAL1);
> if (reg != lower_32_bits(hw->aq.arq.desc_buf.pa))
> ret_code = IAVF_ERR_ADMIN_QUEUE_ERROR;
>
> @@ -455,11 +434,11 @@ static enum iavf_status iavf_shutdown_asq(struct iavf_hw *hw)
> }
>
> /* Stop firmware AdminQ processing */
> - wr32(hw, hw->aq.asq.head, 0);
> - wr32(hw, hw->aq.asq.tail, 0);
> - wr32(hw, hw->aq.asq.len, 0);
> - wr32(hw, hw->aq.asq.bal, 0);
> - wr32(hw, hw->aq.asq.bah, 0);
> + wr32(hw, IAVF_VF_ATQH1, 0);
> + wr32(hw, IAVF_VF_ATQT1, 0);
> + wr32(hw, IAVF_VF_ATQLEN1, 0);
> + wr32(hw, IAVF_VF_ATQBAL1, 0);
> + wr32(hw, IAVF_VF_ATQBAH1, 0);
>
> hw->aq.asq.count = 0; /* to indicate uninitialized queue */
>
> @@ -489,11 +468,11 @@ static enum iavf_status iavf_shutdown_arq(struct iavf_hw *hw)
> }
>
> /* Stop firmware AdminQ processing */
> - wr32(hw, hw->aq.arq.head, 0);
> - wr32(hw, hw->aq.arq.tail, 0);
> - wr32(hw, hw->aq.arq.len, 0);
> - wr32(hw, hw->aq.arq.bal, 0);
> - wr32(hw, hw->aq.arq.bah, 0);
> + wr32(hw, IAVF_VF_ARQH1, 0);
> + wr32(hw, IAVF_VF_ARQT1, 0);
> + wr32(hw, IAVF_VF_ARQLEN1, 0);
> + wr32(hw, IAVF_VF_ARQBAL1, 0);
> + wr32(hw, IAVF_VF_ARQBAH1, 0);
>
> hw->aq.arq.count = 0; /* to indicate uninitialized queue */
>
> @@ -529,9 +508,6 @@ enum iavf_status iavf_init_adminq(struct iavf_hw *hw)
> goto init_adminq_exit;
> }
>
> - /* Set up register offsets */
> - iavf_adminq_init_regs(hw);
> -
> /* setup ASQ command write back timeout */
> hw->aq.asq_cmd_timeout = IAVF_ASQ_CMD_TIMEOUT;
>
> @@ -587,9 +563,9 @@ static u16 iavf_clean_asq(struct iavf_hw *hw)
>
> desc = IAVF_ADMINQ_DESC(*asq, ntc);
> details = IAVF_ADMINQ_DETAILS(*asq, ntc);
> - while (rd32(hw, hw->aq.asq.head) != ntc) {
> + while (rd32(hw, IAVF_VF_ATQH1) != ntc) {
> iavf_debug(hw, IAVF_DEBUG_AQ_MESSAGE,
> - "ntc %d head %d.\n", ntc, rd32(hw, hw->aq.asq.head));
> + "ntc %d head %d.\n", ntc, rd32(hw, IAVF_VF_ATQH1));
>
> if (details->callback) {
> IAVF_ADMINQ_CALLBACK cb_func =
> @@ -624,7 +600,7 @@ bool iavf_asq_done(struct iavf_hw *hw)
> /* AQ designers suggest use of head for better
> * timing reliability than DD bit
> */
> - return rd32(hw, hw->aq.asq.head) == hw->aq.asq.next_to_use;
> + return rd32(hw, IAVF_VF_ATQH1) == hw->aq.asq.next_to_use;
> }
>
> /**
> @@ -663,7 +639,7 @@ enum iavf_status iavf_asq_send_command(struct iavf_hw *hw,
>
> hw->aq.asq_last_status = IAVF_AQ_RC_OK;
>
> - val = rd32(hw, hw->aq.asq.head);
> + val = rd32(hw, IAVF_VF_ATQH1);
> if (val >= hw->aq.num_asq_entries) {
> iavf_debug(hw, IAVF_DEBUG_AQ_MESSAGE,
> "AQTX: head overrun at %d\n", val);
> @@ -755,7 +731,7 @@ enum iavf_status iavf_asq_send_command(struct iavf_hw *hw,
> if (hw->aq.asq.next_to_use == hw->aq.asq.count)
> hw->aq.asq.next_to_use = 0;
> if (!details->postpone)
> - wr32(hw, hw->aq.asq.tail, hw->aq.asq.next_to_use);
> + wr32(hw, IAVF_VF_ATQT1, hw->aq.asq.next_to_use);
>
> /* if cmd_details are not defined or async flag is not set,
> * we need to wait for desc write back
> @@ -810,7 +786,7 @@ enum iavf_status iavf_asq_send_command(struct iavf_hw *hw,
> /* update the error if time out occurred */
> if ((!cmd_completed) &&
> (!details->async && !details->postpone)) {
> - if (rd32(hw, hw->aq.asq.len) & IAVF_VF_ATQLEN1_ATQCRIT_MASK) {
> + if (rd32(hw, IAVF_VF_ATQLEN1) & IAVF_VF_ATQLEN1_ATQCRIT_MASK) {
> iavf_debug(hw, IAVF_DEBUG_AQ_MESSAGE,
> "AQTX: AQ Critical error.\n");
> status = IAVF_ERR_ADMIN_QUEUE_CRITICAL_ERROR;
> @@ -878,7 +854,7 @@ enum iavf_status iavf_clean_arq_element(struct iavf_hw *hw,
> }
>
> /* set next_to_use to head */
> - ntu = rd32(hw, hw->aq.arq.head) & IAVF_VF_ARQH1_ARQH_MASK;
> + ntu = rd32(hw, IAVF_VF_ARQH1) & IAVF_VF_ARQH1_ARQH_MASK;
> if (ntu == ntc) {
> /* nothing to do - shouldn't need to update ring's values */
> ret_code = IAVF_ERR_ADMIN_QUEUE_NO_WORK;
> @@ -926,7 +902,7 @@ enum iavf_status iavf_clean_arq_element(struct iavf_hw *hw,
> desc->params.external.addr_low = cpu_to_le32(lower_32_bits(bi->pa));
>
> /* set tail = the last cleaned desc index. */
> - wr32(hw, hw->aq.arq.tail, ntc);
> + wr32(hw, IAVF_VF_ARQT1, ntc);
> /* ntc is updated to tail + 1 */
> ntc++;
> if (ntc == hw->aq.num_arq_entries)
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_adminq.h b/drivers/net/ethernet/intel/iavf/iavf_adminq.h
> index 1f60518eb0e5..406506f64bdd 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_adminq.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_adminq.h
> @@ -29,13 +29,6 @@ struct iavf_adminq_ring {
> /* used for interrupt processing */
> u16 next_to_use;
> u16 next_to_clean;
> -
> - /* used for queue tracking */
> - u32 head;
> - u32 tail;
> - u32 len;
> - u32 bah;
> - u32 bal;
> };
>
> /* ASQ transaction details */
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_common.c b/drivers/net/ethernet/intel/iavf/iavf_common.c
> index 8091e6feca01..89d2bce529ae 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_common.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_common.c
> @@ -279,11 +279,11 @@ void iavf_debug_aq(struct iavf_hw *hw, enum iavf_debug_mask mask, void *desc,
> **/
> bool iavf_check_asq_alive(struct iavf_hw *hw)
> {
> - if (hw->aq.asq.len)
> - return !!(rd32(hw, hw->aq.asq.len) &
> - IAVF_VF_ATQLEN1_ATQENABLE_MASK);
> - else
> + /* Check if the queue is initialized */
> + if (!hw->aq.asq.count)
> return false;
> +
> + return !!(rd32(hw, IAVF_VF_ATQLEN1) & IAVF_VF_ATQLEN1_ATQENABLE_MASK);
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 6e27b7938b8a..146755498feb 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -3253,7 +3253,7 @@ static void iavf_adminq_task(struct work_struct *work)
> goto freedom;
>
> /* check for error indications */
> - val = rd32(hw, hw->aq.arq.len);
> + val = rd32(hw, IAVF_VF_ARQLEN1);
> if (val == 0xdeadbeef || val == 0xffffffff) /* device in reset */
> goto freedom;
> oldval = val;
> @@ -3270,9 +3270,9 @@ static void iavf_adminq_task(struct work_struct *work)
> val &= ~IAVF_VF_ARQLEN1_ARQCRIT_MASK;
> }
> if (oldval != val)
> - wr32(hw, hw->aq.arq.len, val);
> + wr32(hw, IAVF_VF_ARQLEN1, val);
>
> - val = rd32(hw, hw->aq.asq.len);
> + val = rd32(hw, IAVF_VF_ATQLEN1);
> oldval = val;
> if (val & IAVF_VF_ATQLEN1_ATQVFE_MASK) {
> dev_info(&adapter->pdev->dev, "ASQ VF Error detected\n");
> @@ -3287,7 +3287,7 @@ static void iavf_adminq_task(struct work_struct *work)
> val &= ~IAVF_VF_ATQLEN1_ATQCRIT_MASK;
> }
> if (oldval != val)
> - wr32(hw, hw->aq.asq.len, val);
> + wr32(hw, IAVF_VF_ATQLEN1, val);
>
> freedom:
> kfree(event.msg_buf);
Powered by blists - more mailing lists