| Message ID | 20150408205837.4329.94829.stgit@jtkirshe-mobl.jf.intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Jeff Kirsher |
| Headers | show |
From: Jeff Kirsher > Sent: 08 April 2015 21:59 > We were using s64 for lat_ns (latency nano-second value) since in > our calculations a negative value could be a resultant. For negative > values, we then assign lat_ns to be zero, so the value passed to > do_div() was never negative, but do_div() expects the argument type > to be u64, so do a cast to resolve a compile warning seen on > PowerPC. ... > - do_div(lat_ns, speed); > + do_div((u64)lat_ns, speed); Personally I think that adding casts to avoid warnings from integer promotions and function arguments isn't a good idea. The problem is that C casts are too powerful and can hide other bugs. In the past I've been bitten by a cast inside a (badly written) #define function. The above is a typical case where the compiler should really be doing value domain checks before reporting anything at all - if at all. David
On Thu, 2015-04-09 at 09:04 +0000, David Laight wrote: > From: Jeff Kirsher > > Sent: 08 April 2015 21:59 > > We were using s64 for lat_ns (latency nano-second value) since in > > our calculations a negative value could be a resultant. For > negative > > values, we then assign lat_ns to be zero, so the value passed to > > do_div() was never negative, but do_div() expects the argument type > > to be u64, so do a cast to resolve a compile warning seen on > > PowerPC. > ... > > - do_div(lat_ns, speed); > > + do_div((u64)lat_ns, speed); > > Personally I think that adding casts to avoid warnings > from integer promotions and function arguments isn't a good idea. > The problem is that C casts are too powerful and can hide other bugs. > In the past I've been bitten by a cast inside a (badly written) > #define > function. > > The above is a typical case where the compiler should really be doing > value domain checks before reporting anything at all - if at all. I agree with you that I do not like casts because it can potentially hide bugs. On that note, I have a follow on patch that should resolve the PowerPC compile warning without having to do a cast. Just re-works the existing code, expect a v2 shortly.
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index 9d81c03..30d277a 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -1045,7 +1045,7 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link) if (lat_ns < 0) lat_ns = 0; else - do_div(lat_ns, speed); + do_div((u64)lat_ns, speed); value = lat_ns; while (value > PCI_LTR_VALUE_MASK) {
We were using s64 for lat_ns (latency nano-second value) since in our calculations a negative value could be a resultant. For negative values, we then assign lat_ns to be zero, so the value passed to do_div() was never negative, but do_div() expects the argument type to be u64, so do a cast to resolve a compile warning seen on PowerPC. CC: Yanjiang Jin <yanjiang.jin@windriver.com> CC: Yanir Lubetkin <yanirx.lubetkin@intel.com> Reported-by: Yanjiang Jin <yanjiang.jin@windriver.com> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)