| Message ID | 20221221195900.23276-1-hhhawa@amazon.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/1] i2c: designware: use casting of u64 in clock multiplication to avoid overflow | expand |
On Wed, Dec 21, 2022 at 07:59:00PM +0000, Hanna Hawa wrote: > From: Lareine Khawaly <lareine@amazon.com> > > In functions i2c_dw_scl_lcnt() and i2c_dw_scl_hcnt() may have overflow > by depending on the values of the given parameters including the ic_clk. > For example in our use case where ic_clk is larger than one million, > multiplication of ic_clk * 4700 will result in 32 bit overflow. > > Add cast of u64 to the calculation to avoid multiplication overflow, and > use the corresponding define for divide. Perfect, thank you! Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Fixes: 2373f6b9744d ("i2c-designware: split of i2c-designware.c into core and bus specific parts") > Signed-off-by: Lareine Khawaly <lareine@amazon.com> > Signed-off-by: Hanna Hawa <hhhawa@amazon.com> > --- > Change log v4->v5: > - Fix indentation > > Change Log v3->v4: > - update line length when possible > - fix change log location in the patch > > Change Log v2->v3: > - Avoid changing the ic_clk parameter to u64, and do casting in the > calculation itself instead. > - i2c_dw_clk_rate() returns unsigned long which is confusing because the > function return the value of get_clk_rate_khz() which returns u32. > This is not effect the overflow issue, pushed change in separated > patch. > - use DIV_ROUND_CLOSEST_ULL instead of DIV_ROUND_CLOSEST > > Change Log v1->v2: > - Update commit message and add fix tag. > > drivers/i2c/busses/i2c-designware-common.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c > index e0a46dfd1c15..6fdb25a5f801 100644 > --- a/drivers/i2c/busses/i2c-designware-common.c > +++ b/drivers/i2c/busses/i2c-designware-common.c > @@ -351,7 +351,8 @@ u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset) > * > * If your hardware is free from tHD;STA issue, try this one. > */ > - return DIV_ROUND_CLOSEST(ic_clk * tSYMBOL, MICRO) - 8 + offset; > + return DIV_ROUND_CLOSEST_ULL((u64)ic_clk * tSYMBOL, MICRO) - > + 8 + offset; > else > /* > * Conditional expression: > @@ -367,7 +368,8 @@ u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset) > * The reason why we need to take into account "tf" here, > * is the same as described in i2c_dw_scl_lcnt(). > */ > - return DIV_ROUND_CLOSEST(ic_clk * (tSYMBOL + tf), MICRO) - 3 + offset; > + return DIV_ROUND_CLOSEST_ULL((u64)ic_clk * (tSYMBOL + tf), MICRO) - > + 3 + offset; > } > > u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) > @@ -383,7 +385,8 @@ u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) > * account the fall time of SCL signal (tf). Default tf value > * should be 0.3 us, for safety. > */ > - return DIV_ROUND_CLOSEST(ic_clk * (tLOW + tf), MICRO) - 1 + offset; > + return DIV_ROUND_CLOSEST_ULL((u64)ic_clk * (tLOW + tf), MICRO) - > + 1 + offset; > } > > int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev) > -- > 2.38.1 >
On 12/21/22 22:07, Andy Shevchenko wrote: > On Wed, Dec 21, 2022 at 07:59:00PM +0000, Hanna Hawa wrote: >> From: Lareine Khawaly <lareine@amazon.com> >> >> In functions i2c_dw_scl_lcnt() and i2c_dw_scl_hcnt() may have overflow >> by depending on the values of the given parameters including the ic_clk. >> For example in our use case where ic_clk is larger than one million, >> multiplication of ic_clk * 4700 will result in 32 bit overflow. >> >> Add cast of u64 to the calculation to avoid multiplication overflow, and >> use the corresponding define for divide. > > Perfect, thank you! > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Wed, Dec 21, 2022 at 07:59:00PM +0000, Hanna Hawa wrote: > From: Lareine Khawaly <lareine@amazon.com> > > In functions i2c_dw_scl_lcnt() and i2c_dw_scl_hcnt() may have overflow > by depending on the values of the given parameters including the ic_clk. > For example in our use case where ic_clk is larger than one million, > multiplication of ic_clk * 4700 will result in 32 bit overflow. > > Add cast of u64 to the calculation to avoid multiplication overflow, and > use the corresponding define for divide. > > Fixes: 2373f6b9744d ("i2c-designware: split of i2c-designware.c into core and bus specific parts") > Signed-off-by: Lareine Khawaly <lareine@amazon.com> > Signed-off-by: Hanna Hawa <hhhawa@amazon.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index e0a46dfd1c15..6fdb25a5f801 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -351,7 +351,8 @@ u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset) * * If your hardware is free from tHD;STA issue, try this one. */ - return DIV_ROUND_CLOSEST(ic_clk * tSYMBOL, MICRO) - 8 + offset; + return DIV_ROUND_CLOSEST_ULL((u64)ic_clk * tSYMBOL, MICRO) - + 8 + offset; else /* * Conditional expression: @@ -367,7 +368,8 @@ u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset) * The reason why we need to take into account "tf" here, * is the same as described in i2c_dw_scl_lcnt(). */ - return DIV_ROUND_CLOSEST(ic_clk * (tSYMBOL + tf), MICRO) - 3 + offset; + return DIV_ROUND_CLOSEST_ULL((u64)ic_clk * (tSYMBOL + tf), MICRO) - + 3 + offset; } u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) @@ -383,7 +385,8 @@ u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) * account the fall time of SCL signal (tf). Default tf value * should be 0.3 us, for safety. */ - return DIV_ROUND_CLOSEST(ic_clk * (tLOW + tf), MICRO) - 1 + offset; + return DIV_ROUND_CLOSEST_ULL((u64)ic_clk * (tLOW + tf), MICRO) - + 1 + offset; } int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)