2013/10/21 Dirk Brandewie <dirk.brandewie@xxxxxxxxx>: > > > On Monday, October 21, 2013, Rafael J. Wysocki wrote: >> >> On Monday, October 21, 2013 03:43:51 PM Dirk Brandewie wrote: >> > On 10/21/2013 03:47 PM, Rafael J. Wysocki wrote: >> > > On Monday, October 21, 2013 08:56:22 AM Dirk Brandewie wrote: >> > >> On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: >> > >>> The expression 'pstate << 8' is evaluated using 32-bit arithmetic >> > >>> while >> > >>> 'val' expects an expression of type u64. >> > >>> >> > >>> Signed-off-by: Geyslan G. Bem <geyslan@xxxxxxxxx> >> > >> Acked-by: Dirk Brandewie <dirk.j.brandewie@xxxxxxxxx> >> > > >> > > Actually, isn't (pstate << 8) guaranteed not to overflow? >> > > >> > >> > Yes, I was assuming this was caught by a static checking tool. >> Yes, it was caught by Coverity, and I didn't debug the possibles pstate's. >> What was caught by the tool was the fact that 1UL << 32 might overflow on >> 32-bit, so using BIT(32) wasn't correct. This is the entire output: CID 1108110 (#1 of 1): Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)overflow_before_widen: Potentially overflowing expression "pstate << 8" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic before being used in a context which expects an expression of type "u64" (64 bits, unsigned). To avoid overflow, cast the left operand to "u64" before performing the left shift. >> >> > I didn't see a downside to giving the compilier complete information. >> >> Well, in that case the function's argument should be u64 rather than int. >> >> Either you know that it won't overflow, in which case the explicit type >> casting doesn't change anything, or you are not sure, in which case it's >> better to use u64 as the original type anyway in my opinion. > > > pstate << 8 can't overflow we can drop this I realized that are five calls to intel_pstate_set_pstate() /drivers/cpufreq/intel_pstate.c:410 /drivers/cpufreq/intel_pstate.c:417 /drivers/cpufreq/intel_pstate.c:432 /drivers/cpufreq/intel_pstate.c:514 /drivers/cpufreq/intel_pstate.c:566 I really don't know if the values that pstate assumes could cause overflow. And I really don't know if these values may change in the future. So I have assumed that the most careful is to cast, making the code error proof. But you know the code, thus know what is better. ;) Cheers. > >> >> Thanks! >> >> -- >> I speak only for myself. >> Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
