Messages in this thread | | | Date | Tue, 06 Feb 2018 10:35:42 -0600 | From | "Gustavo A. R. Silva" <> | Subject | Re: [PATCH v2 8/8] platform: vivid-cec: use 64-bit arithmetic instead of 32-bit |
| |
Quoting Hans Verkuil <hverkuil@xs4all.nl>:
> On 02/05/18 22:54, Gustavo A. R. Silva wrote: >> Hi Hans, >> >> Quoting Hans Verkuil <hverkuil@xs4all.nl>: >> >>> 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 <gustavo@embeddedor.com> >>>> --- >>>> 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
| |