sf: Correct data types in stm_is_locked_sr()

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

Quoted 31 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644
@@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len,
 

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 4 lines

drivers/mtd/spi/spi_flash.c +1 -1

 #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?

Quoted 32 lines · Jagan Teki

Quoted 31 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644
@@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len,
 

Quoted 6 lines

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 4 lines

drivers/mtd/spi/spi_flash.c +1 -1

 #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.

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 1 line

This is now same as Linux too.

I couldn't find it on l2-mtd and ML as well, it is still uint64_t

Quoted 41 lines · Jagan Teki

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 4 lines

Quoted 1 line

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.

Quoted 42 lines · Marek Vasut

Quoted 41 lines · Jagan Teki

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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?

Quoted 43 lines · Jagan Teki

Quoted 42 lines · Marek Vasut

Quoted 41 lines · Jagan Teki

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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_.
"

Quoted 57 lines · Marek Vasut

Quoted 43 lines · Jagan Teki

Quoted 42 lines · Marek Vasut

Quoted 41 lines · Jagan Teki

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 7 lines

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.

Quoted 66 lines · Jagan Teki

Quoted 64 lines · Marek Vasut

Quoted 43 lines · Jagan Teki

Quoted 42 lines · Marek Vasut

Quoted 41 lines · Jagan Teki

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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_.
"

Quoted 2 lines

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 Marek,

Quoted 67 lines · Marek Vasut

Quoted 66 lines · Jagan Teki

Quoted 64 lines · Marek Vasut

Quoted 43 lines · Jagan Teki

Quoted 42 lines · Marek Vasut

Quoted 41 lines · Jagan Teki

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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_.
"

Quoted 4 lines

Quoted 2 lines

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.

Quoted 31 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644
@@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len,
 

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 4 lines

drivers/mtd/spi/spi_flash.c +1 -1

 #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?

Jagan.

Hello Jagan,

Quoted 32 lines · Jagan Teki

Quoted 31 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644
@@ -665,7 +665,7 @@ int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len,
 

Quoted 6 lines

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 4 lines

drivers/mtd/spi/spi_flash.c +1 -1

 #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.

Hi Albert,

Quoted 43 lines · Albert ARIBAUD

Hello Jagan,

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 2 lines

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 ?

Quoted 2 lines

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!

Signature · 1 line

Jagan.

Hello Jagan,

Quoted 46 lines · Jagan Teki

Hi Albert,

Quoted 43 lines · Albert ARIBAUD

Hello Jagan,

Quoted 38 lines · Jagan Teki

Quoted 30 lines · Marek Vasut

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 <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jagan Teki <[email protected]>

---
 drivers/mtd/spi/spi_flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/spi/spi_flash.c +0 -0

index 2ae2e3c..44d9e9b 100644

drivers/mtd/spi/spi_flash.c int sst_write_bp(struct spi_flash *flash, u32 offset, size_t len, · line 668

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +1 -1

@@ -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?

Quoted 4 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

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.