* [PATCH 13/38] ARM: shmobile: apmu: correct type of CPU id [not found] <1442842450-29769-1-git-send-email-a.hajda@samsung.com> @ 2015-09-21 13:33 ` Andrzej Hajda 2015-10-02 1:57 ` Simon Horman 2015-09-21 13:42 ` [PATCH 00/38] Fixes related to incorrect usage of unsigned types David Howells 1 sibling, 1 reply; 5+ messages in thread From: Andrzej Hajda @ 2015-09-21 13:33 UTC (permalink / raw) To: linux-arm-kernel CPU id can be negative, so it cannot be assigned to unsigned variable. The problem has been detected using proposed semantic patch scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1]. [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576 Signed-off-by: Andrzej Hajda <a.hajda@samsung•com> --- arch/arm/mach-shmobile/platsmp-apmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c b/arch/arm/mach-shmobile/platsmp-apmu.c index 4e54512..292c67b 100644 --- a/arch/arm/mach-shmobile/platsmp-apmu.c +++ b/arch/arm/mach-shmobile/platsmp-apmu.c @@ -88,7 +88,7 @@ static void apmu_init_cpu(struct resource *res, int cpu, int bit) static void apmu_parse_cfg(void (*fn)(struct resource *res, int cpu, int bit), struct rcar_apmu_config *apmu_config, int num) { - u32 id; + int id; int k; int bit, index; bool is_allowed; -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 13/38] ARM: shmobile: apmu: correct type of CPU id 2015-09-21 13:33 ` [PATCH 13/38] ARM: shmobile: apmu: correct type of CPU id Andrzej Hajda @ 2015-10-02 1:57 ` Simon Horman 0 siblings, 0 replies; 5+ messages in thread From: Simon Horman @ 2015-10-02 1:57 UTC (permalink / raw) To: linux-arm-kernel On Mon, Sep 21, 2015 at 03:33:45PM +0200, Andrzej Hajda wrote: > CPU id can be negative, so it cannot be assigned to unsigned variable. > > The problem has been detected using proposed semantic patch > scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci [1]. > > [1]: http://permalink.gmane.org/gmane.linux.kernel/2038576 > > Signed-off-by: Andrzej Hajda <a.hajda@samsung•com> Thanks, I have queued this up as a cleanup for v4.4. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 00/38] Fixes related to incorrect usage of unsigned types [not found] <1442842450-29769-1-git-send-email-a.hajda@samsung.com> 2015-09-21 13:33 ` [PATCH 13/38] ARM: shmobile: apmu: correct type of CPU id Andrzej Hajda @ 2015-09-21 13:42 ` David Howells 2015-09-22 9:13 ` Andrzej Hajda 1 sibling, 1 reply; 5+ messages in thread From: David Howells @ 2015-09-21 13:42 UTC (permalink / raw) To: linux-arm-kernel Andrzej Hajda <a.hajda@samsung•com> wrote: > Semantic patch finds comparisons of types: > unsigned < 0 > unsigned >= 0 > The former is always false, the latter is always true. > Such comparisons are useless, so theoretically they could be > safely removed, but their presence quite often indicates bugs. Or someone has left them in because they don't matter and there's the possibility that the type being tested might be or become signed under some circumstances. If the comparison is useless, I'd expect the compiler to just discard it - for such cases your patch is pointless. If I have, for example: unsigned x; if (x == 0 || x > 27) give_a_range_error(); I will write this as: unsigned x; if (x <= 0 || x > 27) give_a_range_error(); because it that gives a way to handle x being changed to signed at some point in the future for no cost. In which case, your changing the <= to an == "because the < part of the case is useless" is arguably wrong. David ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 00/38] Fixes related to incorrect usage of unsigned types 2015-09-21 13:42 ` [PATCH 00/38] Fixes related to incorrect usage of unsigned types David Howells @ 2015-09-22 9:13 ` Andrzej Hajda 2015-09-22 9:46 ` Jacek Anaszewski 0 siblings, 1 reply; 5+ messages in thread From: Andrzej Hajda @ 2015-09-22 9:13 UTC (permalink / raw) To: linux-arm-kernel On 09/21/2015 03:42 PM, David Howells wrote: > Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public•gmane.org> wrote: > >> Semantic patch finds comparisons of types: >> unsigned < 0 >> unsigned >= 0 >> The former is always false, the latter is always true. >> Such comparisons are useless, so theoretically they could be >> safely removed, but their presence quite often indicates bugs. > > Or someone has left them in because they don't matter and there's the > possibility that the type being tested might be or become signed under some > circumstances. If the comparison is useless, I'd expect the compiler to just > discard it - for such cases your patch is pointless. > > If I have, for example: > > unsigned x; > > if (x == 0 || x > 27) > give_a_range_error(); > > I will write this as: > > unsigned x; > > if (x <= 0 || x > 27) > give_a_range_error(); > > because it that gives a way to handle x being changed to signed at some point > in the future for no cost. In which case, your changing the <= to an == > "because the < part of the case is useless" is arguably wrong. This is why I have not checked for such cases - I have skipped checks of type unsigned <= 0 exactly for the reasons above. However I have left two other checks as they seems to me more suspicious - they are always true or false. But as Dmitry and Andrew pointed out Linus have quite strong opinion against removing range checks in such cases as he finds it clearer. I think it applies to patches 29-36. I am not sure about patches 26-28,37. Regards Andrzej > > David > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA at public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 00/38] Fixes related to incorrect usage of unsigned types 2015-09-22 9:13 ` Andrzej Hajda @ 2015-09-22 9:46 ` Jacek Anaszewski 0 siblings, 0 replies; 5+ messages in thread From: Jacek Anaszewski @ 2015-09-22 9:46 UTC (permalink / raw) To: linux-arm-kernel On 09/22/2015 11:13 AM, Andrzej Hajda wrote: > On 09/21/2015 03:42 PM, David Howells wrote: >> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public•gmane.org> wrote: >> >>> Semantic patch finds comparisons of types: >>> unsigned < 0 >>> unsigned >= 0 >>> The former is always false, the latter is always true. >>> Such comparisons are useless, so theoretically they could be >>> safely removed, but their presence quite often indicates bugs. >> >> Or someone has left them in because they don't matter and there's the >> possibility that the type being tested might be or become signed under some >> circumstances. If the comparison is useless, I'd expect the compiler to just >> discard it - for such cases your patch is pointless. >> >> If I have, for example: >> >> unsigned x; >> >> if (x == 0 || x > 27) >> give_a_range_error(); >> >> I will write this as: >> >> unsigned x; >> >> if (x <= 0 || x > 27) >> give_a_range_error(); >> >> because it that gives a way to handle x being changed to signed at some point >> in the future for no cost. In which case, your changing the <= to an == >> "because the < part of the case is useless" is arguably wrong. > > This is why I have not checked for such cases - I have skipped checks of type > unsigned <= 0 > exactly for the reasons above. > > However I have left two other checks as they seems to me more suspicious - they > are always true or false. But as Dmitry and Andrew pointed out Linus have quite > strong opinion against removing range checks in such cases as he finds it > clearer. I think it applies to patches 29-36. I am not sure about patches 26-28,37. Dropped 30/38 and 31/38 from LED tree then. -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-02 1:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1442842450-29769-1-git-send-email-a.hajda@samsung.com>
2015-09-21 13:33 ` [PATCH 13/38] ARM: shmobile: apmu: correct type of CPU id Andrzej Hajda
2015-10-02 1:57 ` Simon Horman
2015-09-21 13:42 ` [PATCH 00/38] Fixes related to incorrect usage of unsigned types David Howells
2015-09-22 9:13 ` Andrzej Hajda
2015-09-22 9:46 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox