On Sat, 7 Jul 2012 21:57:47 -0700, Guenter Roeck wrote: > On Sat, Jul 07, 2012 at 05:03:09PM +0200, Jean Delvare wrote: > > On Thu, 21 Jun 2012 06:56:54 -0700, Guenter Roeck wrote: > > > @@ -67,7 +67,8 @@ static ssize_t show_power(struct device *dev, > > > REG_TDP_LIMIT3, &val); > > > > > > tdp_limit = val >> 16; > > > - curr_pwr_watts = (tdp_limit + data->base_tdp) << running_avg_range; > > > + curr_pwr_watts = ((u64)(tdp_limit + > > > + data->base_tdp)) << running_avg_range; > > > > Even then, "tdp_limit + data->base_tdp" could overflow already, on > > 32-bit architectures [1]. So I think what you really want is: > > > > curr_pwr_watts = ((u64)tdp_limit + data->base_tdp) << running_avg_range; > > > Both tdp_limit and data->base_tdp can not be larger than 65535, so I figured > that the "inner" overflow is not an issue. Ah, right, I could have easily figured that out by myself, had I taken the time to read the code. Sorry for the noise, and this patch of yours is: Acked-by: Jean Delvare <khali@xxxxxxxxxxxx> > > And BTW, this might be good to push to stable trees, as we've had real > > problems with overflows in this driver in the past. I don't know the > > realistic values for tdp_limit, base_tdp and running_avg_range so I'm > > not sure. > > Max: (65535 + 65535) << 16 = 8589803520 = 0x1FFFE0000. No idea if this is can > happen in the real world. No idea either. Maybe Andreas can tell. But if we have no reason to believe this can't happen, I'd say we play it safe and push the fix to stable trees. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
