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

[Gustavo A. R. Silva]

Quoting Hans Verkuil <hverkuil@xxxxxxxxx>:

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