| Message ID | 1457662816-10584-1-git-send-email-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | ea9619aed6908e83b0679bd9c9aa4ae97714ef97 |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: > The stm_is_locked_sr() function is picked from Linux kernel. For reason > unknown, the 64bit data types used by the function and present in Linux > were replaced with 32bit unsigned ones, which causes trouble. > > The testcase performed was done using ST M25P80 chip. > The command used was: > => sf protect unlock 0 0x10000 > > The call chain starts in stm_unlock(), which calls stm_is_locked_sr() > with negative ofs argument. This works fine in Linux, where the "ofs" > is loff_t, which is signed long long, while this fails in U-Boot, where > "ofs" is u32 (unsigned int). Because of this signedness problem, the > expression past the return statement to be incorrectly evaluated to 1, > which in turn propagates back to stm_unlock() and results in -EINVAL. > > The correction is very simple, just use the correctly sized data types > with correct signedness in the function to make it work as intended. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Simon Glass <sjg@chromium.org> > Cc: Jagan Teki <jteki@openedev.com> > --- > drivers/mtd/spi/spi_flash.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c > index 2ae2e3c..44d9e9b 100644 > --- a/drivers/mtd/spi/spi_flash.c > +++ b/drivers/mtd/spi/spi_flash.c > @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, > > #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) > static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, > - u32 *len) > + u64 *len) What about uint64_t?
On 03/11/2016 07:39 AM, Jagan Teki wrote: > On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >> The stm_is_locked_sr() function is picked from Linux kernel. For reason >> unknown, the 64bit data types used by the function and present in Linux >> were replaced with 32bit unsigned ones, which causes trouble. >> >> The testcase performed was done using ST M25P80 chip. >> The command used was: >> => sf protect unlock 0 0x10000 >> >> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >> with negative ofs argument. This works fine in Linux, where the "ofs" >> is loff_t, which is signed long long, while this fails in U-Boot, where >> "ofs" is u32 (unsigned int). Because of this signedness problem, the >> expression past the return statement to be incorrectly evaluated to 1, >> which in turn propagates back to stm_unlock() and results in -EINVAL. >> >> The correction is very simple, just use the correctly sized data types >> with correct signedness in the function to make it work as intended. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Jagan Teki <jteki@openedev.com> >> --- >> drivers/mtd/spi/spi_flash.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >> index 2ae2e3c..44d9e9b 100644 >> --- a/drivers/mtd/spi/spi_flash.c >> +++ b/drivers/mtd/spi/spi_flash.c >> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >> >> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >> - u32 *len) >> + u64 *len) > > What about uint64_t? This is now same as Linux too.
On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: > On 03/11/2016 07:39 AM, Jagan Teki wrote: >> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>> unknown, the 64bit data types used by the function and present in Linux >>> were replaced with 32bit unsigned ones, which causes trouble. >>> >>> The testcase performed was done using ST M25P80 chip. >>> The command used was: >>> => sf protect unlock 0 0x10000 >>> >>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>> with negative ofs argument. This works fine in Linux, where the "ofs" >>> is loff_t, which is signed long long, while this fails in U-Boot, where >>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>> expression past the return statement to be incorrectly evaluated to 1, >>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>> >>> The correction is very simple, just use the correctly sized data types >>> with correct signedness in the function to make it work as intended. >>> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Simon Glass <sjg@chromium.org> >>> Cc: Jagan Teki <jteki@openedev.com> >>> --- >>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>> index 2ae2e3c..44d9e9b 100644 >>> --- a/drivers/mtd/spi/spi_flash.c >>> +++ b/drivers/mtd/spi/spi_flash.c >>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>> >>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>> - u32 *len) >>> + u64 *len) >> >> What about uint64_t? > > This is now same as Linux too. I couldn't find it on l2-mtd and ML as well, it is still uint64_t
On 03/11/2016 06:34 PM, Jagan Teki wrote: > On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: >> On 03/11/2016 07:39 AM, Jagan Teki wrote: >>> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>>> unknown, the 64bit data types used by the function and present in Linux >>>> were replaced with 32bit unsigned ones, which causes trouble. >>>> >>>> The testcase performed was done using ST M25P80 chip. >>>> The command used was: >>>> => sf protect unlock 0 0x10000 >>>> >>>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>>> with negative ofs argument. This works fine in Linux, where the "ofs" >>>> is loff_t, which is signed long long, while this fails in U-Boot, where >>>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>>> expression past the return statement to be incorrectly evaluated to 1, >>>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>>> >>>> The correction is very simple, just use the correctly sized data types >>>> with correct signedness in the function to make it work as intended. >>>> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>> Cc: Simon Glass <sjg@chromium.org> >>>> Cc: Jagan Teki <jteki@openedev.com> >>>> --- >>>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>>> index 2ae2e3c..44d9e9b 100644 >>>> --- a/drivers/mtd/spi/spi_flash.c >>>> +++ b/drivers/mtd/spi/spi_flash.c >>>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>>> >>>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>>> - u32 *len) >>>> + u64 *len) >>> >>> What about uint64_t? >> >> This is now same as Linux too. > > I couldn't find it on l2-mtd and ML as well, it is still uint64_t > You are not supposed to use stdint.h types in either kernel or u-boot if this is what you are concerned about. Thus, u64.
On 11 March 2016 at 23:32, Marek Vasut <marex@denx.de> wrote: > On 03/11/2016 06:34 PM, Jagan Teki wrote: >> On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: >>> On 03/11/2016 07:39 AM, Jagan Teki wrote: >>>> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>>>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>>>> unknown, the 64bit data types used by the function and present in Linux >>>>> were replaced with 32bit unsigned ones, which causes trouble. >>>>> >>>>> The testcase performed was done using ST M25P80 chip. >>>>> The command used was: >>>>> => sf protect unlock 0 0x10000 >>>>> >>>>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>>>> with negative ofs argument. This works fine in Linux, where the "ofs" >>>>> is loff_t, which is signed long long, while this fails in U-Boot, where >>>>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>>>> expression past the return statement to be incorrectly evaluated to 1, >>>>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>>>> >>>>> The correction is very simple, just use the correctly sized data types >>>>> with correct signedness in the function to make it work as intended. >>>>> >>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>> Cc: Simon Glass <sjg@chromium.org> >>>>> Cc: Jagan Teki <jteki@openedev.com> >>>>> --- >>>>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>>>> index 2ae2e3c..44d9e9b 100644 >>>>> --- a/drivers/mtd/spi/spi_flash.c >>>>> +++ b/drivers/mtd/spi/spi_flash.c >>>>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>>>> >>>>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>>>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>>>> - u32 *len) >>>>> + u64 *len) >>>> >>>> What about uint64_t? >>> >>> This is now same as Linux too. >> >> I couldn't find it on l2-mtd and ML as well, it is still uint64_t >> > You are not supposed to use stdint.h types in either kernel or u-boot if > this is what you are concerned about. Thus, u64. No, I'm saying Linux is still using uint64_t and why can't we use the same?
On 03/11/2016 07:07 PM, Jagan Teki wrote: > On 11 March 2016 at 23:32, Marek Vasut <marex@denx.de> wrote: >> On 03/11/2016 06:34 PM, Jagan Teki wrote: >>> On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: >>>> On 03/11/2016 07:39 AM, Jagan Teki wrote: >>>>> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>>>>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>>>>> unknown, the 64bit data types used by the function and present in Linux >>>>>> were replaced with 32bit unsigned ones, which causes trouble. >>>>>> >>>>>> The testcase performed was done using ST M25P80 chip. >>>>>> The command used was: >>>>>> => sf protect unlock 0 0x10000 >>>>>> >>>>>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>>>>> with negative ofs argument. This works fine in Linux, where the "ofs" >>>>>> is loff_t, which is signed long long, while this fails in U-Boot, where >>>>>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>>>>> expression past the return statement to be incorrectly evaluated to 1, >>>>>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>>>>> >>>>>> The correction is very simple, just use the correctly sized data types >>>>>> with correct signedness in the function to make it work as intended. >>>>>> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>> Cc: Simon Glass <sjg@chromium.org> >>>>>> Cc: Jagan Teki <jteki@openedev.com> >>>>>> --- >>>>>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>>>>> index 2ae2e3c..44d9e9b 100644 >>>>>> --- a/drivers/mtd/spi/spi_flash.c >>>>>> +++ b/drivers/mtd/spi/spi_flash.c >>>>>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>>>>> >>>>>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>>>>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>>>>> - u32 *len) >>>>>> + u64 *len) >>>>> >>>>> What about uint64_t? >>>> >>>> This is now same as Linux too. >>> >>> I couldn't find it on l2-mtd and ML as well, it is still uint64_t >>> >> You are not supposed to use stdint.h types in either kernel or u-boot if >> this is what you are concerned about. Thus, u64. > > No, I'm saying Linux is still using uint64_t and why can't we use the same? > Very quick google search gets you for example here: http://article.gmane.org/gmane.linux.kernel/259313 Quote: " In short: having the kernel use the same names as user space is ACTIVELY BAD, exactly because those names have standards-defined visibility, which means that the kernel _cannot_ use them in all places anyway. So don't even _try_. " There are multiple discussions about the same thing in U-Boot ML as well, I am sure you can find them yourself. I would be much more interested in getting this fix into current release instead of discussing some stupid type.
On 12 March 2016 at 00:03, Marek Vasut <marex@denx.de> wrote: > On 03/11/2016 07:07 PM, Jagan Teki wrote: >> On 11 March 2016 at 23:32, Marek Vasut <marex@denx.de> wrote: >>> On 03/11/2016 06:34 PM, Jagan Teki wrote: >>>> On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: >>>>> On 03/11/2016 07:39 AM, Jagan Teki wrote: >>>>>> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>>>>>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>>>>>> unknown, the 64bit data types used by the function and present in Linux >>>>>>> were replaced with 32bit unsigned ones, which causes trouble. >>>>>>> >>>>>>> The testcase performed was done using ST M25P80 chip. >>>>>>> The command used was: >>>>>>> => sf protect unlock 0 0x10000 >>>>>>> >>>>>>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>>>>>> with negative ofs argument. This works fine in Linux, where the "ofs" >>>>>>> is loff_t, which is signed long long, while this fails in U-Boot, where >>>>>>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>>>>>> expression past the return statement to be incorrectly evaluated to 1, >>>>>>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>>>>>> >>>>>>> The correction is very simple, just use the correctly sized data types >>>>>>> with correct signedness in the function to make it work as intended. >>>>>>> >>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>> Cc: Simon Glass <sjg@chromium.org> >>>>>>> Cc: Jagan Teki <jteki@openedev.com> >>>>>>> --- >>>>>>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>>>>>> index 2ae2e3c..44d9e9b 100644 >>>>>>> --- a/drivers/mtd/spi/spi_flash.c >>>>>>> +++ b/drivers/mtd/spi/spi_flash.c >>>>>>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>>>>>> >>>>>>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>>>>>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>>>>>> - u32 *len) >>>>>>> + u64 *len) >>>>>> >>>>>> What about uint64_t? >>>>> >>>>> This is now same as Linux too. >>>> >>>> I couldn't find it on l2-mtd and ML as well, it is still uint64_t >>>> >>> You are not supposed to use stdint.h types in either kernel or u-boot if >>> this is what you are concerned about. Thus, u64. >> >> No, I'm saying Linux is still using uint64_t and why can't we use the same? >> > Very quick google search gets you for example here: > > http://article.gmane.org/gmane.linux.kernel/259313 > > Quote: > " > In short: having the kernel use the same names as user space is ACTIVELY > BAD, exactly because those names have standards-defined visibility, > which means that the kernel _cannot_ use them in all places anyway. So > don't even _try_. > " Yes, clear I knew this too - but this protect code is a copy from Linux it better to be the same. ie only my concern. thanks!
Hello Jagan, On Fri, 11 Mar 2016 12:09:37 +0530, Jagan Teki <jagannadh.teki@gmail.com> wrote: > On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: > > The stm_is_locked_sr() function is picked from Linux kernel. For reason > > unknown, the 64bit data types used by the function and present in Linux > > were replaced with 32bit unsigned ones, which causes trouble. > > > > The testcase performed was done using ST M25P80 chip. > > The command used was: > > => sf protect unlock 0 0x10000 > > > > The call chain starts in stm_unlock(), which calls stm_is_locked_sr() > > with negative ofs argument. This works fine in Linux, where the "ofs" > > is loff_t, which is signed long long, while this fails in U-Boot, where > > "ofs" is u32 (unsigned int). Because of this signedness problem, the > > expression past the return statement to be incorrectly evaluated to 1, > > which in turn propagates back to stm_unlock() and results in -EINVAL. > > > > The correction is very simple, just use the correctly sized data types > > with correct signedness in the function to make it work as intended. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Jagan Teki <jteki@openedev.com> > > --- > > drivers/mtd/spi/spi_flash.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c > > index 2ae2e3c..44d9e9b 100644 > > --- a/drivers/mtd/spi/spi_flash.c > > +++ b/drivers/mtd/spi/spi_flash.c > > @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, > > > > #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) > > static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, > > - u32 *len) > > + u64 *len) > > What about uint64_t? Well, the U-Boot coding style [1] suggest that we follow the Linux coding style [2] which itself suggests [chapter 5, item (d)] that when uNN types are being used already in some code, then changes to this code should keep on using uNN types. [1] http://www.denx.de/wiki/U-Boot/CodingStyle [2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/CodingStyle > Jagan. Amicalement,
On 03/11/2016 07:44 PM, Jagan Teki wrote: > On 12 March 2016 at 00:03, Marek Vasut <marex@denx.de> wrote: >> On 03/11/2016 07:07 PM, Jagan Teki wrote: >>> On 11 March 2016 at 23:32, Marek Vasut <marex@denx.de> wrote: >>>> On 03/11/2016 06:34 PM, Jagan Teki wrote: >>>>> On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: >>>>>> On 03/11/2016 07:39 AM, Jagan Teki wrote: >>>>>>> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>>>>>>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>>>>>>> unknown, the 64bit data types used by the function and present in Linux >>>>>>>> were replaced with 32bit unsigned ones, which causes trouble. >>>>>>>> >>>>>>>> The testcase performed was done using ST M25P80 chip. >>>>>>>> The command used was: >>>>>>>> => sf protect unlock 0 0x10000 >>>>>>>> >>>>>>>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>>>>>>> with negative ofs argument. This works fine in Linux, where the "ofs" >>>>>>>> is loff_t, which is signed long long, while this fails in U-Boot, where >>>>>>>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>>>>>>> expression past the return statement to be incorrectly evaluated to 1, >>>>>>>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>>>>>>> >>>>>>>> The correction is very simple, just use the correctly sized data types >>>>>>>> with correct signedness in the function to make it work as intended. >>>>>>>> >>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>>> Cc: Simon Glass <sjg@chromium.org> >>>>>>>> Cc: Jagan Teki <jteki@openedev.com> >>>>>>>> --- >>>>>>>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>>>>>>> index 2ae2e3c..44d9e9b 100644 >>>>>>>> --- a/drivers/mtd/spi/spi_flash.c >>>>>>>> +++ b/drivers/mtd/spi/spi_flash.c >>>>>>>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>>>>>>> >>>>>>>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>>>>>>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>>>>>>> - u32 *len) >>>>>>>> + u64 *len) >>>>>>> >>>>>>> What about uint64_t? >>>>>> >>>>>> This is now same as Linux too. >>>>> >>>>> I couldn't find it on l2-mtd and ML as well, it is still uint64_t >>>>> >>>> You are not supposed to use stdint.h types in either kernel or u-boot if >>>> this is what you are concerned about. Thus, u64. >>> >>> No, I'm saying Linux is still using uint64_t and why can't we use the same? >>> >> Very quick google search gets you for example here: >> >> http://article.gmane.org/gmane.linux.kernel/259313 >> >> Quote: >> " >> In short: having the kernel use the same names as user space is ACTIVELY >> BAD, exactly because those names have standards-defined visibility, >> which means that the kernel _cannot_ use them in all places anyway. So >> don't even _try_. >> " > > Yes, clear I knew this too - but this protect code is a copy from > Linux it better to be the same. ie only my concern. Thus, linux should be fixed.
Hi Albert, On 12 March 2016 at 00:17, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hello Jagan, > > On Fri, 11 Mar 2016 12:09:37 +0530, Jagan Teki > <jagannadh.teki@gmail.com> wrote: >> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >> > The stm_is_locked_sr() function is picked from Linux kernel. For reason >> > unknown, the 64bit data types used by the function and present in Linux >> > were replaced with 32bit unsigned ones, which causes trouble. >> > >> > The testcase performed was done using ST M25P80 chip. >> > The command used was: >> > => sf protect unlock 0 0x10000 >> > >> > The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >> > with negative ofs argument. This works fine in Linux, where the "ofs" >> > is loff_t, which is signed long long, while this fails in U-Boot, where >> > "ofs" is u32 (unsigned int). Because of this signedness problem, the >> > expression past the return statement to be incorrectly evaluated to 1, >> > which in turn propagates back to stm_unlock() and results in -EINVAL. >> > >> > The correction is very simple, just use the correctly sized data types >> > with correct signedness in the function to make it work as intended. >> > >> > Signed-off-by: Marek Vasut <marex@denx.de> >> > Cc: Simon Glass <sjg@chromium.org> >> > Cc: Jagan Teki <jteki@openedev.com> >> > --- >> > drivers/mtd/spi/spi_flash.c | 6 +++--- >> > 1 file changed, 3 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >> > index 2ae2e3c..44d9e9b 100644 >> > --- a/drivers/mtd/spi/spi_flash.c >> > +++ b/drivers/mtd/spi/spi_flash.c >> > @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >> > >> > #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >> > static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >> > - u32 *len) >> > + u64 *len) >> >> What about uint64_t? > > Well, the U-Boot coding style [1] suggest that we follow the Linux > coding style [2] which itself suggests [chapter 5, item (d)] that when uNN types means uint32_t/uint64_t ? > uNN types are being used already in some code, then changes to this > code should keep on using uNN types. Sorry, I didn't understand here - if the code having these uNN types the changes to same uNN types? thanks!
Hello Jagan, On Sat, 12 Mar 2016 00:41:25 +0530, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Albert, > > On 12 March 2016 at 00:17, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > > Hello Jagan, > > > > On Fri, 11 Mar 2016 12:09:37 +0530, Jagan Teki > > <jagannadh.teki@gmail.com> wrote: > >> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: > >> > The stm_is_locked_sr() function is picked from Linux kernel. For reason > >> > unknown, the 64bit data types used by the function and present in Linux > >> > were replaced with 32bit unsigned ones, which causes trouble. > >> > > >> > The testcase performed was done using ST M25P80 chip. > >> > The command used was: > >> > => sf protect unlock 0 0x10000 > >> > > >> > The call chain starts in stm_unlock(), which calls stm_is_locked_sr() > >> > with negative ofs argument. This works fine in Linux, where the "ofs" > >> > is loff_t, which is signed long long, while this fails in U-Boot, where > >> > "ofs" is u32 (unsigned int). Because of this signedness problem, the > >> > expression past the return statement to be incorrectly evaluated to 1, > >> > which in turn propagates back to stm_unlock() and results in -EINVAL. > >> > > >> > The correction is very simple, just use the correctly sized data types > >> > with correct signedness in the function to make it work as intended. > >> > > >> > Signed-off-by: Marek Vasut <marex@denx.de> > >> > Cc: Simon Glass <sjg@chromium.org> > >> > Cc: Jagan Teki <jteki@openedev.com> > >> > --- > >> > drivers/mtd/spi/spi_flash.c | 6 +++--- > >> > 1 file changed, 3 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c > >> > index 2ae2e3c..44d9e9b 100644 > >> > --- a/drivers/mtd/spi/spi_flash.c > >> > +++ b/drivers/mtd/spi/spi_flash.c > >> > @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, > >> > > >> > #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) > >> > static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, > >> > - u32 *len) > >> > + u64 *len) > >> > >> What about uint64_t? > > > > Well, the U-Boot coding style [1] suggest that we follow the Linux > > coding style [2] which itself suggests [chapter 5, item (d)] that when > > uNN types means uint32_t/uint64_t ? No, uNN means u8/u16/u32, but I'll admit that may not have been totally unambiguous. > > uNN types are being used already in some code, then changes to this > > code should keep on using uNN types. > > Sorry, I didn't understand here - if the code having these uNN types > the changes to same uNN types? It was better explained in the URL I gave. :) Basically: the Linux (and therefore U-Boot) coding style guide says if some code uses u8/u16/u32, then changes to this code should keep using u8/u16/u32; and here, drivers/mtd/spi/spi_flash.c uses u8, u16 and u32 so the wrongly-sized u32 should be changed into a u64, not into a uint64_t. > thanks! > -- > Jagan. Amicalement,
Hi Albert, On 12 March 2016 at 01:04, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hello Jagan, > > On Sat, 12 Mar 2016 00:41:25 +0530, Jagan Teki > <jagannadh.teki@gmail.com> wrote: >> Hi Albert, >> >> On 12 March 2016 at 00:17, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: >> > Hello Jagan, >> > >> > On Fri, 11 Mar 2016 12:09:37 +0530, Jagan Teki >> > <jagannadh.teki@gmail.com> wrote: >> >> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >> >> > The stm_is_locked_sr() function is picked from Linux kernel. For reason >> >> > unknown, the 64bit data types used by the function and present in Linux >> >> > were replaced with 32bit unsigned ones, which causes trouble. >> >> > >> >> > The testcase performed was done using ST M25P80 chip. >> >> > The command used was: >> >> > => sf protect unlock 0 0x10000 >> >> > >> >> > The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >> >> > with negative ofs argument. This works fine in Linux, where the "ofs" >> >> > is loff_t, which is signed long long, while this fails in U-Boot, where >> >> > "ofs" is u32 (unsigned int). Because of this signedness problem, the >> >> > expression past the return statement to be incorrectly evaluated to 1, >> >> > which in turn propagates back to stm_unlock() and results in -EINVAL. >> >> > >> >> > The correction is very simple, just use the correctly sized data types >> >> > with correct signedness in the function to make it work as intended. >> >> > >> >> > Signed-off-by: Marek Vasut <marex@denx.de> >> >> > Cc: Simon Glass <sjg@chromium.org> >> >> > Cc: Jagan Teki <jteki@openedev.com> >> >> > --- >> >> > drivers/mtd/spi/spi_flash.c | 6 +++--- >> >> > 1 file changed, 3 insertions(+), 3 deletions(-) >> >> > >> >> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >> >> > index 2ae2e3c..44d9e9b 100644 >> >> > --- a/drivers/mtd/spi/spi_flash.c >> >> > +++ b/drivers/mtd/spi/spi_flash.c >> >> > @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >> >> > >> >> > #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >> >> > static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >> >> > - u32 *len) >> >> > + u64 *len) >> >> >> >> What about uint64_t? >> > >> > Well, the U-Boot coding style [1] suggest that we follow the Linux >> > coding style [2] which itself suggests [chapter 5, item (d)] that when >> >> uNN types means uint32_t/uint64_t ? > > No, uNN means u8/u16/u32, but I'll admit that may not have been totally > unambiguous. > >> > uNN types are being used already in some code, then changes to this >> > code should keep on using uNN types. >> >> Sorry, I didn't understand here - if the code having these uNN types >> the changes to same uNN types? > > It was better explained in the URL I gave. :) > > Basically: the Linux (and therefore U-Boot) coding style guide says if > some code uses u8/u16/u32, then changes to this code should keep using > u8/u16/u32; and here, drivers/mtd/spi/spi_flash.c uses u8, u16 and u32 > so the wrongly-sized u32 should be changed into a u64, not into a > uint64_t. Thanks for detailed explanation.
Hi Marek, On 12 March 2016 at 00:29, Marek Vasut <marex@denx.de> wrote: > On 03/11/2016 07:44 PM, Jagan Teki wrote: >> On 12 March 2016 at 00:03, Marek Vasut <marex@denx.de> wrote: >>> On 03/11/2016 07:07 PM, Jagan Teki wrote: >>>> On 11 March 2016 at 23:32, Marek Vasut <marex@denx.de> wrote: >>>>> On 03/11/2016 06:34 PM, Jagan Teki wrote: >>>>>> On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: >>>>>>> On 03/11/2016 07:39 AM, Jagan Teki wrote: >>>>>>>> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>>>>>>>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>>>>>>>> unknown, the 64bit data types used by the function and present in Linux >>>>>>>>> were replaced with 32bit unsigned ones, which causes trouble. >>>>>>>>> >>>>>>>>> The testcase performed was done using ST M25P80 chip. >>>>>>>>> The command used was: >>>>>>>>> => sf protect unlock 0 0x10000 >>>>>>>>> >>>>>>>>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>>>>>>>> with negative ofs argument. This works fine in Linux, where the "ofs" >>>>>>>>> is loff_t, which is signed long long, while this fails in U-Boot, where >>>>>>>>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>>>>>>>> expression past the return statement to be incorrectly evaluated to 1, >>>>>>>>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>>>>>>>> >>>>>>>>> The correction is very simple, just use the correctly sized data types >>>>>>>>> with correct signedness in the function to make it work as intended. >>>>>>>>> >>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>>>> Cc: Simon Glass <sjg@chromium.org> >>>>>>>>> Cc: Jagan Teki <jteki@openedev.com> >>>>>>>>> --- >>>>>>>>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>>>>>>>> index 2ae2e3c..44d9e9b 100644 >>>>>>>>> --- a/drivers/mtd/spi/spi_flash.c >>>>>>>>> +++ b/drivers/mtd/spi/spi_flash.c >>>>>>>>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>>>>>>>> >>>>>>>>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>>>>>>>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>>>>>>>> - u32 *len) >>>>>>>>> + u64 *len) >>>>>>>> >>>>>>>> What about uint64_t? >>>>>>> >>>>>>> This is now same as Linux too. >>>>>> >>>>>> I couldn't find it on l2-mtd and ML as well, it is still uint64_t >>>>>> >>>>> You are not supposed to use stdint.h types in either kernel or u-boot if >>>>> this is what you are concerned about. Thus, u64. >>>> >>>> No, I'm saying Linux is still using uint64_t and why can't we use the same? >>>> >>> Very quick google search gets you for example here: >>> >>> http://article.gmane.org/gmane.linux.kernel/259313 >>> >>> Quote: >>> " >>> In short: having the kernel use the same names as user space is ACTIVELY >>> BAD, exactly because those names have standards-defined visibility, >>> which means that the kernel _cannot_ use them in all places anyway. So >>> don't even _try_. >>> " >> >> Yes, clear I knew this too - but this protect code is a copy from >> Linux it better to be the same. ie only my concern. > > Thus, linux should be fixed. I think Linux look OK with using uint64_t as offset and remaining were not using not uNN and issue with u-boot only as you mentioned on the log message. thanks!
On 12 March 2016 at 20:07, Jagan Teki <jagannadh.teki@gmail.com> wrote: > Hi Marek, > > On 12 March 2016 at 00:29, Marek Vasut <marex@denx.de> wrote: >> On 03/11/2016 07:44 PM, Jagan Teki wrote: >>> On 12 March 2016 at 00:03, Marek Vasut <marex@denx.de> wrote: >>>> On 03/11/2016 07:07 PM, Jagan Teki wrote: >>>>> On 11 March 2016 at 23:32, Marek Vasut <marex@denx.de> wrote: >>>>>> On 03/11/2016 06:34 PM, Jagan Teki wrote: >>>>>>> On 11 March 2016 at 17:59, Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 03/11/2016 07:39 AM, Jagan Teki wrote: >>>>>>>>> On 11 March 2016 at 07:50, Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> The stm_is_locked_sr() function is picked from Linux kernel. For reason >>>>>>>>>> unknown, the 64bit data types used by the function and present in Linux >>>>>>>>>> were replaced with 32bit unsigned ones, which causes trouble. >>>>>>>>>> >>>>>>>>>> The testcase performed was done using ST M25P80 chip. >>>>>>>>>> The command used was: >>>>>>>>>> => sf protect unlock 0 0x10000 >>>>>>>>>> >>>>>>>>>> The call chain starts in stm_unlock(), which calls stm_is_locked_sr() >>>>>>>>>> with negative ofs argument. This works fine in Linux, where the "ofs" >>>>>>>>>> is loff_t, which is signed long long, while this fails in U-Boot, where >>>>>>>>>> "ofs" is u32 (unsigned int). Because of this signedness problem, the >>>>>>>>>> expression past the return statement to be incorrectly evaluated to 1, >>>>>>>>>> which in turn propagates back to stm_unlock() and results in -EINVAL. >>>>>>>>>> >>>>>>>>>> The correction is very simple, just use the correctly sized data types >>>>>>>>>> with correct signedness in the function to make it work as intended. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>>>>>>> Cc: Simon Glass <sjg@chromium.org> >>>>>>>>>> Cc: Jagan Teki <jteki@openedev.com> >>>>>>>>>> --- >>>>>>>>>> drivers/mtd/spi/spi_flash.c | 6 +++--- >>>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c >>>>>>>>>> index 2ae2e3c..44d9e9b 100644 >>>>>>>>>> --- a/drivers/mtd/spi/spi_flash.c >>>>>>>>>> +++ b/drivers/mtd/spi/spi_flash.c >>>>>>>>>> @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, >>>>>>>>>> >>>>>>>>>> #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) >>>>>>>>>> static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, >>>>>>>>>> - u32 *len) >>>>>>>>>> + u64 *len) >>>>>>>>> >>>>>>>>> What about uint64_t? >>>>>>>> >>>>>>>> This is now same as Linux too. >>>>>>> >>>>>>> I couldn't find it on l2-mtd and ML as well, it is still uint64_t >>>>>>> >>>>>> You are not supposed to use stdint.h types in either kernel or u-boot if >>>>>> this is what you are concerned about. Thus, u64. >>>>> >>>>> No, I'm saying Linux is still using uint64_t and why can't we use the same? >>>>> >>>> Very quick google search gets you for example here: >>>> >>>> http://article.gmane.org/gmane.linux.kernel/259313 >>>> >>>> Quote: >>>> " >>>> In short: having the kernel use the same names as user space is ACTIVELY >>>> BAD, exactly because those names have standards-defined visibility, >>>> which means that the kernel _cannot_ use them in all places anyway. So >>>> don't even _try_. >>>> " >>> >>> Yes, clear I knew this too - but this protect code is a copy from >>> Linux it better to be the same. ie only my concern. >> >> Thus, linux should be fixed. > > I think Linux look OK with using uint64_t as offset and remaining were > not using not uNN and issue with u-boot only as you mentioned on the > log message. Applied to u-boot-spi/master thanks!
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 2ae2e3c..44d9e9b 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, - u32 *len) + u64 *len) { u8 mask = SR_BP2 | SR_BP1 | SR_BP0; int shift = ffs(mask) - 1; @@ -685,11 +685,11 @@ static void stm_get_locked_range(struct spi_flash *flash, u8 sr, loff_t *ofs, /* * Return 1 if the entire region is locked, 0 otherwise */ -static int stm_is_locked_sr(struct spi_flash *flash, u32 ofs, u32 len, +static int stm_is_locked_sr(struct spi_flash *flash, loff_t ofs, u64 len, u8 sr) { loff_t lock_offs; - u32 lock_len; + u64 lock_len; stm_get_locked_range(flash, sr, &lock_offs, &lock_len);
The stm_is_locked_sr() function is picked from Linux kernel. For reason unknown, the 64bit data types used by the function and present in Linux were replaced with 32bit unsigned ones, which causes trouble. The testcase performed was done using ST M25P80 chip. The command used was: => sf protect unlock 0 0x10000 The call chain starts in stm_unlock(), which calls stm_is_locked_sr() with negative ofs argument. This works fine in Linux, where the "ofs" is loff_t, which is signed long long, while this fails in U-Boot, where "ofs" is u32 (unsigned int). Because of this signedness problem, the expression past the return statement to be incorrectly evaluated to 1, which in turn propagates back to stm_unlock() and results in -EINVAL. The correction is very simple, just use the correctly sized data types with correct signedness in the function to make it work as intended. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Simon Glass <sjg@chromium.org> Cc: Jagan Teki <jteki@openedev.com> --- drivers/mtd/spi/spi_flash.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)