Re: [PATCH] IB/hfi1: Fix a wrapping test (make it less strict)

Dan Carpenter
2017-10-25T05:14:35Z

On Tue, Oct 24, 2017 at 07:54:40PM +0000, Marciniszyn, Mike wrote:
> > Subject: Re: [PATCH] IB/hfi1: Fix a wrapping test (make it less strict)
> >
> > On Mon, Oct 23, 2017 at 02:33:46PM -0400, Doug Ledford wrote:
> > > > > > @@ -3781,7 +3781,7 @@ static int
> > > > > > __subn_get_opa_hfi1_cong_log(struct
> > > > > > opa_smp *smp, u32 am,
> > > > > > * required to wrap the counter are supposed to
> > > > > > * be zeroed (CA10-49 IBTA, release 1.2.1, V1).
> > > > > > */
> > > > > > - if ((u64)(ts - cce->timestamp) > (2 * UINT_MAX))
> > > > > > + if ((u64)(ts - cce->timestamp) > (2ULL *
> > > > > > UINT_MAX))
> >
> > This is really weird looking. Both ts and cce->timestamp are s64, why
> > do the convoluted conversion to unsigned? And surely UINT_MAX is not
> > the right thing..
> >
> > if ((ts - cce->timestamp)/2 > 0xFFFFFFFF)
> >
> > ?
> >
> > ktime_get is defined to be monotonic, so ts - cce->timestamp should
> > never go negative.
> >
>
> I agree that this is an issue.
>
> My proposal:
> - Change s64 to u64 for ts and in timestamp in opa_hfi1_cong_log_event_internal
> - Change the calls:
> ktime_to_ns(ktime_get()) / 1024
> -- to --
> ktime_get_ns() / 1024
> - Change to use Jason's test from above with UINT_MAX
>
> Dan, we can do the patch or you can send a v2?
>

Can you do the patch and give me a Reported-by tag?

regards,
dan carpenter