| Message ID | 20200211144704.GA6461@embeddedor |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] i2c: altera: Fix potential integer overflow | expand |
On 2/11/20 8:47 AM, Gustavo A. R. Silva wrote: > Factor out 100 from the equation and do 32-bit arithmetic (3 * clk_mhz / 10) > instead of 64-bit. > > Notice that clk_mhz is MHz, so the multiplication will never wrap 32 bits > and there is no need for div_u64(). > > Addresses-Coverity: 1458369 ("Unintentional integer overflow") > Fixes: 0560ad576268 ("i2c: altera: Add Altera I2C Controller driver") > Suggested-by: David Laight <David.Laight@ACULAB.COM> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > Changes in v2: > - Update subject and changelog text. > - Avoid the need for 64-bit arithmetic at all. > > drivers/i2c/busses/i2c-altera.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-altera.c b/drivers/i2c/busses/i2c-altera.c > index 5255d3755411..1de23b4f3809 100644 > --- a/drivers/i2c/busses/i2c-altera.c > +++ b/drivers/i2c/busses/i2c-altera.c > @@ -171,7 +171,7 @@ static void altr_i2c_init(struct altr_i2c_dev *idev) > /* SCL Low Time */ > writel(t_low, idev->base + ALTR_I2C_SCL_LOW); > /* SDA Hold Time, 300ns */ > - writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD); > + writel(3 * clk_mhz / 10, idev->base + ALTR_I2C_SDA_HOLD); > > /* Mask all master interrupt bits */ > altr_i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false); > Reviewed-by: Thor Thayer <thor.thayer@linux.intel.com>
On Tue, Feb 11, 2020 at 08:47:04AM -0600, Gustavo A. R. Silva wrote: > Factor out 100 from the equation and do 32-bit arithmetic (3 * clk_mhz / 10) > instead of 64-bit. > > Notice that clk_mhz is MHz, so the multiplication will never wrap 32 bits > and there is no need for div_u64(). Was there ever? With u32 clk_mhz = clk_get_rate(idev->i2c_clk) / 1000000; a later multiplication with 300 should not wrap u32? > /* SDA Hold Time, 300ns */ > - writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD); > + writel(3 * clk_mhz / 10, idev->base + ALTR_I2C_SDA_HOLD); The change itself is OK, yet I wonder about the comment above: 'clk_mhz * 0.3' will not give a constant 300ns, or?
From: Wolfram Sang > Sent: 13 February 2020 09:10 > > On Tue, Feb 11, 2020 at 08:47:04AM -0600, Gustavo A. R. Silva wrote: > > Factor out 100 from the equation and do 32-bit arithmetic (3 * clk_mhz / 10) > > instead of 64-bit. > > > > Notice that clk_mhz is MHz, so the multiplication will never wrap 32 bits > > and there is no need for div_u64(). > > Was there ever? With > > u32 clk_mhz = clk_get_rate(idev->i2c_clk) / 1000000; > > a later multiplication with 300 should not wrap u32? > > > /* SDA Hold Time, 300ns */ > > - writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD); > > + writel(3 * clk_mhz / 10, idev->base + ALTR_I2C_SDA_HOLD); > > The change itself is OK, yet I wonder about the comment above: > > 'clk_mhz * 0.3' will not give a constant 300ns, or? Depends on the definition of the register. A count of zero may mean one clock period. So maybe it could have (3 * clk - 1)/10 instead of (3 * clk + 9)/10. OTOH nothing probably requires that much hold time. If that is the 'standard' Altera Avalon slave I2C 'megafunction' I2C master then it is probably so slow to use it can't matter. Most of those blocks are crap, they aren't even small. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/i2c/busses/i2c-altera.c b/drivers/i2c/busses/i2c-altera.c index 5255d3755411..1de23b4f3809 100644 --- a/drivers/i2c/busses/i2c-altera.c +++ b/drivers/i2c/busses/i2c-altera.c @@ -171,7 +171,7 @@ static void altr_i2c_init(struct altr_i2c_dev *idev) /* SCL Low Time */ writel(t_low, idev->base + ALTR_I2C_SCL_LOW); /* SDA Hold Time, 300ns */ - writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD); + writel(3 * clk_mhz / 10, idev->base + ALTR_I2C_SDA_HOLD); /* Mask all master interrupt bits */ altr_i2c_int_enable(idev, ALTR_I2C_ALL_IRQ, false);
Factor out 100 from the equation and do 32-bit arithmetic (3 * clk_mhz / 10) instead of 64-bit. Notice that clk_mhz is MHz, so the multiplication will never wrap 32 bits and there is no need for div_u64(). Addresses-Coverity: 1458369 ("Unintentional integer overflow") Fixes: 0560ad576268 ("i2c: altera: Add Altera I2C Controller driver") Suggested-by: David Laight <David.Laight@ACULAB.COM> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- Changes in v2: - Update subject and changelog text. - Avoid the need for 64-bit arithmetic at all. drivers/i2c/busses/i2c-altera.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)