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 <[email protected]>
---
drivers/pci/pcie/aspm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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) {
--
2.7.4
On Tue, Feb 13, 2018 at 6:59 PM, Gustavo A. R. Silva
<[email protected]> 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?
--
With Best Regards,
Andy Shevchenko
On Tue, Feb 13, 2018 at 9:05 PM, Gustavo A. R. Silva
<[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Quoting Andy Shevchenko <[email protected]>:
> On Tue, Feb 13, 2018 at 6:59 PM, Gustavo A. R. Silva
> <[email protected]> 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 01:05:50PM -0600, Gustavo A. R. Silva wrote:
> Hi Andy,
>
> Quoting Andy Shevchenko <[email protected]>:
>
> > On Tue, Feb 13, 2018 at 6:59 PM, Gustavo A. R. Silva
> > <[email protected]> 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!