| Message ID | 20180213165948.GA14035@embeddedor.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI/ASPM: Use 64-bit arithmetic instead of 32-bit | expand |
On Tue, Feb 13, 2018 at 6:59 PM, Gustavo A. R. Silva <garsilva@embeddedor.com> 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 ? But still the question. have you checked all callers? Does it even makes sense?
Hi Andy, Quoting Andy Shevchenko <andy.shevchenko@gmail.com>: > On Tue, Feb 13, 2018 at 6:59 PM, Gustavo A. R. Silva > <garsilva@embeddedor.com> 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. Thanks -- Gustavo
On Tue, Feb 13, 2018 at 9:05 PM, Gustavo A. R. Silva <garsilva@embeddedor.com> wrote: > 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. Whatever Bjorn prefers.
On Tue, Feb 13, 2018 at 01:05:50PM -0600, Gustavo A. R. Silva wrote: > Hi Andy, > > Quoting Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Tue, Feb 13, 2018 at 6:59 PM, Gustavo A. R. Silva > > <garsilva@embeddedor.com> 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!
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 57feef2..684d326 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -322,7 +322,7 @@ static u32 calc_l1ss_pwron(struct pci_dev *pdev, u32 scale, u32 val) static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value) { - u64 threshold_ns = threshold_us * 1000; + u64 threshold_ns = threshold_us * 1000ULL; /* See PCIe r3.1, sec 7.33.3 and sec 6.18 */ if (threshold_ns < 32) {
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. Addresses-Coverity-ID: 1462501 Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com> --- drivers/pci/pcie/aspm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)