* [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 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
* [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
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