On Tue, Feb 13, 2018 at 01:05:50PM -0600, Gustavo A. R. Silva wrote: > Hi Andy, > > Quoting Andy Shevchenko <andy.shevchenko@xxxxxxxxx>: > > > On Tue, Feb 13, 2018 at 6:59 PM, Gustavo A. R. Silva > > <garsilva@xxxxxxxxxxxxxx> wrote: > > > Add suffix ULL to constant 1000 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 threshold_us * 1000 is currently being evaluated > > > using 32-bit arithmetic. > > > > > - u64 threshold_ns = threshold_us * 1000; > > > + u64 threshold_ns = threshold_us * 1000ULL; > > > > Shouldn't be other way around, i.e. > > > > (u64)threshold_us ? > > > > Either way works. The thing is that casting threshold_us to u64 may imply > that there is something wrong with threshold_us, which does not seem to be > the case. So adding the suffix ULL to the constant 1000 is good enough to > make the expression be evaluated using 64-bit arithmetic instead of 32-bit. > > But, again, either way works. > > > But still the question. have you checked all callers? Does it even makes > > sense? > > > > The proposed patch was due to fact that currently threshold_ns is of type > u64. But based on the following piece of code (which is the only piece of > code from where encode_l12_threshold is being called): > > * Based on PCIe r3.1, sec 5.5.3.3.1, Figures 5-16 and 5-17, and > * Table 5-11. T(POWER_OFF) is at most 2us and T(L1.2) is at > * least 4us. > */ > l1_2_threshold = 2 + 4 + t_common_mode + t_power_on; > encode_l12_threshold(l1_2_threshold, &scale, &value); > > It seems to me that it makes no sense for threshold_ns to be of type u64, > because the expression threshold_us * 1000 will never exceed the 32-bit > limits. So if you agree I can send a patch to change its type to u32 > instead. Changing it to u32 sounds good to me. I can't remember why I chose u64 to begin with, but it doesn't look necessary. Thanks for cleaning this up!
