2024-03-01 14:46:22

by Muhammad Usama Anjum

[permalink] [raw]
Subject: [PATCH] mtd: spi-nor: core: correct type of i to be signed

The i should be signed to find out the end of the loop. Otherwise,
i >= 0 is always true and loop becomes infinite.

Fixes: 6a9eda34418f ("mtd: spi-nor: core: set mtd->eraseregions for non-uniform erase map")
Signed-off-by: Muhammad Usama Anjum <[email protected]>
---
drivers/mtd/spi-nor/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 65b32ea59afc6..46bc45b80883f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3373,7 +3373,7 @@ static u32
spi_nor_get_region_erasesize(const struct spi_nor_erase_region *region,
const struct spi_nor_erase_type *erase_type)
{
- u8 i;
+ s8 i;

if (region->overlaid)
return region->size;
--
2.39.2



2024-03-01 15:04:43

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH] mtd: spi-nor: core: correct type of i to be signed

Hi,

On Fri Mar 1, 2024 at 3:45 PM CET, Muhammad Usama Anjum wrote:
> The i should be signed to find out the end of the loop. Otherwise,
> i >= 0 is always true and loop becomes infinite.
>
> Fixes: 6a9eda34418f ("mtd: spi-nor: core: set mtd->eraseregions for non-uniform erase map")
> Signed-off-by: Muhammad Usama Anjum <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 65b32ea59afc6..46bc45b80883f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3373,7 +3373,7 @@ static u32
> spi_nor_get_region_erasesize(const struct spi_nor_erase_region *region,
> const struct spi_nor_erase_type *erase_type)
> {
> - u8 i;
> + s8 i;

Can we just have an "int" here. So we don't shoot ourselves in the
foot again. I'm really no friend of these kind of micro
optimizations, it should have been int in the first place IMHO.

-michael

>
> if (region->overlaid)
> return region->size;

Attachments:
signature.asc (259.00 B)

2024-03-01 17:05:15

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH] mtd: spi-nor: core: correct type of i to be signed

On Fri, Mar 01, 2024 at 07:45:16PM +0500, Muhammad Usama Anjum wrote:
> The i should be signed to find out the end of the loop. Otherwise,
> i >= 0 is always true and loop becomes infinite.
>
> Fixes: 6a9eda34418f ("mtd: spi-nor: core: set mtd->eraseregions for non-uniform erase map")
> Signed-off-by: Muhammad Usama Anjum <[email protected]>
> ---
> drivers/mtd/spi-nor/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 65b32ea59afc6..46bc45b80883f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3373,7 +3373,7 @@ static u32
> spi_nor_get_region_erasesize(const struct spi_nor_erase_region *region,
> const struct spi_nor_erase_type *erase_type)
> {
> - u8 i;
> + s8 i;

I have a patch in my output which also addresses this bug but you beat
me to the punch. Declaring iterators as "u8 i;" is a canonical bug.

https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/

regards,
dan carpenter


2024-03-04 07:51:40

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH] mtd: spi-nor: core: correct type of i to be signed



On 3/1/24 15:04, Michael Walle wrote:
> Hi,
>
> On Fri Mar 1, 2024 at 3:45 PM CET, Muhammad Usama Anjum wrote:
>> The i should be signed to find out the end of the loop. Otherwise,
>> i >= 0 is always true and loop becomes infinite.
>>
>> Fixes: 6a9eda34418f ("mtd: spi-nor: core: set mtd->eraseregions for non-uniform erase map")
>> Signed-off-by: Muhammad Usama Anjum <[email protected]>
>> ---
>> drivers/mtd/spi-nor/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 65b32ea59afc6..46bc45b80883f 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3373,7 +3373,7 @@ static u32
>> spi_nor_get_region_erasesize(const struct spi_nor_erase_region *region,
>> const struct spi_nor_erase_type *erase_type)
>> {
>> - u8 i;
>> + s8 i;
>
> Can we just have an "int" here. So we don't shoot ourselves in the

Muhammad, do you care to resubmit using int? Would be better indeed.

> foot again. I'm really no friend of these kind of micro
> optimizations, it should have been int in the first place IMHO.
>
> -michael
>
>>
>> if (region->overlaid)
>> return region->size;
>

2024-03-04 08:53:21

by Muhammad Usama Anjum

[permalink] [raw]
Subject: Re: [PATCH] mtd: spi-nor: core: correct type of i to be signed

On 3/4/24 12:51 PM, Tudor Ambarus wrote:
>
>
> On 3/1/24 15:04, Michael Walle wrote:
>> Hi,
>>
>> On Fri Mar 1, 2024 at 3:45 PM CET, Muhammad Usama Anjum wrote:
>>> The i should be signed to find out the end of the loop. Otherwise,
>>> i >= 0 is always true and loop becomes infinite.
>>>
>>> Fixes: 6a9eda34418f ("mtd: spi-nor: core: set mtd->eraseregions for non-uniform erase map")
>>> Signed-off-by: Muhammad Usama Anjum <[email protected]>
>>> ---
>>> drivers/mtd/spi-nor/core.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 65b32ea59afc6..46bc45b80883f 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -3373,7 +3373,7 @@ static u32
>>> spi_nor_get_region_erasesize(const struct spi_nor_erase_region *region,
>>> const struct spi_nor_erase_type *erase_type)
>>> {
>>> - u8 i;
>>> + s8 i;
>>
>> Can we just have an "int" here. So we don't shoot ourselves in the
>
> Muhammad, do you care to resubmit using int? Would be better indeed.
I'm sending a v2 with int.

>
>> foot again. I'm really no friend of these kind of micro
>> optimizations, it should have been int in the first place IMHO.
>>
>> -michael
>>
>>>
>>> if (region->overlaid)
>>> return region->size;
>>
>

--
BR,
Muhammad Usama Anjum

2024-03-04 08:53:56

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH] mtd: spi-nor: core: correct type of i to be signed



On 3/4/24 08:46, Muhammad Usama Anjum wrote:
> On 3/4/24 12:51 PM, Tudor Ambarus wrote:
>>
>>
>> On 3/1/24 15:04, Michael Walle wrote:
>>> Hi,
>>>
>>> On Fri Mar 1, 2024 at 3:45 PM CET, Muhammad Usama Anjum wrote:
>>>> The i should be signed to find out the end of the loop. Otherwise,
>>>> i >= 0 is always true and loop becomes infinite.
>>>>
>>>> Fixes: 6a9eda34418f ("mtd: spi-nor: core: set mtd->eraseregions for non-uniform erase map")
>>>> Signed-off-by: Muhammad Usama Anjum <[email protected]>
>>>> ---
>>>> drivers/mtd/spi-nor/core.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 65b32ea59afc6..46bc45b80883f 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -3373,7 +3373,7 @@ static u32
>>>> spi_nor_get_region_erasesize(const struct spi_nor_erase_region *region,
>>>> const struct spi_nor_erase_type *erase_type)
>>>> {
>>>> - u8 i;
>>>> + s8 i;
>>>
>>> Can we just have an "int" here. So we don't shoot ourselves in the
>>
>> Muhammad, do you care to resubmit using int? Would be better indeed.
> I'm sending a v2 with int.

thanks!

Subject: Re: [PATCH] mtd: spi-nor: core: correct type of i to be signed

Il 01/03/24 15:45, Muhammad Usama Anjum ha scritto:
> The i should be signed to find out the end of the loop. Otherwise,
> i >= 0 is always true and loop becomes infinite.
>
> Fixes: 6a9eda34418f ("mtd: spi-nor: core: set mtd->eraseregions for non-uniform erase map")
> Signed-off-by: Muhammad Usama Anjum <[email protected]>

Reviewed-by: AngeloGioacchino Del Regno <[email protected]>