From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753425AbcIDKlx (ORCPT ); Sun, 4 Sep 2016 06:41:53 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:56818 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752855AbcIDKlv (ORCPT ); Sun, 4 Sep 2016 06:41:51 -0400 Date: Sun, 4 Sep 2016 12:39:27 +0200 (CEST) From: Thomas Gleixner To: Liav Rehana cc: "linux-kernel@vger.kernel.org" , "john.stultz@linaro.org" , Noam Camus , Elad Kanfi Subject: RE: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation. In-Reply-To: Message-ID: References: <1472728478-3788-1-git-send-email-liavr@mellanox.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 4 Sep 2016, Liav Rehana wrote: > >> The root of the problem is that in case the multiplication of delta > >> and > >> tkr->mult in the line that I've changed is too big that the MSB of the > >> result is set, then the shift will cause an unwanted sign extension. > > > I completely understand that, but as I said before: > > > > > This typecast is just a baindaid. What happens if you double the > > > > suspend time? The multiplication will simply overflow. So the > > > > proper fix is to sanity check delta and do multiple conversions if > > > > delta is big enough. Preferrably this happens somewhere at the call > > > > site and not in this hotpath function. > > > > That sign extension will be avoided completely if the variable nsec > > > was unsigned (u64 instead of s64), so I think the correct solution for > > > this is to change the type of nsec to u64. > > > That's a different story and its not a solution for the general problem of > > > delta * mult >= (1 << 31) or delta * mult >= (1 << 32) > > The case that delta * mult >= 1 << 31 is not a problem by itself, but it causes > an unwanted sign extension since the type of nsec is signed. That sign > extension is what causes the loop to take too long, and not the overflow. > I understand that the typecast is not a general solution, so as I've said, I > think that changing the type of nsec to u64 instead of s64 will be a good and > general solution, as it will indeed solve the problem of the unwanted sign > extension. > > To summarize: a sign extension occurs if the nsec variable is signed, and so > I ask if you think it will be a good solution to change its type to unsigned. Do you actually read what I write? I asked John before: > John, why is that stuff signed at all? Shouldn't we use u64 for all of this? So to summarize: - Yes, we can use u64 if there is nothing which I missed, but John will have the last word on this - No, making it u64 does not solve the general problem. It just papers over the problem you observe. And we don't add 'paper over' fixes, period. Thanks, tglx