mtd: spi-nor: core: correct type of i to be signed

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

Posted by Muhammad Usama Anjum 1 year, 11 months ago

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 <usama.anjum@collabora.com>
---
 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

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

Posted by AngeloGioacchino Del Regno 1 year, 11 months ago

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 <usama.anjum@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

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

Posted by Dan Carpenter 1 year, 11 months ago

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 <usama.anjum@collabora.com>
> ---
>  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

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

Posted by Michael Walle 1 year, 11 months ago

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 <usama.anjum@collabora.com>
> ---
>  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;

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

Posted by Tudor Ambarus 1 year, 11 months ago


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 <usama.anjum@collabora.com>
>> ---
>>  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;
>

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

Posted by Muhammad Usama Anjum 1 year, 11 months ago

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 <usama.anjum@collabora.com>
>>> ---
>>>  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

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

Posted by Tudor Ambarus 1 year, 11 months ago


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 <usama.anjum@collabora.com>
>>>> ---
>>>>  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!