Hi Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, June 22, 2020 8:00 PM
> To: Sun, GuinanX <guinanx.sun@intel.com>; dev@dpdk.org
> Cc: Chylkowski, JakubX <jakubx.chylkowski@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 13/21] net/ixgbe/base: modify Klocwork hits
> for DDK 7.0
>
> On 6/12/2020 4:24 AM, Guinan Sun wrote:
> > Fix issues found by Klocwork for DDK 7.0
>
> This doesn't say much on its own, can you please document the actual issues
> fixed, like "fix unchecked return value" and I think can drop the Klocwork
> reference, and even not asking what DDK is ...
>
We classified the inspection results and divided them into the following two types.
The first one is the return value check.
The second is type conversion.
In addition, regarding this part of the content, we think it is to improve the code style.
If it is a fix, the modified content is more scattered, and it is not easy to collect the fix version.
So, the title is tentatively 'modify coding style'.
> Feel free to split patch into multiple patches if there are multiple types of issues
> have been fixed.
First of all, I agree with you, but for the improvement of coding style, it is not necessary to split into multiple patches,
because this has nothing to do with the function, it is just an improvement point.
If the function is different, it will be split into different patches.
>
> >
> > Signed-off-by: Jakub Chylkowski <jakubx.chylkowski@intel.com>
> > Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
>
> <...>
@@ -1547,7 +1547,7 @@ void ixgbe_fdir_add_signature_filter_82599(struct ixgbe_hw *hw,
* is for FDIRCMD. Then do a 64-bit register write from FDIRHASH.
*/
fdirhashcmd = (u64)fdircmd << 32;
- fdirhashcmd |= ixgbe_atr_compute_sig_hash_82599(input, common);
+ fdirhashcmd |= (u64)ixgbe_atr_compute_sig_hash_82599(input, common);
IXGBE_WRITE_REG64(hw, IXGBE_FDIRHASH, fdirhashcmd);
DEBUGOUT2("Tx Queue=%x hash=%x\n", queue, (u32)fdirhashcmd);
@@ -1636,7 +1636,7 @@ STATIC u32 ixgbe_get_fdirtcpm_82599(union ixgbe_atr_input *input_mask)
{
u32 mask = IXGBE_NTOHS(input_mask->formatted.dst_port);
mask <<= IXGBE_FDIRTCPM_DPORTM_SHIFT;
- mask |= IXGBE_NTOHS(input_mask->formatted.src_port);
+ mask |= (u32)IXGBE_NTOHS(input_mask->formatted.src_port);
mask = ((mask & 0x55555555) << 1) | ((mask & 0xAAAAAAAA) >> 1);
mask = ((mask & 0x33333333) << 2) | ((mask & 0xCCCCCCCC) >> 2);
mask = ((mask & 0x0F0F0F0F) << 4) | ((mask & 0xF0F0F0F0) >> 4);
@@ -1868,14 +1868,14 @@ s32 ixgbe_fdir_write_perfect_filter_82599(struct ixgbe_hw *hw,
/* record source and destination port (little-endian)*/
fdirport = IXGBE_NTOHS(input->formatted.dst_port);
fdirport <<= IXGBE_FDIRPORT_DESTINATION_SHIFT;
- fdirport |= IXGBE_NTOHS(input->formatted.src_port);
+ fdirport |= (u32)IXGBE_NTOHS(input->formatted.src_port);
IXGBE_WRITE_REG(hw, IXGBE_FDIRPORT, fdirport);
}
/* record VLAN (little-endian) and flex_bytes(big-endian) */
fdirvlan = IXGBE_STORE_AS_BE16(input->formatted.flex_bytes);
fdirvlan <<= IXGBE_FDIRVLAN_FLEX_SHIFT;
- fdirvlan |= IXGBE_NTOHS(input->formatted.vlan_id);
+ fdirvlan |= (u32)IXGBE_NTOHS(input->formatted.vlan_id);
IXGBE_WRITE_REG(hw, IXGBE_FDIRVLAN, fdirvlan);
if (cloud_mode) {
@@ -2093,9 +2093,7 @@ s32 ixgbe_start_hw_82599(struct ixgbe_hw *hw)
if (ret_val != IXGBE_SUCCESS)
goto out;
- ret_val = ixgbe_start_hw_gen2(hw);
- if (ret_val != IXGBE_SUCCESS)
- goto out;
+ ixgbe_start_hw_gen2(hw);
/* We need to run link autotry after the driver loads */
hw->mac.autotry_restart = true;
@@ -422,7 +422,7 @@ s32 ixgbe_start_hw_generic(struct ixgbe_hw *hw)
* 82599
* X540
**/
-s32 ixgbe_start_hw_gen2(struct ixgbe_hw *hw)
+void ixgbe_start_hw_gen2(struct ixgbe_hw *hw)
{
u32 i;
u32 regval;
@@ -447,8 +447,6 @@ s32 ixgbe_start_hw_gen2(struct ixgbe_hw *hw)
IXGBE_DCA_RXCTRL_HEAD_WRO_EN);
IXGBE_WRITE_REG(hw, IXGBE_DCA_RXCTRL(i), regval);
}
-
- return IXGBE_SUCCESS;
}
/**
@@ -739,7 +737,7 @@ s32 ixgbe_read_pba_num_generic(struct ixgbe_hw *hw, u32 *pba_num)
DEBUGOUT("NVM Read Error\n");
return ret_val;
}
- *pba_num |= data;
+ *pba_num |= (u32)data;
return IXGBE_SUCCESS;
}
@@ -23,7 +23,7 @@ u16 ixgbe_get_pcie_msix_count_generic(struct ixgbe_hw *hw);
s32 ixgbe_init_ops_generic(struct ixgbe_hw *hw);
s32 ixgbe_init_hw_generic(struct ixgbe_hw *hw);
s32 ixgbe_start_hw_generic(struct ixgbe_hw *hw);
-s32 ixgbe_start_hw_gen2(struct ixgbe_hw *hw);
+void ixgbe_start_hw_gen2(struct ixgbe_hw *hw);
s32 ixgbe_clear_hw_cntrs_generic(struct ixgbe_hw *hw);
s32 ixgbe_read_pba_num_generic(struct ixgbe_hw *hw, u32 *pba_num);
s32 ixgbe_read_pba_string_generic(struct ixgbe_hw *hw, u8 *pba_num,
@@ -167,7 +167,7 @@ s32 ixgbe_dcb_config_tx_desc_arbiter_82598(struct ixgbe_hw *hw,
for (i = 0; i < IXGBE_DCB_MAX_TRAFFIC_CLASS; i++) {
max_credits = max[i];
reg = max_credits << IXGBE_TDTQ2TCCR_MCL_SHIFT;
- reg |= refill[i];
+ reg |= (u32)(refill[i]);
reg |= (u32)(bwg_id[i]) << IXGBE_TDTQ2TCCR_BWG_SHIFT;
if (tsa[i] == ixgbe_dcb_tsa_group_strict_cee)
@@ -166,7 +166,7 @@ s32 ixgbe_dcb_config_tx_desc_arbiter_82599(struct ixgbe_hw *hw, u16 *refill,
for (i = 0; i < IXGBE_DCB_MAX_TRAFFIC_CLASS; i++) {
max_credits = max[i];
reg = max_credits << IXGBE_RTTDT2C_MCL_SHIFT;
- reg |= refill[i];
+ reg |= (u32)(refill[i]);
reg |= (u32)(bwg_id[i]) << IXGBE_RTTDT2C_BWG_SHIFT;
if (tsa[i] == ixgbe_dcb_tsa_group_strict_cee)
@@ -8,10 +8,10 @@
STATIC void ixgbe_i2c_start(struct ixgbe_hw *hw);
STATIC void ixgbe_i2c_stop(struct ixgbe_hw *hw);
-STATIC s32 ixgbe_clock_in_i2c_byte(struct ixgbe_hw *hw, u8 *data);
+STATIC void ixgbe_clock_in_i2c_byte(struct ixgbe_hw *hw, u8 *data);
STATIC s32 ixgbe_clock_out_i2c_byte(struct ixgbe_hw *hw, u8 data);
STATIC s32 ixgbe_get_i2c_ack(struct ixgbe_hw *hw);
-STATIC s32 ixgbe_clock_in_i2c_bit(struct ixgbe_hw *hw, bool *data);
+STATIC void ixgbe_clock_in_i2c_bit(struct ixgbe_hw *hw, bool *data);
STATIC s32 ixgbe_clock_out_i2c_bit(struct ixgbe_hw *hw, bool data);
STATIC void ixgbe_raise_i2c_clk(struct ixgbe_hw *hw, u32 *i2cctl);
STATIC void ixgbe_lower_i2c_clk(struct ixgbe_hw *hw, u32 *i2cctl);
@@ -46,11 +46,7 @@ STATIC s32 ixgbe_out_i2c_byte_ack(struct ixgbe_hw *hw, u8 byte)
*/
STATIC s32 ixgbe_in_i2c_byte_ack(struct ixgbe_hw *hw, u8 *byte)
{
- s32 status;
-
- status = ixgbe_clock_in_i2c_byte(hw, byte);
- if (status)
- return status;
+ ixgbe_clock_in_i2c_byte(hw, byte);
/* ACK */
return ixgbe_clock_out_i2c_bit(hw, false);
}
@@ -123,8 +119,7 @@ s32 ixgbe_read_i2c_combined_generic_int(struct ixgbe_hw *hw, u8 addr, u16 reg,
if (ixgbe_in_i2c_byte_ack(hw, &low_bits))
goto fail;
/* Get csum */
- if (ixgbe_clock_in_i2c_byte(hw, &csum_byte))
- goto fail;
+ ixgbe_clock_in_i2c_byte(hw, &csum_byte);
/* NACK */
if (ixgbe_clock_out_i2c_bit(hw, false))
goto fail;
@@ -2034,9 +2029,7 @@ STATIC s32 ixgbe_read_i2c_byte_generic_int(struct ixgbe_hw *hw, u8 byte_offset,
if (status != IXGBE_SUCCESS)
goto fail;
- status = ixgbe_clock_in_i2c_byte(hw, data);
- if (status != IXGBE_SUCCESS)
- goto fail;
+ ixgbe_clock_in_i2c_byte(hw, data);
status = ixgbe_clock_out_i2c_bit(hw, nack);
if (status != IXGBE_SUCCESS)
@@ -2281,7 +2274,7 @@ STATIC void ixgbe_i2c_stop(struct ixgbe_hw *hw)
*
* Clocks in one byte data via I2C data/clock
**/
-STATIC s32 ixgbe_clock_in_i2c_byte(struct ixgbe_hw *hw, u8 *data)
+STATIC void ixgbe_clock_in_i2c_byte(struct ixgbe_hw *hw, u8 *data)
{
s32 i;
bool bit = 0;
@@ -2293,8 +2286,6 @@ STATIC s32 ixgbe_clock_in_i2c_byte(struct ixgbe_hw *hw, u8 *data)
ixgbe_clock_in_i2c_bit(hw, &bit);
*data |= bit << i;
}
-
- return IXGBE_SUCCESS;
}
/**
@@ -2390,7 +2381,7 @@ STATIC s32 ixgbe_get_i2c_ack(struct ixgbe_hw *hw)
*
* Clocks in one bit via I2C data/clock
**/
-STATIC s32 ixgbe_clock_in_i2c_bit(struct ixgbe_hw *hw, bool *data)
+STATIC void ixgbe_clock_in_i2c_bit(struct ixgbe_hw *hw, bool *data)
{
u32 i2cctl = IXGBE_READ_REG(hw, IXGBE_I2CCTL_BY_MAC(hw));
u32 data_oe_bit = IXGBE_I2C_DATA_OE_N_EN_BY_MAC(hw);
@@ -2415,8 +2406,6 @@ STATIC s32 ixgbe_clock_in_i2c_bit(struct ixgbe_hw *hw, bool *data)
/* Minimum low period of clock is 4.7 us */
usec_delay(IXGBE_I2C_T_LOW);
-
- return IXGBE_SUCCESS;
}
/**
@@ -288,7 +288,7 @@ s32 ixgbe_start_hw_X540(struct ixgbe_hw *hw)
if (ret_val != IXGBE_SUCCESS)
goto out;
- ret_val = ixgbe_start_hw_gen2(hw);
+ ixgbe_start_hw_gen2(hw);
out:
return ret_val;
@@ -699,7 +699,7 @@ static s32 ixgbe_setup_fw_link(struct ixgbe_hw *hw)
for (i = 0; i < sizeof(ixgbe_fw_map) / sizeof(ixgbe_fw_map[0]); ++i) {
if (hw->phy.autoneg_advertised & ixgbe_fw_map[i].phy_speed)
- setup[0] |= ixgbe_fw_map[i].fw_speed;
+ setup[0] |= (u32)(ixgbe_fw_map[i].fw_speed);
}
setup[0] |= FW_PHY_ACT_SETUP_LINK_HP | FW_PHY_ACT_SETUP_LINK_AN;