[dpdk-dev] [PATCH 35/39] net/ixgbe/base: hold semaphore for shadow RAM access
Ferruh Yigit
ferruh.yigit at intel.com
Mon Sep 19 19:08:27 CEST 2016
On 8/27/2016 4:48 PM, Xiao Wang wrote:
> The semaphore is not being held for complete shadow RAM accesses
> which could result in corruption. Refactor the code so that it is
> possible to hold the semaphore around ixgbe_host_interface_command
> by introducing an unlocked form. This patch also eliminates the
> function ixgbe_read_ee_hostif_data_X550 in favor of the function
> ixgbe_read_ee_hostif_X550 and it now gets both semaphore bits
> at once instead of nesting them. The new arrangement is able to
> get both the management interface and the EEPROM semaphores at the
> same time instead of separately.
>
> Signed-off-by: Xiao Wang <xiao.w.wang at intel.com>
> ---
> drivers/net/ixgbe/base/ixgbe_common.c | 106 ++++++++++++++++++++++------------
> drivers/net/ixgbe/base/ixgbe_common.h | 3 +-
> drivers/net/ixgbe/base/ixgbe_x550.c | 57 +++++-------------
> drivers/net/ixgbe/base/ixgbe_x550.h | 2 -
> 4 files changed, 88 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/net/ixgbe/base/ixgbe_common.c b/drivers/net/ixgbe/base/ixgbe_common.c
> index 9776ab9..d31fb81 100644
> --- a/drivers/net/ixgbe/base/ixgbe_common.c
> +++ b/drivers/net/ixgbe/base/ixgbe_common.c
> @@ -1066,7 +1066,7 @@ void ixgbe_set_lan_id_multi_port_pcie(struct ixgbe_hw *hw)
> if (hw->device_id == IXGBE_DEV_ID_X550EM_A_SFP) {
> hw->eeprom.ops.read(hw, IXGBE_EEPROM_CTRL_4, &ee_ctrl_4);
> bus->instance_id = (ee_ctrl_4 & IXGBE_EE_CTRL_4_INST_ID) >>
> - IXGBE_EE_CTRL_4_INST_ID_SHIFT;
> + IXGBE_EE_CTRL_4_INST_ID_SHIFT;
> }
> }
>
> @@ -2877,7 +2877,7 @@ out:
> * advertised settings
> **/
> s32 ixgbe_negotiate_fc(struct ixgbe_hw *hw, u32 adv_reg, u32 lp_reg,
> - u32 adv_sym, u32 adv_asm, u32 lp_sym, u32 lp_asm)
> + u32 adv_sym, u32 adv_asm, u32 lp_sym, u32 lp_asm)
> {
> if ((!(adv_reg)) || (!(lp_reg))) {
> ERROR_REPORT3(IXGBE_ERROR_UNSUPPORTED,
> @@ -3920,7 +3920,8 @@ s32 ixgbe_set_vfta_generic(struct ixgbe_hw *hw, u32 vlan, u32 vind,
> vfta_delta = 1 << (vlan % 32);
> vfta = IXGBE_READ_REG(hw, IXGBE_VFTA(regidx));
>
> - /* vfta_delta represents the difference between the current value
> + /*
> + * vfta_delta represents the difference between the current value
These whitespace fixes not belong to this patch.
It is good to make cleanups, but these are making noise for real patch,
there are a few more of these in previous patches, perhaps all can be
merged into a cleanup patch?
> * of vfta and the value we want in the register. Since the diff
> * is an XOR mask we can just update the vfta using an XOR
> */
> @@ -3953,7 +3954,7 @@ vfta_update:
> * @vind: VMDq output index that maps queue to VLAN id in VLVFB
> * @vlan_on: boolean flag to turn on/off VLAN in VLVF
> * @vfta_delta: pointer to the difference between the current value of VFTA
> - * and the desired value
> + * and the desired value
> * @vfta: the desired value of the VFTA
> * @vlvf_bypass: boolean flag indicating updating default pool is okay
> *
> @@ -3980,6 +3981,7 @@ s32 ixgbe_set_vlvf_generic(struct ixgbe_hw *hw, u32 vlan, u32 vind,
> */
> if (!(IXGBE_READ_REG(hw, IXGBE_VT_CTL) & IXGBE_VT_CTL_VT_ENABLE))
> return IXGBE_SUCCESS;
> +
> vlvf_index = ixgbe_find_vlvf_slot(hw, vlan, vlvf_bypass);
> if (vlvf_index < 0)
> return vlvf_index;
> @@ -4009,6 +4011,7 @@ s32 ixgbe_set_vlvf_generic(struct ixgbe_hw *hw, u32 vlan, u32 vind,
>
> return IXGBE_SUCCESS;
> }
> +
> /* If there are still bits set in the VLVFB registers
> * for the VLAN ID indicated we need to see if the
> * caller is requesting that we clear the VFTA entry bit.
> @@ -4413,43 +4416,31 @@ u8 ixgbe_calculate_checksum(u8 *buffer, u32 length)
> }
>
> /**
> - * ixgbe_host_interface_command - Issue command to manageability block
> + * ixgbe_hic_unlocked - Issue command to manageability block unlocked
> * @hw: pointer to the HW structure
> - * @buffer: contains the command to write and where the return status will
> - * be placed
> + * @buffer: command to write and where the return status will be placed
> * @length: length of buffer, must be multiple of 4 bytes
> * @timeout: time in ms to wait for command completion
> - * @return_data: read and return data from the buffer (true) or not (false)
> - * Needed because FW structures are big endian and decoding of
> - * these fields can be 8 bit or 16 bit based on command. Decoding
> - * is not easily understood without making a table of commands.
> - * So we will leave this up to the caller to read back the data
> - * in these cases.
> *
> * Communicates with the manageability block. On success return IXGBE_SUCCESS
> * else returns semaphore error when encountering an error acquiring
> * semaphore or IXGBE_ERR_HOST_INTERFACE_COMMAND when command fails.
> + *
> + * This function assumes that the IXGBE_GSSR_SW_MNG_SM semaphore is held
> + * by the caller.
> **/
> -s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, u32 *buffer,
> - u32 length, u32 timeout, bool return_data)
> +s32 ixgbe_hic_unlocked(struct ixgbe_hw *hw, u32 *buffer, u32 length,
> + u32 timeout)
> {
> - u32 hicr, i, bi, fwsts;
> - u32 hdr_size = sizeof(struct ixgbe_hic_hdr);
> - u16 buf_len;
> + u32 hicr, i, fwsts;
> u16 dword_len;
> - s32 status;
>
> - DEBUGFUNC("ixgbe_host_interface_command");
> + DEBUGFUNC("ixgbe_hic_unlocked");
>
> - if (length == 0 || length > IXGBE_HI_MAX_BLOCK_BYTE_LENGTH) {
> + if (!length || length > IXGBE_HI_MAX_BLOCK_BYTE_LENGTH) {
> DEBUGOUT1("Buffer length failure buffersize=%d.\n", length);
> return IXGBE_ERR_HOST_INTERFACE_COMMAND;
> }
> - /* Take management host interface semaphore */
> - status = hw->mac.ops.acquire_swfw_sync(hw, IXGBE_GSSR_SW_MNG_SM);
> -
> - if (status)
> - return status;
>
> /* Set bit 9 of FWSTS clearing FW reset indication */
> fwsts = IXGBE_READ_REG(hw, IXGBE_FWSTS);
> @@ -4457,17 +4448,15 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, u32 *buffer,
>
> /* Check that the host interface is enabled. */
> hicr = IXGBE_READ_REG(hw, IXGBE_HICR);
> - if ((hicr & IXGBE_HICR_EN) == 0) {
> + if (!(hicr & IXGBE_HICR_EN)) {
> DEBUGOUT("IXGBE_HOST_EN bit disabled.\n");
> - status = IXGBE_ERR_HOST_INTERFACE_COMMAND;
> - goto rel_out;
> + return IXGBE_ERR_HOST_INTERFACE_COMMAND;
> }
>
> /* Calculate length in DWORDs. We must be DWORD aligned */
> - if ((length % (sizeof(u32))) != 0) {
> + if (length % sizeof(u32)) {
> DEBUGOUT("Buffer length failure, not aligned to dword");
> - status = IXGBE_ERR_INVALID_ARGUMENT;
> - goto rel_out;
> + return IXGBE_ERR_INVALID_ARGUMENT;
> }
>
> dword_len = length >> 2;
> @@ -4490,14 +4479,59 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, u32 *buffer,
> }
>
> /* Check command completion */
> - if ((timeout != 0 && i == timeout) ||
> + if ((timeout && i == timeout) ||
> !(IXGBE_READ_REG(hw, IXGBE_HICR) & IXGBE_HICR_SV)) {
> ERROR_REPORT1(IXGBE_ERROR_CAUTION,
> "Command has failed with no status valid.\n");
> - status = IXGBE_ERR_HOST_INTERFACE_COMMAND;
> - goto rel_out;
> + return IXGBE_ERR_HOST_INTERFACE_COMMAND;
> + }
> +
> + return IXGBE_SUCCESS;
> +}
> +
> +/**
> + * ixgbe_host_interface_command - Issue command to manageability block
> + * @hw: pointer to the HW structure
> + * @buffer: contains the command to write and where the return status will
> + * be placed
> + * @length: length of buffer, must be multiple of 4 bytes
> + * @timeout: time in ms to wait for command completion
> + * @return_data: read and return data from the buffer (true) or not (false)
> + * Needed because FW structures are big endian and decoding of
> + * these fields can be 8 bit or 16 bit based on command. Decoding
> + * is not easily understood without making a table of commands.
> + * So we will leave this up to the caller to read back the data
> + * in these cases.
> + *
> + * Communicates with the manageability block. On success return IXGBE_SUCCESS
> + * else returns semaphore error when encountering an error acquiring
> + * semaphore or IXGBE_ERR_HOST_INTERFACE_COMMAND when command fails.
> + **/
> +s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, u32 *buffer,
> + u32 length, u32 timeout, bool return_data)
> +{
> + u32 hdr_size = sizeof(struct ixgbe_hic_hdr);
> + u16 dword_len;
> + u16 buf_len;
> + s32 status;
> + u32 bi;
> +
> + DEBUGFUNC("ixgbe_host_interface_command");
> +
> + if (length == 0 || length > IXGBE_HI_MAX_BLOCK_BYTE_LENGTH) {
> + DEBUGOUT1("Buffer length failure buffersize=%d.\n", length);
> + return IXGBE_ERR_HOST_INTERFACE_COMMAND;
> }
>
> + /* Take management host interface semaphore */
> + status = hw->mac.ops.acquire_swfw_sync(hw, IXGBE_GSSR_SW_MNG_SM);
> + if (status)
> + return status;
> +
> + status = ixgbe_hic_unlocked(hw, buffer, length, timeout);
> + if (status)
> + goto rel_out;
> +
> if (!return_data)
> goto rel_out;
>
> @@ -4512,7 +4546,7 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, u32 *buffer,
>
> /* If there is any thing in data position pull it in */
> buf_len = ((struct ixgbe_hic_hdr *)buffer)->buf_len;
> - if (buf_len == 0)
> + if (!buf_len)
> goto rel_out;
>
> if (length < buf_len + hdr_size) {
> diff --git a/drivers/net/ixgbe/base/ixgbe_common.h b/drivers/net/ixgbe/base/ixgbe_common.h
> index 0545f85..66dd565 100644
> --- a/drivers/net/ixgbe/base/ixgbe_common.h
> +++ b/drivers/net/ixgbe/base/ixgbe_common.h
> @@ -133,7 +133,7 @@ s32 ixgbe_clear_vmdq_generic(struct ixgbe_hw *hw, u32 rar, u32 vmdq);
> s32 ixgbe_insert_mac_addr_generic(struct ixgbe_hw *hw, u8 *addr, u32 vmdq);
> s32 ixgbe_init_uta_tables_generic(struct ixgbe_hw *hw);
> s32 ixgbe_set_vfta_generic(struct ixgbe_hw *hw, u32 vlan,
> - u32 vind, bool vlan_on, bool vlvf_bypass);
> + u32 vind, bool vlan_on, bool vlvf_bypass);
> s32 ixgbe_set_vlvf_generic(struct ixgbe_hw *hw, u32 vlan, u32 vind,
> bool vlan_on, u32 *vfta_delta, u32 vfta,
> bool vlvf_bypass);
> @@ -159,6 +159,7 @@ s32 ixgbe_set_fw_drv_ver_generic(struct ixgbe_hw *hw, u8 maj, u8 min,
> u8 ixgbe_calculate_checksum(u8 *buffer, u32 length);
> s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, u32 *buffer,
> u32 length, u32 timeout, bool return_data);
> +s32 ixgbe_hic_unlocked(struct ixgbe_hw *, u32 *buffer, u32 length, u32 timeout);
>
> void ixgbe_clear_tx_pending(struct ixgbe_hw *hw);
>
> diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c b/drivers/net/ixgbe/base/ixgbe_x550.c
> index 0cc7a3f..6f4dfd9 100644
> --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> @@ -3192,13 +3192,13 @@ s32 ixgbe_setup_phy_loopback_x550em(struct ixgbe_hw *hw)
> *
> * Reads a 16 bit word from the EEPROM using the hostif.
> **/
> -s32 ixgbe_read_ee_hostif_data_X550(struct ixgbe_hw *hw, u16 offset,
> - u16 *data)
> +s32 ixgbe_read_ee_hostif_X550(struct ixgbe_hw *hw, u16 offset, u16 *data)
> {
> - s32 status;
> + const u32 mask = IXGBE_GSSR_SW_MNG_SM | IXGBE_GSSR_EEP_SM;
> struct ixgbe_hic_read_shadow_ram buffer;
> + s32 status;
>
> - DEBUGFUNC("ixgbe_read_ee_hostif_data_X550");
> + DEBUGFUNC("ixgbe_read_ee_hostif_X550");
> buffer.hdr.req.cmd = FW_READ_SHADOW_RAM_CMD;
> buffer.hdr.req.buf_lenh = 0;
> buffer.hdr.req.buf_lenl = FW_READ_SHADOW_RAM_LEN;
> @@ -3209,42 +3209,18 @@ s32 ixgbe_read_ee_hostif_data_X550(struct ixgbe_hw *hw, u16 offset,
> /* one word */
> buffer.length = IXGBE_CPU_TO_BE16(sizeof(u16));
>
> - status = ixgbe_host_interface_command(hw, (u32 *)&buffer,
> - sizeof(buffer),
> - IXGBE_HI_COMMAND_TIMEOUT, false);
> -
> + status = hw->mac.ops.acquire_swfw_sync(hw, mask);
> if (status)
> return status;
>
> - *data = (u16)IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG,
> - FW_NVM_DATA_OFFSET);
> -
> - return 0;
> -}
> -
> -/**
> - * ixgbe_read_ee_hostif_X550 - Read EEPROM word using a host interface command
> - * @hw: pointer to hardware structure
> - * @offset: offset of word in the EEPROM to read
> - * @data: word read from the EEPROM
> - *
> - * Reads a 16 bit word from the EEPROM using the hostif.
> - **/
> -s32 ixgbe_read_ee_hostif_X550(struct ixgbe_hw *hw, u16 offset,
> - u16 *data)
> -{
> - s32 status = IXGBE_SUCCESS;
> -
> - DEBUGFUNC("ixgbe_read_ee_hostif_X550");
> -
> - if (hw->mac.ops.acquire_swfw_sync(hw, IXGBE_GSSR_EEP_SM) ==
> - IXGBE_SUCCESS) {
> - status = ixgbe_read_ee_hostif_data_X550(hw, offset, data);
> - hw->mac.ops.release_swfw_sync(hw, IXGBE_GSSR_EEP_SM);
> - } else {
> - status = IXGBE_ERR_SWFW_SYNC;
> + status = ixgbe_hic_unlocked(hw, (u32 *)&buffer, sizeof(buffer),
> + IXGBE_HI_COMMAND_TIMEOUT);
> + if (!status) {
> + *data = (u16)IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG,
> + FW_NVM_DATA_OFFSET);
> }
>
> + hw->mac.ops.release_swfw_sync(hw, mask);
> return status;
> }
>
> @@ -3260,6 +3236,7 @@ s32 ixgbe_read_ee_hostif_X550(struct ixgbe_hw *hw, u16 offset,
> s32 ixgbe_read_ee_hostif_buffer_X550(struct ixgbe_hw *hw,
> u16 offset, u16 words, u16 *data)
> {
> + const u32 mask = IXGBE_GSSR_SW_MNG_SM | IXGBE_GSSR_EEP_SM;
> struct ixgbe_hic_read_shadow_ram buffer;
> u32 current_word = 0;
> u16 words_to_read;
> @@ -3269,7 +3246,7 @@ s32 ixgbe_read_ee_hostif_buffer_X550(struct ixgbe_hw *hw,
> DEBUGFUNC("ixgbe_read_ee_hostif_buffer_X550");
>
> /* Take semaphore for the entire operation. */
> - status = hw->mac.ops.acquire_swfw_sync(hw, IXGBE_GSSR_EEP_SM);
> + status = hw->mac.ops.acquire_swfw_sync(hw, mask);
> if (status) {
> DEBUGOUT("EEPROM read buffer - semaphore failed\n");
> return status;
> @@ -3289,10 +3266,8 @@ s32 ixgbe_read_ee_hostif_buffer_X550(struct ixgbe_hw *hw,
> buffer.address = IXGBE_CPU_TO_BE32((offset + current_word) * 2);
> buffer.length = IXGBE_CPU_TO_BE16(words_to_read * 2);
>
> - status = ixgbe_host_interface_command(hw, (u32 *)&buffer,
> - sizeof(buffer),
> - IXGBE_HI_COMMAND_TIMEOUT,
> - false);
> + status = ixgbe_hic_unlocked(hw, (u32 *)&buffer, sizeof(buffer),
> + IXGBE_HI_COMMAND_TIMEOUT);
>
> if (status) {
> DEBUGOUT("Host interface command failed\n");
> @@ -3317,7 +3292,7 @@ s32 ixgbe_read_ee_hostif_buffer_X550(struct ixgbe_hw *hw,
> }
>
> out:
> - hw->mac.ops.release_swfw_sync(hw, IXGBE_GSSR_EEP_SM);
> + hw->mac.ops.release_swfw_sync(hw, mask);
> return status;
> }
>
> diff --git a/drivers/net/ixgbe/base/ixgbe_x550.h b/drivers/net/ixgbe/base/ixgbe_x550.h
> index c7253f0..36b36f6 100644
> --- a/drivers/net/ixgbe/base/ixgbe_x550.h
> +++ b/drivers/net/ixgbe/base/ixgbe_x550.h
> @@ -55,8 +55,6 @@ s32 ixgbe_read_ee_hostif_buffer_X550(struct ixgbe_hw *hw,
> u16 offset, u16 words, u16 *data);
> s32 ixgbe_read_ee_hostif_X550(struct ixgbe_hw *hw, u16 offset,
> u16 *data);
> -s32 ixgbe_read_ee_hostif_data_X550(struct ixgbe_hw *hw, u16 offset,
> - u16 *data);
> s32 ixgbe_write_ee_hostif_data_X550(struct ixgbe_hw *hw, u16 offset,
> u16 data);
> s32 ixgbe_set_eee_X550(struct ixgbe_hw *hw, bool enable_eee);
>
More information about the dev
mailing list