diff mbox series

[11/21] net/ixgbe/base: modify loop accounting for retries

Message ID 20200612032410.20864-12-guinanx.sun@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers show
Series update ixgbe base code | expand

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Guinan Sun June 12, 2020, 3:24 a.m. UTC
The condition for comparing retry against max_retry was flawed in the
do-while loops.  For the case where retry was initialized to 0 and
max_retry was initialized to 1, we'd break out of the loop at the
condition when the intent is to retry the code at least once.
Otherwise, the loop is unnecessary.  The other places have a larger
max_retry so code would get run multiple times (if necessary), but not
to the intended extent.

Signed-off-by: Jeb Cramer <jeb.j.cramer@intel.com>
Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
---
 drivers/net/ixgbe/base/ixgbe_phy.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Ferruh Yigit June 22, 2020, 11:59 a.m. UTC | #1
On 6/12/2020 4:24 AM, Guinan Sun wrote:
> The condition for comparing retry against max_retry was flawed in the
> do-while loops.  For the case where retry was initialized to 0 and
> max_retry was initialized to 1, we'd break out of the loop at the
> condition when the intent is to retry the code at least once.
> Otherwise, the loop is unnecessary.  The other places have a larger
> max_retry so code would get run multiple times (if necessary), but not
> to the intended extent.
> 
> Signed-off-by: Jeb Cramer <jeb.j.cramer@intel.com>
> Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> ---
>  drivers/net/ixgbe/base/ixgbe_phy.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
> index 9bb24f1ef..823cf161e 100644
> --- a/drivers/net/ixgbe/base/ixgbe_phy.c
> +++ b/drivers/net/ixgbe/base/ixgbe_phy.c
> @@ -143,7 +143,7 @@ s32 ixgbe_read_i2c_combined_generic_int(struct ixgbe_hw *hw, u8 addr, u16 reg,
>  		else
>  			DEBUGOUT("I2C byte read combined error.\n");
>  		retry++;
> -	} while (retry < max_retry);
> +	} while (retry <= max_retry);
>  
>  	return IXGBE_ERR_I2C;

Ahh, previous patch becomes correct with this change, can you please combine
them? No need to break first and fix later.
Guinan Sun June 30, 2020, 2:52 a.m. UTC | #2
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: Cramer, Jeb J <jeb.j.cramer@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 11/21] net/ixgbe/base: modify loop accounting
> for retries
> 
> On 6/12/2020 4:24 AM, Guinan Sun wrote:
> > The condition for comparing retry against max_retry was flawed in the
> > do-while loops.  For the case where retry was initialized to 0 and
> > max_retry was initialized to 1, we'd break out of the loop at the
> > condition when the intent is to retry the code at least once.
> > Otherwise, the loop is unnecessary.  The other places have a larger
> > max_retry so code would get run multiple times (if necessary), but not
> > to the intended extent.
> >
> > Signed-off-by: Jeb Cramer <jeb.j.cramer@intel.com>
> > Signed-off-by: Guinan Sun <guinanx.sun@intel.com>
> > ---
> >  drivers/net/ixgbe/base/ixgbe_phy.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c
> > b/drivers/net/ixgbe/base/ixgbe_phy.c
> > index 9bb24f1ef..823cf161e 100644
> > --- a/drivers/net/ixgbe/base/ixgbe_phy.c
> > +++ b/drivers/net/ixgbe/base/ixgbe_phy.c
> > @@ -143,7 +143,7 @@ s32 ixgbe_read_i2c_combined_generic_int(struct
> ixgbe_hw *hw, u8 addr, u16 reg,
> >  		else
> >  			DEBUGOUT("I2C byte read combined error.\n");
> >  		retry++;
> > -	} while (retry < max_retry);
> > +	} while (retry <= max_retry);
> >
> >  	return IXGBE_ERR_I2C;
> 
> Ahh, previous patch becomes correct with this change, can you please combine
> them? No need to break first and fix later.

Patch V2 will fix it.
diff mbox series

Patch

diff --git a/drivers/net/ixgbe/base/ixgbe_phy.c b/drivers/net/ixgbe/base/ixgbe_phy.c
index 9bb24f1ef..823cf161e 100644
--- a/drivers/net/ixgbe/base/ixgbe_phy.c
+++ b/drivers/net/ixgbe/base/ixgbe_phy.c
@@ -143,7 +143,7 @@  s32 ixgbe_read_i2c_combined_generic_int(struct ixgbe_hw *hw, u8 addr, u16 reg,
 		else
 			DEBUGOUT("I2C byte read combined error.\n");
 		retry++;
-	} while (retry < max_retry);
+	} while (retry <= max_retry);
 
 	return IXGBE_ERR_I2C;
 }
@@ -208,7 +208,7 @@  s32 ixgbe_write_i2c_combined_generic_int(struct ixgbe_hw *hw, u8 addr, u16 reg,
 		else
 			DEBUGOUT("I2C byte write combined error.\n");
 		retry++;
-	} while (retry < max_retry);
+	} while (retry <= max_retry);
 
 	return IXGBE_ERR_I2C;
 }
@@ -2062,7 +2062,7 @@  STATIC s32 ixgbe_read_i2c_byte_generic_int(struct ixgbe_hw *hw, u8 byte_offset,
 		else
 			DEBUGOUT("I2C byte read error.\n");
 		retry++;
-	} while (retry < max_retry);
+	} while (retry <= max_retry);
 
 	return status;
 }
@@ -2165,7 +2165,7 @@  STATIC s32 ixgbe_write_i2c_byte_generic_int(struct ixgbe_hw *hw, u8 byte_offset,
 		else
 			DEBUGOUT("I2C byte write error.\n");
 		retry++;
-	} while (retry < max_retry);
+	} while (retry <= max_retry);
 
 	if (lock)
 		hw->mac.ops.release_swfw_sync(hw, swfw_mask);