Add suffix LL and ULL to various constants in order to give the compiler
complete information about the proper arithmetic to use. Such constants
are used in contexts that expect expressions of type u64 (64 bits, unsigned)
and s64 (64 bits, signed).
The mentioned expressions are currently being evaluated using 32-bit
arithmetic, wich is some cases can lead to unintentional integer
overflows.
This patchset addresses the following Coverity IDs:
CIDs: 200604, 1056807, 1056808, 1271223, 1324146
CIDs: 1392628, 1392630, 1446589, 1454996, 1458347
Thank you
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Add suffix ULL and LL to constants instead of casting variables.
- Extend the proposed code changes to other similar cases that had not
previously been considered in v1 of this patchset.
Gustavo A. R. Silva (8):
rtl2832: use 64-bit arithmetic instead of 32-bit in
rtl2832_set_frontend
dvb-frontends: ves1820: use 64-bit arithmetic instead of 32-bit
i2c: max2175: use 64-bit arithmetic instead of 32-bit
i2c: ov9650: use 64-bit arithmetic instead of 32-bit
pci: cx88-input: use 64-bit arithmetic instead of 32-bit
rockchip/rga: use 64-bit arithmetic instead of 32-bit
platform: sh_veu: use 64-bit arithmetic instead of 32-bit
platform: vivid-cec: use 64-bit arithmetic instead of 32-bit
drivers/media/dvb-frontends/rtl2832.c | 4 ++--
drivers/media/dvb-frontends/ves1820.c | 2 +-
drivers/media/i2c/max2175.c | 2 +-
drivers/media/i2c/ov9650.c | 9 +++++----
drivers/media/pci/cx88/cx88-input.c | 4 ++--
drivers/media/platform/rockchip/rga/rga-buf.c | 3 ++-
drivers/media/platform/sh_veu.c | 4 ++--
drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++--
8 files changed, 24 insertions(+), 15 deletions(-)
--
2.7.4
Add suffix ULL to constant 7 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
u64 (64 bits, unsigned).
The expression dev->pdata->clk * 7 is currently being evaluated
using 32-bit arithmetic.
Addresses-Coverity-ID: 1271223
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Add suffix ULL to constant instead of casting a variable.
drivers/media/dvb-frontends/rtl2832.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
index 94bf5b7..fa3b816 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -498,7 +498,7 @@ static int rtl2832_set_frontend(struct dvb_frontend *fe)
* RSAMP_RATIO = floor(CrystalFreqHz * 7 * pow(2, 22)
* / ConstWithBandwidthMode)
*/
- num = dev->pdata->clk * 7;
+ num = dev->pdata->clk * 7ULL;
num *= 0x400000;
num = div_u64(num, bw_mode);
resamp_ratio = num & 0x3ffffff;
@@ -511,7 +511,7 @@ static int rtl2832_set_frontend(struct dvb_frontend *fe)
* / (CrystalFreqHz * 7))
*/
num = bw_mode << 20;
- num2 = dev->pdata->clk * 7;
+ num2 = dev->pdata->clk * 7ULL;
num = div_u64(num, num2);
num = -num;
cfreq_off_ratio = num & 0xfffff;
--
2.7.4
Add suffix ULL to constant 10 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
u64 (64 bits, unsigned).
The expression fpxin = state->config->xin * 10 is currently being
evaluated using 32-bit arithmetic.
Addresses-Coverity-ID: 200604
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Add suffix ULL to constant instead of casting a variable.
drivers/media/dvb-frontends/ves1820.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/dvb-frontends/ves1820.c b/drivers/media/dvb-frontends/ves1820.c
index 1d89792..1760098 100644
--- a/drivers/media/dvb-frontends/ves1820.c
+++ b/drivers/media/dvb-frontends/ves1820.c
@@ -137,7 +137,7 @@ static int ves1820_set_symbolrate(struct ves1820_state *state, u32 symbolrate)
NDEC = 3;
/* yeuch! */
- fpxin = state->config->xin * 10;
+ fpxin = state->config->xin * 10ULL;
fptmp = fpxin; do_div(fptmp, 123);
if (symbolrate < fptmp)
SFIL = 1;
--
2.7.4
Add suffix LL to constant 1000000 in order to give the compiler
complete information about the proper arithmetic to use. Notice
that this constant is used in a context that expects an expression
of type ktime_t (64 bits, signed).
The expression ir->polling * 1000000 is currently being evaluated
using 32-bit arithmetic.
Addresses-Coverity-ID: 1392628
Addresses-Coverity-ID: 1392630
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Add suffix LL to constant instead of casting a variable.
drivers/media/pci/cx88/cx88-input.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/cx88/cx88-input.c b/drivers/media/pci/cx88/cx88-input.c
index 4e9953e..6f4e692 100644
--- a/drivers/media/pci/cx88/cx88-input.c
+++ b/drivers/media/pci/cx88/cx88-input.c
@@ -180,7 +180,7 @@ static enum hrtimer_restart cx88_ir_work(struct hrtimer *timer)
struct cx88_IR *ir = container_of(timer, struct cx88_IR, timer);
cx88_ir_handle_key(ir);
- missed = hrtimer_forward_now(&ir->timer, ir->polling * 1000000);
+ missed = hrtimer_forward_now(&ir->timer, ir->polling * 1000000LL);
if (missed > 1)
ir_dprintk("Missed ticks %ld\n", missed - 1);
@@ -200,7 +200,7 @@ static int __cx88_ir_start(void *priv)
if (ir->polling) {
hrtimer_init(&ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
ir->timer.function = cx88_ir_work;
- hrtimer_start(&ir->timer, ir->polling * 1000000,
+ hrtimer_start(&ir->timer, ir->polling * 1000000LL,
HRTIMER_MODE_REL);
}
if (ir->sampling) {
--
2.7.4
Cast p to dma_addr_t in order to avoid a potential integer overflow.
This variable is being used in a context that expects an expression
of type dma_addr_t (u64).
The expression p << PAGE_SHIFT is currently being evaluated
using 32-bit arithmetic.
Addresses-Coverity-ID: 1458347
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code change.
drivers/media/platform/rockchip/rga/rga-buf.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c b/drivers/media/platform/rockchip/rga/rga-buf.c
index 49cacc7..a43b57a 100644
--- a/drivers/media/platform/rockchip/rga/rga-buf.c
+++ b/drivers/media/platform/rockchip/rga/rga-buf.c
@@ -140,7 +140,8 @@ void rga_buf_map(struct vb2_buffer *vb)
address = sg_phys(sgl);
for (p = 0; p < len; p++) {
- dma_addr_t phys = address + (p << PAGE_SHIFT);
+ dma_addr_t phys = address +
+ ((dma_addr_t)p << PAGE_SHIFT);
pages[mapped_size + p] = phys;
}
--
2.7.4
Cast left and top to dma_addr_t in order to give the compiler complete
information about the proper arithmetic to use. Notice that these
variables are being used in contexts that expect expressions of
type dma_addr_t (64 bit, unsigned).
Such expressions are currently being evaluated using 32-bit arithmetic.
Also, move the expression (((dma_addr_t)left * veu->vfmt_out.fmt->depth) >> 3)
at the end in order to avoid a line wrapping checkpatch.pl warning.
Addresses-Coverity-ID: 1056807
Addresses-Coverity-ID: 1056808
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Move the expression (((dma_addr_t)left * veu->vfmt_out.fmt->depth) >> 3)
at the end in order to avoid a line wrapping checkpatch.pl warning.
drivers/media/platform/sh_veu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c
index 976ea0b..1a0cde0 100644
--- a/drivers/media/platform/sh_veu.c
+++ b/drivers/media/platform/sh_veu.c
@@ -520,8 +520,8 @@ static void sh_veu_colour_offset(struct sh_veu_dev *veu, struct sh_veu_vfmt *vfm
/* dst_left and dst_top validity will be verified in CROP / COMPOSE */
unsigned int left = vfmt->frame.left & ~0x03;
unsigned int top = vfmt->frame.top;
- dma_addr_t offset = ((left * veu->vfmt_out.fmt->depth) >> 3) +
- top * veu->vfmt_out.bytesperline;
+ dma_addr_t offset = (dma_addr_t)top * veu->vfmt_out.bytesperline +
+ (((dma_addr_t)left * veu->vfmt_out.fmt->depth) >> 3);
unsigned int y_line;
vfmt->offset_y = offset;
--
2.7.4
Add suffix LL to constant 2 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
s64 (64 bits, signed).
The expression 2 * (clock_rate - abs_nco_freq) is currently being
evaluated using 32-bit arithmetic.
Addresses-Coverity-ID: 1446589
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Add suffix LL to constant instead of casting a variable.
drivers/media/i2c/max2175.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/max2175.c b/drivers/media/i2c/max2175.c
index 2f1966b..87cba15 100644
--- a/drivers/media/i2c/max2175.c
+++ b/drivers/media/i2c/max2175.c
@@ -643,7 +643,7 @@ static int max2175_set_nco_freq(struct max2175 *ctx, s32 nco_freq)
if (abs_nco_freq < clock_rate / 2) {
nco_val_desired = 2 * nco_freq;
} else {
- nco_val_desired = 2 * (clock_rate - abs_nco_freq);
+ nco_val_desired = 2LL * (clock_rate - abs_nco_freq);
if (nco_freq < 0)
nco_val_desired = -nco_val_desired;
}
--
2.7.4
Add suffix ULL to constants 10000 and 1000000 in order to give the
compiler complete information about the proper arithmetic to use.
Notice that these constants are used in contexts that expect
expressions of type u64 (64 bits, unsigned).
The following expressions:
(u64)(fi->interval.numerator * 10000)
(u64)(iv->interval.numerator * 10000)
fiv->interval.numerator * 1000000 / fiv->interval.denominator
are currently being evaluated using 32-bit arithmetic.
Notice that those casts to u64 for the first two expressions are only
effective after such expressions are evaluated using 32-bit arithmetic,
which leads to potential integer overflows. So based on those casts, it
seems that the original intention of the code is to actually use 64-bit
arithmetic instead of 32-bit.
Also, notice that once the suffix ULL is added to the constants, the
outer casts to u64 are no longer needed.
Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
Fixes: 84a15ded76ec ("[media] V4L: Add driver for OV9650/52 image sensors")
Fixes: 79211c8ed19c ("remove abs64()")
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Add suffix ULL to constants instead of casting variables.
- Remove unnecessary casts to u64 as part of the code change.
- Extend the same code change to other similar expressions.
drivers/media/i2c/ov9650.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c
index e519f27..e716e98 100644
--- a/drivers/media/i2c/ov9650.c
+++ b/drivers/media/i2c/ov9650.c
@@ -1130,7 +1130,7 @@ static int __ov965x_set_frame_interval(struct ov965x *ov965x,
if (fi->interval.denominator == 0)
return -EINVAL;
- req_int = (u64)(fi->interval.numerator * 10000) /
+ req_int = fi->interval.numerator * 10000ULL /
fi->interval.denominator;
for (i = 0; i < ARRAY_SIZE(ov965x_intervals); i++) {
@@ -1139,7 +1139,7 @@ static int __ov965x_set_frame_interval(struct ov965x *ov965x,
if (mbus_fmt->width != iv->size.width ||
mbus_fmt->height != iv->size.height)
continue;
- err = abs((u64)(iv->interval.numerator * 10000) /
+ err = abs(iv->interval.numerator * 10000ULL /
iv->interval.denominator - req_int);
if (err < min_err) {
fiv = iv;
@@ -1148,8 +1148,9 @@ static int __ov965x_set_frame_interval(struct ov965x *ov965x,
}
ov965x->fiv = fiv;
- v4l2_dbg(1, debug, &ov965x->sd, "Changed frame interval to %u us\n",
- fiv->interval.numerator * 1000000 / fiv->interval.denominator);
+ v4l2_dbg(1, debug, &ov965x->sd, "Changed frame interval to %llu us\n",
+ fiv->interval.numerator * 1000000ULL /
+ fiv->interval.denominator);
return 0;
}
--
2.7.4
Add suffix ULL to constant 10 in order to give the compiler complete
information about the proper arithmetic to use. Notice that this
constant is used in a context that expects an expression of type
u64 (64 bits, unsigned).
The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
evaluated using 32-bit arithmetic.
Also, remove unnecessary parentheses and add a code comment to make it
clear what is the reason of the code change.
Addresses-Coverity-ID: 1454996
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Update subject and changelog to better reflect the proposed code changes.
- Add suffix ULL to constant instead of casting a variable.
- Remove unncessary parentheses.
- Add code comment.
drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c
index b55d278..614787b 100644
--- a/drivers/media/platform/vivid/vivid-cec.c
+++ b/drivers/media/platform/vivid/vivid-cec.c
@@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct cec_adapter *adap, ktime_t ts,
if (adap == NULL)
return;
- ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
- len * 10 * CEC_TIM_DATA_BIT_TOTAL));
+
+ /*
+ * Suffix ULL on constant 10 makes the expression
+ * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
+ * be evaluated using 64-bit unsigned arithmetic (u64), which
+ * is what ktime_sub_us expects as second argument.
+ */
+ ts = ktime_sub_us(ts, CEC_TIM_START_BIT_TOTAL +
+ 10ULL * len * CEC_TIM_DATA_BIT_TOTAL);
cec_queue_pin_cec_event(adap, false, ts);
ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
cec_queue_pin_cec_event(adap, true, ts);
--
2.7.4
On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote:
> Add suffix ULL to constant 10 in order to give the compiler complete
> information about the proper arithmetic to use. Notice that this
> constant is used in a context that expects an expression of type
> u64 (64 bits, unsigned).
>
> The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
> evaluated using 32-bit arithmetic.
>
> Also, remove unnecessary parentheses and add a code comment to make it
> clear what is the reason of the code change.
>
> Addresses-Coverity-ID: 1454996
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
> Changes in v2:
> - Update subject and changelog to better reflect the proposed code changes.
> - Add suffix ULL to constant instead of casting a variable.
> - Remove unncessary parentheses.
unncessary -> unnecessary
> - Add code comment.
>
> drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c
> index b55d278..614787b 100644
> --- a/drivers/media/platform/vivid/vivid-cec.c
> +++ b/drivers/media/platform/vivid/vivid-cec.c
> @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct cec_adapter *adap, ktime_t ts,
>
> if (adap == NULL)
> return;
> - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
> - len * 10 * CEC_TIM_DATA_BIT_TOTAL));
> +
> + /*
> + * Suffix ULL on constant 10 makes the expression
> + * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
> + * be evaluated using 64-bit unsigned arithmetic (u64), which
> + * is what ktime_sub_us expects as second argument.
> + */
That's not really the comment that I was looking for. It still doesn't
explain *why* this is needed at all. How about something like this:
/*
* Add the ULL suffix to the constant 10 to work around a false Coverity
* "Unintentional integer overflow" warning. Coverity isn't smart enough
* to understand that len is always <= 16, so there is no chance of an
* integer overflow.
*/
Regards,
Hans
> + ts = ktime_sub_us(ts, CEC_TIM_START_BIT_TOTAL +
> + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL);
> cec_queue_pin_cec_event(adap, false, ts);
> ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
> cec_queue_pin_cec_event(adap, true, ts);
>
Hi Hans,
Quoting Hans Verkuil <[email protected]>:
> On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote:
>> Add suffix ULL to constant 10 in order to give the compiler complete
>> information about the proper arithmetic to use. Notice that this
>> constant is used in a context that expects an expression of type
>> u64 (64 bits, unsigned).
>>
>> The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
>> evaluated using 32-bit arithmetic.
>>
>> Also, remove unnecessary parentheses and add a code comment to make it
>> clear what is the reason of the code change.
>>
>> Addresses-Coverity-ID: 1454996
>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>> ---
>> Changes in v2:
>> - Update subject and changelog to better reflect the proposed code changes.
>> - Add suffix ULL to constant instead of casting a variable.
>> - Remove unncessary parentheses.
>
> unncessary -> unnecessary
>
Thanks for this.
>> - Add code comment.
>>
>> drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vivid/vivid-cec.c
>> b/drivers/media/platform/vivid/vivid-cec.c
>> index b55d278..614787b 100644
>> --- a/drivers/media/platform/vivid/vivid-cec.c
>> +++ b/drivers/media/platform/vivid/vivid-cec.c
>> @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct
>> cec_adapter *adap, ktime_t ts,
>>
>> if (adap == NULL)
>> return;
>> - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
>> - len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>> +
>> + /*
>> + * Suffix ULL on constant 10 makes the expression
>> + * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
>> + * be evaluated using 64-bit unsigned arithmetic (u64), which
>> + * is what ktime_sub_us expects as second argument.
>> + */
>
> That's not really the comment that I was looking for. It still doesn't
> explain *why* this is needed at all. How about something like this:
>
In MHO the reason for the change is simply the discrepancy between the
arithmetic expected by
the function ktime_sub_us and the arithmetic in which the expression
is being evaluated. And this
has nothing to do with any particular tool.
> /*
> * Add the ULL suffix to the constant 10 to work around a false Coverity
> * "Unintentional integer overflow" warning. Coverity isn't smart enough
> * to understand that len is always <= 16, so there is no chance of an
> * integer overflow.
> */
>
:P
In my opinion it is not a good idea to tie the code to a particular tool.
There are only three appearances of the word 'Coverity' in the whole
code base, and, honestly I don't want to add more.
So I think I will document this issue as a FP in the Coverity platform.
Thanks!
--
Gustavo
> Regards,
>
> Hans
>
>> + ts = ktime_sub_us(ts, CEC_TIM_START_BIT_TOTAL +
>> + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL);
>> cec_queue_pin_cec_event(adap, false, ts);
>> ts = ktime_add_us(ts, CEC_TIM_START_BIT_LOW);
>> cec_queue_pin_cec_event(adap, true, ts);
>>
On 02/05/18 22:54, Gustavo A. R. Silva wrote:
> Hi Hans,
>
> Quoting Hans Verkuil <[email protected]>:
>
>> On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote:
>>> Add suffix ULL to constant 10 in order to give the compiler complete
>>> information about the proper arithmetic to use. Notice that this
>>> constant is used in a context that expects an expression of type
>>> u64 (64 bits, unsigned).
>>>
>>> The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
>>> evaluated using 32-bit arithmetic.
>>>
>>> Also, remove unnecessary parentheses and add a code comment to make it
>>> clear what is the reason of the code change.
>>>
>>> Addresses-Coverity-ID: 1454996
>>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>>> ---
>>> Changes in v2:
>>> - Update subject and changelog to better reflect the proposed code changes.
>>> - Add suffix ULL to constant instead of casting a variable.
>>> - Remove unncessary parentheses.
>>
>> unncessary -> unnecessary
>>
>
> Thanks for this.
>
>>> - Add code comment.
>>>
>>> drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vivid/vivid-cec.c
>>> b/drivers/media/platform/vivid/vivid-cec.c
>>> index b55d278..614787b 100644
>>> --- a/drivers/media/platform/vivid/vivid-cec.c
>>> +++ b/drivers/media/platform/vivid/vivid-cec.c
>>> @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct
>>> cec_adapter *adap, ktime_t ts,
>>>
>>> if (adap == NULL)
>>> return;
>>> - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
>>> - len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>> +
>>> + /*
>>> + * Suffix ULL on constant 10 makes the expression
>>> + * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
>>> + * be evaluated using 64-bit unsigned arithmetic (u64), which
>>> + * is what ktime_sub_us expects as second argument.
>>> + */
>>
>> That's not really the comment that I was looking for. It still doesn't
>> explain *why* this is needed at all. How about something like this:
>>
>
> In MHO the reason for the change is simply the discrepancy between the
> arithmetic expected by
> the function ktime_sub_us and the arithmetic in which the expression
> is being evaluated. And this
> has nothing to do with any particular tool.
Hmm, you have a point.
OK, I've looked at the other patches in this patch series as well, and
the only thing I would like to see changed is the 'Addresses-Coverity-ID'
line in the patches: patch 4 says:
Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
but that's the only one that mentions the specific coverity error.
It would be nice if that can be added to the other patches as well so
we have a record of the actual coverity error.
>
>> /*
>> * Add the ULL suffix to the constant 10 to work around a false Coverity
>> * "Unintentional integer overflow" warning. Coverity isn't smart enough
>> * to understand that len is always <= 16, so there is no chance of an
>> * integer overflow.
>> */
>>
>
> :P
>
> In my opinion it is not a good idea to tie the code to a particular tool.
> There are only three appearances of the word 'Coverity' in the whole
> code base, and, honestly I don't want to add more.
>
> So I think I will document this issue as a FP in the Coverity platform.
FP?
Regards,
Hans
Quoting Hans Verkuil <[email protected]>:
> On 02/05/18 22:54, Gustavo A. R. Silva wrote:
>> Hi Hans,
>>
>> Quoting Hans Verkuil <[email protected]>:
>>
>>> On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote:
>>>> Add suffix ULL to constant 10 in order to give the compiler complete
>>>> information about the proper arithmetic to use. Notice that this
>>>> constant is used in a context that expects an expression of type
>>>> u64 (64 bits, unsigned).
>>>>
>>>> The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
>>>> evaluated using 32-bit arithmetic.
>>>>
>>>> Also, remove unnecessary parentheses and add a code comment to make it
>>>> clear what is the reason of the code change.
>>>>
>>>> Addresses-Coverity-ID: 1454996
>>>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>>>> ---
>>>> Changes in v2:
>>>> - Update subject and changelog to better reflect the proposed
>>>> code changes.
>>>> - Add suffix ULL to constant instead of casting a variable.
>>>> - Remove unncessary parentheses.
>>>
>>> unncessary -> unnecessary
>>>
>>
>> Thanks for this.
>>
>>>> - Add code comment.
>>>>
>>>> drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++--
>>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/vivid/vivid-cec.c
>>>> b/drivers/media/platform/vivid/vivid-cec.c
>>>> index b55d278..614787b 100644
>>>> --- a/drivers/media/platform/vivid/vivid-cec.c
>>>> +++ b/drivers/media/platform/vivid/vivid-cec.c
>>>> @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct
>>>> cec_adapter *adap, ktime_t ts,
>>>>
>>>> if (adap == NULL)
>>>> return;
>>>> - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
>>>> - len * 10 * CEC_TIM_DATA_BIT_TOTAL));
>>>> +
>>>> + /*
>>>> + * Suffix ULL on constant 10 makes the expression
>>>> + * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
>>>> + * be evaluated using 64-bit unsigned arithmetic (u64), which
>>>> + * is what ktime_sub_us expects as second argument.
>>>> + */
>>>
>>> That's not really the comment that I was looking for. It still doesn't
>>> explain *why* this is needed at all. How about something like this:
>>>
>>
>> In MHO the reason for the change is simply the discrepancy between the
>> arithmetic expected by
>> the function ktime_sub_us and the arithmetic in which the expression
>> is being evaluated. And this
>> has nothing to do with any particular tool.
>
> Hmm, you have a point.
>
> OK, I've looked at the other patches in this patch series as well, and
> the only thing I would like to see changed is the 'Addresses-Coverity-ID'
> line in the patches: patch 4 says:
>
> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>
> but that's the only one that mentions the specific coverity error.
> It would be nice if that can be added to the other patches as well so
> we have a record of the actual coverity error.
>
OK. I'll send v3 of the whole patch series shortly.
Thank you!
>>
>>> /*
>>> * Add the ULL suffix to the constant 10 to work around a false Coverity
>>> * "Unintentional integer overflow" warning. Coverity isn't smart enough
>>> * to understand that len is always <= 16, so there is no chance of an
>>> * integer overflow.
>>> */
>>>
>>
>> :P
>>
>> In my opinion it is not a good idea to tie the code to a particular tool.
>> There are only three appearances of the word 'Coverity' in the whole
>> code base, and, honestly I don't want to add more.
>>
>> So I think I will document this issue as a FP in the Coverity platform.
>
> FP?
>
False Positive.
> Regards,
>
> Hans
--
Gustavo
On Mon 2018-02-05 22:29:41, Hans Verkuil wrote:
> On 02/05/2018 09:36 PM, Gustavo A. R. Silva wrote:
> > Add suffix ULL to constant 10 in order to give the compiler complete
> > information about the proper arithmetic to use. Notice that this
> > constant is used in a context that expects an expression of type
> > u64 (64 bits, unsigned).
> >
> > The expression len * 10 * CEC_TIM_DATA_BIT_TOTAL is currently being
> > evaluated using 32-bit arithmetic.
> >
> > Also, remove unnecessary parentheses and add a code comment to make it
> > clear what is the reason of the code change.
> >
> > Addresses-Coverity-ID: 1454996
> > Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > ---
> > Changes in v2:
> > - Update subject and changelog to better reflect the proposed code changes.
> > - Add suffix ULL to constant instead of casting a variable.
> > - Remove unncessary parentheses.
>
> unncessary -> unnecessary
>
> > - Add code comment.
> >
> > drivers/media/platform/vivid/vivid-cec.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c
> > index b55d278..614787b 100644
> > --- a/drivers/media/platform/vivid/vivid-cec.c
> > +++ b/drivers/media/platform/vivid/vivid-cec.c
> > @@ -82,8 +82,15 @@ static void vivid_cec_pin_adap_events(struct cec_adapter *adap, ktime_t ts,
> >
> > if (adap == NULL)
> > return;
> > - ts = ktime_sub_us(ts, (CEC_TIM_START_BIT_TOTAL +
> > - len * 10 * CEC_TIM_DATA_BIT_TOTAL));
> > +
> > + /*
> > + * Suffix ULL on constant 10 makes the expression
> > + * CEC_TIM_START_BIT_TOTAL + 10ULL * len * CEC_TIM_DATA_BIT_TOTAL
> > + * be evaluated using 64-bit unsigned arithmetic (u64), which
> > + * is what ktime_sub_us expects as second argument.
> > + */
>
> That's not really the comment that I was looking for. It still doesn't
> explain *why* this is needed at all. How about something like this:
>
> /*
> * Add the ULL suffix to the constant 10 to work around a false Coverity
> * "Unintentional integer overflow" warning. Coverity isn't smart enough
> * to understand that len is always <= 16, so there is no chance of an
> * integer overflow.
> */
Or maybe it would be better to add comment about Coverity having
false-positive and not to modify the code?
Hmm. Could we do something like BUG_ON(len > 16) to make Coverity
understand the ranges?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html