| Message ID | 20240301144517.2811370-1-usama.anjum@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: spi-nor: core: correct type of i to be signed | expand |
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;
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
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; >
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; >> >
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!
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>
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;
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(-)