On 7/2/2020 4:13 AM, Guinan Sun wrote:
> Fix unchecked return value.
> Add cast for type mismatch.
>
> Signed-off-by: Jakub Chylkowski <jakubx.chylkowski@intel.com>
> Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> ---
> drivers/net/ixgbe/base/ixgbe_82599.c | 12 +++++-------
> drivers/net/ixgbe/base/ixgbe_common.c | 6 ++----
> drivers/net/ixgbe/base/ixgbe_common.h | 2 +-
> drivers/net/ixgbe/base/ixgbe_dcb_82598.c | 2 +-
> drivers/net/ixgbe/base/ixgbe_dcb_82599.c | 2 +-
> drivers/net/ixgbe/base/ixgbe_phy.c | 25 +++++++-----------------
> drivers/net/ixgbe/base/ixgbe_x540.c | 2 +-
> drivers/net/ixgbe/base/ixgbe_x550.c | 2 +-
> 8 files changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/ixgbe/base/ixgbe_82599.c b/drivers/net/ixgbe/base/ixgbe_82599.c
> index 193233746..e425f28af 100644
> --- a/drivers/net/ixgbe/base/ixgbe_82599.c
> +++ b/drivers/net/ixgbe/base/ixgbe_82599.c
> @@ -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);
Hi Guinan, Qi,
These are not coding style changes, as commit log says they are fixes in the code.
Can you please properly separate the patch based on the fix type and provide
relevant patch title?
Hi Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, July 8, 2020 11:26 PM
> To: Sun, GuinanX <guinanx.sun@intel.com>; dev@dpdk.org; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: Guo, Jia <jia.guo@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>;
> Chylkowski, JakubX <jakubx.chylkowski@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 12/20] net/ixgbe/base: modify coding style
>
> On 7/2/2020 4:13 AM, Guinan Sun wrote:
> > Fix unchecked return value.
> > Add cast for type mismatch.
> >
> > Signed-off-by: Jakub Chylkowski <jakubx.chylkowski@intel.com>
> > Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> > ---
> > drivers/net/ixgbe/base/ixgbe_82599.c | 12 +++++-------
> > drivers/net/ixgbe/base/ixgbe_common.c | 6 ++----
> > drivers/net/ixgbe/base/ixgbe_common.h | 2 +-
> > drivers/net/ixgbe/base/ixgbe_dcb_82598.c | 2 +-
> > drivers/net/ixgbe/base/ixgbe_dcb_82599.c | 2 +-
> > drivers/net/ixgbe/base/ixgbe_phy.c | 25 +++++++-----------------
> > drivers/net/ixgbe/base/ixgbe_x540.c | 2 +-
> > drivers/net/ixgbe/base/ixgbe_x550.c | 2 +-
> > 8 files changed, 19 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/base/ixgbe_82599.c
> > b/drivers/net/ixgbe/base/ixgbe_82599.c
> > index 193233746..e425f28af 100644
> > --- a/drivers/net/ixgbe/base/ixgbe_82599.c
> > +++ b/drivers/net/ixgbe/base/ixgbe_82599.c
> > @@ -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);
>
> Hi Guinan, Qi,
>
> These are not coding style changes, as commit log says they are fixes in the
> code.
>
> Can you please properly separate the patch based on the fix type and provide
> relevant patch title?
First, we will make changes to the commit log.
The reason is that this is not a fix, but some unnecessary return value check is deleted.
Secondly, we split this patch into two. This will make the expression clearer.
@@ -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,
@@ -159,7 +159,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;