Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Pavel Machek]

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 
> > ---
> > 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


signature.asc
Description: Digital signature

Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Quoting Hans Verkuil :


On 02/05/18 22:54, Gustavo A. R. Silva wrote:

Hi Hans,

Quoting Hans Verkuil :


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 
---
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

Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Hans Verkuil]

On 02/05/18 22:54, Gustavo A. R. Silva wrote:
> Hi Hans,
> 
> Quoting Hans Verkuil :
> 
>> 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 
>>> ---
>>> 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

Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

Hi Hans,

Quoting Hans Verkuil :


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 
---
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);

Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Hans Verkuil]

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 
> ---
> 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);
> 

[PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit

[Gustavo A. R. Silva]

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 
---
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