[dpdk-dev] [PATCH 35/39] net/ixgbe/base: hold semaphore for shadow RAM access
Wang, Xiao W
xiao.w.wang at intel.com
Fri Sep 23 10:06:25 CEST 2016
Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 20, 2016 1:08 AM
> To: Wang, Xiao W <xiao.w.wang at intel.com>; Lu, Wenzhuo
> <wenzhuo.lu at intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 35/39] net/ixgbe/base: hold semaphore for
> shadow RAM access
>
> 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?
>
Yes, will merge them 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