ubiformat: make it work on mtd parts > 2GiB

I have here a mtd part which is 3 GiB with a flash page size of 256KiB. The 2GiB limit is at erase block 8192. In mtd_is_bad() the computation for the MEMGETBADBLOCK ioctl() looks like the following:

| seek = eb * mtd->eb_size;

with both eb and mtd->eb_size being a signed int results in seek being a signed result. Therefore I changed the type from signed to unsigned int.

The _FILE_OFFSET_BITS=64 define is required to switch off_t from 32bit to 64bit an 32bit systems. This is required in order to keep using lseek() as lseek64 on 32bit system. Without this change lseek() in mtd_read() is called with a 32bit value with most significat bit set and the kernel performs a sign extension for the 64bit value which is used in the mtd layer.

The last change also changes the size of the parameter which is passed to the MEMGETBADBLOCK ioctl() from 32 to 64bit. The counter part in kernel is also defined as loff_t which is of type __kernel_loff_t and this is "long long". So this must have been broken for a while unless I missed something.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 common.mk                            |    2 +-
 ubi-utils/new-utils/include/libmtd.h |   10 ++++++----
 ubi-utils/new-utils/src/libmtd.c     |   20 +++++++++++---------
 ubi-utils/new-utils/src/libscan.c    |    5 +++--
 ubi-utils/new-utils/src/ubiformat.c  |    9 ++++++---
 5 files changed, 27 insertions(+), 19 deletions(-)

common.mk +1 -1

index 77d28bf..65fc1cc 100644
@@ -2,7 +2,7 @@ CC := $(CROSS)gcc
 AR := $(CROSS)ar
 RANLIB := $(CROSS)ranlib
 CFLAGS ?= -O2 -g
-CFLAGS += -Wall -Wwrite-strings -W
+CFLAGS += -Wall -Wwrite-strings -W -D_FILE_OFFSET_BITS=64
 
 DESTDIR ?= /usr/local
 PREFIX=/usr

ubi-utils/new-utils/include/libmtd.h +0 -0

index d3c6a63..94ccd45 100644
@@ -61,10 +61,12 @@ struct mtd_info
 };
 
 int mtd_get_info(const char *node, struct mtd_info *mtd);

Quoted 8 lines · Sebastian Andrzej Siewior

I have here a mtd part which is 3 GiB with a flash page size of 256KiB. The 2GiB limit is at erase block 8192. In mtd_is_bad() the computation for the MEMGETBADBLOCK ioctl() looks like the following:

| seek = eb * mtd->eb_size;

with both eb and mtd->eb_size being a signed int results in seek being a signed result. Therefore I changed the type from signed to unsigned int.

If you make "seek" to be 64 bit, and cast one of the multipliers to off_t, you should be fine.

Quoted 42 lines

The _FILE_OFFSET_BITS=64 define is required to switch off_t from 32bit to 64bit an 32bit systems. This is required in order to keep using lseek() as lseek64 on 32bit system. Without this change lseek() in mtd_read() is called with a 32bit value with most significat bit set and the kernel performs a sign extension for the 64bit value which is used in the mtd layer.

The last change also changes the size of the parameter which is passed to the MEMGETBADBLOCK ioctl() from 32 to 64bit. The counter part in kernel is also defined as loff_t which is of type __kernel_loff_t and this is "long long". So this must have been broken for a while unless I missed something.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 common.mk                            |    2 +-
 ubi-utils/new-utils/include/libmtd.h |   10 ++++++----
 ubi-utils/new-utils/src/libmtd.c     |   20 +++++++++++---------
 ubi-utils/new-utils/src/libscan.c    |    5 +++--
 ubi-utils/new-utils/src/ubiformat.c  |    9 ++++++---
 5 files changed, 27 insertions(+), 19 deletions(-)

common.mk +1 -1

index 77d28bf..65fc1cc 100644
@@ -2,7 +2,7 @@ CC := $(CROSS)gcc
 AR := $(CROSS)ar
 RANLIB := $(CROSS)ranlib
 CFLAGS ?= -O2 -g
-CFLAGS += -Wall -Wwrite-strings -W
+CFLAGS += -Wall -Wwrite-strings -W -D_FILE_OFFSET_BITS=64
 
 DESTDIR ?= /usr/local
 PREFIX=/usr

ubi-utils/new-utils/include/libmtd.h +0 -0

index d3c6a63..94ccd45 100644
@@ -61,10 +61,12 @@ struct mtd_info
 };
 

ubi-utils/new-utils/include/libmtd.h struct mtd_info · line 69

Quoted 11 lines

ubi-utils/new-utils/include/libmtd.h +6 -4

 int mtd_get_info(const char *node, struct mtd_info *mtd);
-int mtd_erase(const struct mtd_info *mtd, int eb);
-int mtd_is_bad(const struct mtd_info *mtd, int eb);
-int mtd_read(const struct mtd_info *mtd, int eb, int offs, void *buf, int len);
-int mtd_write(const struct mtd_info *mtd, int eb, int offs, void *buf, int len);
+int mtd_erase(const struct mtd_info *mtd, unsigned int eb);
+int mtd_is_bad(const struct mtd_info *mtd, unsigned int eb);
+int mtd_read(const struct mtd_info *mtd, unsigned int eb, unsigned int offs,
+		void *buf, int len);
+int mtd_write(const struct mtd_info *mtd, unsigned int eb, unsigned int offs,
+		void *buf, int len);

I do not think you need to make eraseblock number and offset to be unsigned. In fact, I'd like them to be signed, because this is the same we have in the kernel (in UBI/UBIFS), and I'd like to be more or less consistent.

Your patch will be twice as short without this change, right?

Quoted 4 lines

I do not think you need to make eraseblock number and offset to be unsigned. In fact, I'd like them to be signed, because this is the same we have in the kernel (in UBI/UBIFS), and I'd like to be more or less consistent.

That is a point. However those things should never be negative so maybe we could change this in kernel.

While we here, I get a couple of "compare between signed and unsigned" warnings from gcc. I tried to clean them up but I end up with huge patches similar to this one. Are you aware of those or you simply don't get them?

Quoted 5 lines · Sebastian Andrzej Siewior

Quoted 4 lines

I do not think you need to make eraseblock number and offset to be unsigned. In fact, I'd like them to be signed, because this is the same we have in the kernel (in UBI/UBIFS), and I'd like to be more or less consistent.

That is a point. However those things should never be negative so maybe we could change this in kernel.

They should not indeed be negative. However, signed numbers are just much more appropriate when you write the code, because you may put an error code there if something goes wrong.

For example, the UBIFS Garbage collector returns the freed eraseblock number in case of success or a negative error code in case of failure. Compare:

int ubifs_garbage_collect(struct ubifs_info *c)

and

int ubifs_garbage_collect(struct ubifs_info *c, unsigned int *leb)

The first prototype is what we have now. The second is what we would have to have to be able to return LEB number and error code.

And vs. changing UBI/UBIFS - that would be really a lot of work.

Anyway, the thing is that 31-bit should be more than enough for eraseblock number.

Quoted 2 lines

While we here, I get a couple of "compare between signed and unsigned" warnings from gcc. I tried to clean them up but I end up with huge patches similar to this one. Are you aware of those or you simply don't get them?

I'd rather shut the warnings up by an gcc option - is there such an option?

Quoted 6 lines

Quoted 3 lines

While we here, I get a couple of "compare between signed and unsigned" warnings from gcc. I tried to clean them up but I end up with huge patches similar to this one. Are you aware of those or you simply don't get them?

I'd rather shut the warnings up by an gcc option - is there such an option?

You are using -W which deprecated and -Wextra is doing the same thing. One of the things -Wextra does is adding -Wsign-compare so you have to add -Wno-sign-compare to stop that.

Quoted 8 lines · Sebastian Andrzej Siewior

ubi-utils/new-utils/src/libmtd.c +0 -0

index aab4b0e..69705dd 100644
@@ -189,7 +189,7 @@ int mtd_is_bad(const struct mtd_info *mtd, int eb)
 	if (!mtd->allows_bb)
 		return 0;
 

ubi-utils/new-utils/src/libmtd.c int mtd_is_bad(const struct mtd_info *mtd, int eb) · line 192

Quoted 2 lines

ubi-utils/new-utils/src/libmtd.c +1 -1

-	seek = eb * mtd->eb_size;
+	seek = (unsigned int)eb * mtd->eb_size;

I believe this should be:

seek = (off_t)eb * mtd->eb_size;

instead.

I believe this should be:

seek = (off_t)eb * mtd->eb_size;

I have here a mtd part which is 3 GiB with a flash page size of 256KiB. The 2GiB limit is at erase block 8192. In mtd_is_bad() the computation for the MEMGETBADBLOCK ioctl() looks like the following:

Signed-off-by Sebastian Andrzej Siewior <[email protected]>