[U-Boot] [PATCH] env: Allow accessing non-mtd devices
In certain cases, memory device is present as flat file or block device (via mmc or mtdblock layer). Do not attempt MTD operations against it. --- tools/env/fw_env.c | 21 ++++++++++++++++----- tools/env/fw_env.config | 3 +++ 2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 37b60b8..0d8052d 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, ioctl (fd, MEMUNLOCK, &erase);
/* Dataflash does not need an explicit erase cycle */ - if (mtd_type != MTD_DATAFLASH) + if (mtd_type && mtd_type != MTD_DATAFLASH) if (ioctl (fd, MEMERASE, &erase) != 0) { fprintf (stderr, "MTD erase error on %s: %s\n", DEVNAME (dev), @@ -949,23 +949,34 @@ static int flash_write (int fd_current, int fd_target, int dev_target) static int flash_read (int fd) { struct mtd_info_user mtdinfo; + struct stat st; int rc;
- rc = ioctl (fd, MEMGETINFO, &mtdinfo); + rc = fstat (fd, &st); if (rc < 0) { - perror ("Cannot get MTD information"); + perror ("Cannot access MTD device"); return -1; }
+ if (S_ISCHR (st.st_mode)) { + rc = ioctl (fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + perror ("Cannot get MTD information"); + return -1; + } + } else { + memset (&mtdinfo, 0, sizeof (mtdinfo)); + } + if (mtdinfo.type != MTD_NORFLASH && mtdinfo.type != MTD_NANDFLASH && - mtdinfo.type != MTD_DATAFLASH) { + mtdinfo.type != MTD_DATAFLASH && + mtdinfo.type) { fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type); return -1; }
DEVTYPE(dev_current) = mtdinfo.type; - rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE, DEVOFFSET (dev_current), mtdinfo.type);
diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config index 8e21d5a..c086512 100644 --- a/tools/env/fw_env.config +++ b/tools/env/fw_env.config @@ -17,3 +17,6 @@
# NAND example #/dev/mtd0 0x4000 0x4000 0x20000 2 + +# Block device example +#/dev/mmcblk0 0xc0000 0x20000
Dear Lubomir Rintel,
In message 1359589584-19846-1-git-send-email-lkundrak@v3.sk you wrote:
In certain cases, memory device is present as flat file or block device (via mmc or mtdblock layer). Do not attempt MTD operations against it.
What exactly would be such cases?
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 37b60b8..0d8052d 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, ioctl (fd, MEMUNLOCK, &erase);
/* Dataflash does not need an explicit erase cycle */
if (mtd_type != MTD_DATAFLASH)
if (mtd_type && mtd_type != MTD_DATAFLASH)
This change appears to be redundant. If mtd_type is null, then this is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it?
- rc = ioctl (fd, MEMGETINFO, &mtdinfo);
- rc = fstat (fd, &st); if (rc < 0) {
perror ("Cannot get MTD information");
perror ("Cannot access MTD device");
I don't understand this. You talk about a MTD device here, but expect that MEMGETINFO will not work on it? Please explain in which exact circumstances such a situation wouldhappen.
if (mtdinfo.type != MTD_NORFLASH && mtdinfo.type != MTD_NANDFLASH &&
mtdinfo.type != MTD_DATAFLASH) {
mtdinfo.type != MTD_DATAFLASH &&mtdinfo.type) {
Again, this last line appears to be redundant.
}
DEVTYPE(dev_current) = mtdinfo.type;
- rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE,
Please don't make such unrelated white space changes in this commit.
diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config index 8e21d5a..c086512 100644 --- a/tools/env/fw_env.config +++ b/tools/env/fw_env.config @@ -17,3 +17,6 @@
# NAND example #/dev/mtd0 0x4000 0x4000 0x20000 2
+# Block device example +#/dev/mmcblk0 0xc0000 0x20000
I don't see why one would use that. Please elucidate.
Please also make sure to run your patch through checkpatch - it catches a number of "space prohibited between function name and open parenthesis" warnings and tells you that your Signed-off-by: line is missing.
Best regards,
Wolfgang Denk
On Thu, 2013-01-31 at 07:48 +0100, Wolfgang Denk wrote:
Dear Lubomir Rintel,
In message 1359589584-19846-1-git-send-email-lkundrak@v3.sk you wrote:
In certain cases, memory device is present as flat file or block device (via mmc or mtdblock layer). Do not attempt MTD operations against it.
What exactly would be such cases?
Mine is a Kobo Mini e-book reader, which is basically Freescale MX50 with the only storage there being an MMC/SD card (removable from a slot if disassembled), which contains uboot and its environment as well as partition table, root and storage volume.
Apart from wiring a serial console, running fw_* tools seems to be about the only way to modify kernel command line on that device. Also, I can imagine that it would be useful to prepare a flat file image on a different computer and copy it to the image afterwards.
Please also make sure to run your patch through checkpatch - it catches a number of "space prohibited between function name and open parenthesis" warnings
I tried to stick with the coding style already present in the file. No problem though, I'll follow up with an updated patch later.
and tells you that your Signed-off-by: line is missing.
Noted, will fix.
Thank you!
Lubo
Dear Lubomir Rintel,
In message 1359630144.16475.6.camel@hobbes you wrote:
Mine is a Kobo Mini e-book reader, which is basically Freescale MX50 with the only storage there being an MMC/SD card (removable from a slot if disassembled), which contains uboot and its environment as well as partition table, root and storage volume.
OK - but you do not comment on my remarks about the actual code?
Best regards,
Wolfgang Denk
On Thu, 2013-01-31 at 15:10 +0100, Wolfgang Denk wrote:
Dear Lubomir Rintel,
In message 1359630144.16475.6.camel@hobbes you wrote:
Mine is a Kobo Mini e-book reader, which is basically Freescale MX50 with the only storage there being an MMC/SD card (removable from a slot if disassembled), which contains uboot and its environment as well as partition table, root and storage volume.
OK - but you do not comment on my remarks about the actual code?
On Thu, 2013-01-31 at 07:48 +0100, Wolfgang Denk wrote:
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 37b60b8..0d8052d 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, ioctl (fd, MEMUNLOCK, &erase);
/* Dataflash does not need an explicit erase cycle */
if (mtd_type != MTD_DATAFLASH)
if (mtd_type && mtd_type != MTD_DATAFLASH)This change appears to be redundant. If mtd_type is null, then this is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it?
No. We don't want the erase ioctl to be called for non-MTD devices and files (where mtd_type is null).
- rc = ioctl (fd, MEMGETINFO, &mtdinfo);
- rc = fstat (fd, &st); if (rc < 0) {
perror ("Cannot get MTD information");
perror ("Cannot access MTD device");I don't understand this. You talk about a MTD device here, but expect that MEMGETINFO will not work on it? Please explain in which exact circumstances such a situation wouldhappen.
The error message (mention of MTD at that point) is incorrect. fstat() is called to determine whether the device is a character device (such as MTD devices -- MEMGETINFO call will follow) or not (it might be a blockdevice or flat file).
if (mtdinfo.type != MTD_NORFLASH && mtdinfo.type != MTD_NANDFLASH &&
mtdinfo.type != MTD_DATAFLASH) {
mtdinfo.type != MTD_DATAFLASH &&mtdinfo.type) {Again, this last line appears to be redundant.
The same response again -- if the type is nul, then the device is not a flash device at all.
}
DEVTYPE(dev_current) = mtdinfo.type;
- rc = flash_read_buf(dev_current, fd, environment.image, CUR_ENVSIZE,
Please don't make such unrelated white space changes in this commit.
That was a mistake.
Have a nice day! -- Lubo
In certain cases, memory device is present as flat file or block device (via mmc or mtdblock layer). Do not attempt MTD operations against it.
Signed-off-by: Lubomir Rintel lkundrak@v3.sk --- tools/env/fw_env.c | 20 ++++++++++++++++---- tools/env/fw_env.config | 3 +++ 2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 37b60b8..72d77a9 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count, ioctl (fd, MEMUNLOCK, &erase);
/* Dataflash does not need an explicit erase cycle */ - if (mtd_type != MTD_DATAFLASH) + if (mtd_type && mtd_type != MTD_DATAFLASH) if (ioctl (fd, MEMERASE, &erase) != 0) { fprintf (stderr, "MTD erase error on %s: %s\n", DEVNAME (dev), @@ -949,17 +949,29 @@ static int flash_write (int fd_current, int fd_target, int dev_target) static int flash_read (int fd) { struct mtd_info_user mtdinfo; + struct stat st; int rc;
- rc = ioctl (fd, MEMGETINFO, &mtdinfo); + rc = fstat(fd, &st); if (rc < 0) { - perror ("Cannot get MTD information"); + perror("Cannot access the device file"); return -1; }
+ if (S_ISCHR(st.st_mode)) { + rc = ioctl(fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + perror("Cannot get MTD information"); + return -1; + } + } else { + memset(&mtdinfo, 0, sizeof(mtdinfo)); + } + if (mtdinfo.type != MTD_NORFLASH && mtdinfo.type != MTD_NANDFLASH && - mtdinfo.type != MTD_DATAFLASH) { + mtdinfo.type != MTD_DATAFLASH && + mtdinfo.type) { fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type); return -1; } diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config index 8e21d5a..c086512 100644 --- a/tools/env/fw_env.config +++ b/tools/env/fw_env.config @@ -17,3 +17,6 @@
# NAND example #/dev/mtd0 0x4000 0x4000 0x20000 2 + +# Block device example +#/dev/mmcblk0 0xc0000 0x20000
Dear Lubomir Rintel,
In message 1360191923-4688-1-git-send-email-lkundrak@v3.sk you wrote:
In certain cases, memory device is present as flat file or block device (via mmc or mtdblock layer). Do not attempt MTD operations against it.
Signed-off-by: Lubomir Rintel lkundrak@v3.sk
tools/env/fw_env.c | 20 ++++++++++++++++---- tools/env/fw_env.config | 3 +++ 2 files changed, 19 insertions(+), 4 deletions(-)
Arghhh! NAK.
There is no patch version, no history of changes, nothing.
Please read http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions and follow the rules.
Please also see my previous review comments.
Also:
- rc = ioctl (fd, MEMGETINFO, &mtdinfo);
- rc = fstat(fd, &st); if (rc < 0) {
perror ("Cannot get MTD information");
perror("Cannot access the device file");
This error message is still misleading (as you did not use any access(2) system call in your code); also, the use of perror() is - let's say - a bit unusual (not your fault in the first place) and should be fixed; it would be more helpful to print the actual file name here.
Wolfgang Denk
In certain cases, memory device is present as flat file or block device (via mmc or mtdblock layer). Do not attempt MTD operations against it.
Signed-off-by: Lubomir Rintel lkundrak@v3.sk --- Changes for v2: - Coding Style cleanup - Clarified an error message Changes for v3: - Used MTD_ABSENT macro to represent non-MTD devices - Cleaned up places when zero was used instead of MTD_ABSENT Changes for v4: - Further clarification to failed fstat() error message - Improved flash_read() error handling to include file names
tools/env/fw_env.c | 34 +++++++++++++++++++++++----------- tools/env/fw_env.config | 3 +++ 2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 0c68e24..bf30234 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -836,9 +836,9 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
erase.start = blockstart; ioctl (fd, MEMUNLOCK, &erase); - - /* Dataflash does not need an explicit erase cycle */ - if (mtd_type != MTD_DATAFLASH) + /* These do not need an explicit erase cycle */ + if (mtd_type != MTD_ABSENT && + mtd_type != MTD_DATAFLASH) if (ioctl (fd, MEMERASE, &erase) != 0) { fprintf (stderr, "MTD erase error on %s: %s\n", DEVNAME (dev), @@ -949,21 +949,33 @@ static int flash_write (int fd_current, int fd_target, int dev_target) static int flash_read (int fd) { struct mtd_info_user mtdinfo; + struct stat st; int rc;
- rc = ioctl (fd, MEMGETINFO, &mtdinfo); + rc = fstat(fd, &st); if (rc < 0) { - fprintf(stderr, "Cannot get MTD information for %s\n", + fprintf(stderr, "Cannot stat the file %s\n", DEVNAME(dev_current)); return -1; }
- if (mtdinfo.type != MTD_NORFLASH && - mtdinfo.type != MTD_NANDFLASH && - mtdinfo.type != MTD_DATAFLASH) { - fprintf (stderr, "Unsupported flash type %u on %s\n", - mtdinfo.type, DEVNAME(dev_current)); - return -1; + if (S_ISCHR(st.st_mode)) { + rc = ioctl(fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + fprintf(stderr, "Cannot get MTD information for %s\n", + DEVNAME(dev_current)); + return -1; + } + if (mtdinfo.type != MTD_NORFLASH && + mtdinfo.type != MTD_NANDFLASH && + mtdinfo.type != MTD_DATAFLASH) { + fprintf (stderr, "Unsupported flash type %u on %s\n", + mtdinfo.type, DEVNAME(dev_current)); + return -1; + } + } else { + memset(&mtdinfo, 0, sizeof(mtdinfo)); + mtdinfo.type = MTD_ABSENT; }
DEVTYPE(dev_current) = mtdinfo.type; diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config index 8e21d5a..c086512 100644 --- a/tools/env/fw_env.config +++ b/tools/env/fw_env.config @@ -17,3 +17,6 @@
# NAND example #/dev/mtd0 0x4000 0x4000 0x20000 2 + +# Block device example +#/dev/mmcblk0 0xc0000 0x20000
Dear Lubomir Rintel,
In message 1360191866.3594.10.camel@unicorn you wrote:
if (mtd_type != MTD_DATAFLASH)
if (mtd_type && mtd_type != MTD_DATAFLASH)This change appears to be redundant. If mtd_type is null, then this is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it?
No. We don't want the erase ioctl to be called for non-MTD devices and files (where mtd_type is null).
I see. But you are misusing mtd_type.
You should define something like MTD_NO_FLASH or so, and use that.
perror ("Cannot get MTD information");
perror ("Cannot access MTD device");I don't understand this. You talk about a MTD device here, but expect that MEMGETINFO will not work on it? Please explain in which exact circumstances such a situation wouldhappen.
The error message (mention of MTD at that point) is incorrect. fstat()
So it needs to be fixed.
if (mtdinfo.type != MTD_NORFLASH && mtdinfo.type != MTD_NANDFLASH &&
mtdinfo.type != MTD_DATAFLASH) {
mtdinfo.type != MTD_DATAFLASH &&mtdinfo.type) {Again, this last line appears to be redundant.
The same response again -- if the type is nul, then the device is not a flash device at all.
See above. Please make the code sonsistent and define a proper name for this type.
Best regards,
Wolfgang Denk
On Thu, 2013-02-07 at 00:21 +0100, Wolfgang Denk wrote:
Dear Lubomir Rintel,
In message 1360191866.3594.10.camel@unicorn you wrote:
if (mtd_type != MTD_DATAFLASH)
if (mtd_type && mtd_type != MTD_DATAFLASH)This change appears to be redundant. If mtd_type is null, then this is already caught iun te test mtd_type != MTD_DATAFLASH, isn't it?
No. We don't want the erase ioctl to be called for non-MTD devices and files (where mtd_type is null).
I see. But you are misusing mtd_type.
You should define something like MTD_NO_FLASH or so, and use that.
I found it a bit more abusive to use a MTD_* macro in mtd_type variable for something what is not actually a type of a MTD device, specially when a change in MTD ABI would be needed.
Maybe passing a NULL mtd_info_user pointer to flash_{read,write}_buf() instead of zero type (which happens to be MTD_ABSENT, and, as you pointed out, a misuse) would make more sense for a non-MTD file?
perror ("Cannot get MTD information");
perror ("Cannot access MTD device");I don't understand this. You talk about a MTD device here, but expect that MEMGETINFO will not work on it? Please explain in which exact circumstances such a situation wouldhappen.
The error message (mention of MTD at that point) is incorrect. fstat()
So it needs to be fixed.
Hopefully fixed in a follow-up patch. I'll follow up with another one (this time with proper changlog and versioning, sorry for that) once an acceptable solution to the issue above is agreed upon.
Thanks, -- Lubo
Dear Lubomir,
In message 1360244552.29426.9.camel@hobbes you wrote:
You should define something like MTD_NO_FLASH or so, and use that.
I found it a bit more abusive to use a MTD_* macro in mtd_type variable for something what is not actually a type of a MTD device, specially when a change in MTD ABI would be needed.
But that's exactly what you are doing here, just in a way that it is not even visible. By assigning a mtd_type value of 0, you are setting it to MTD_ABSENT - but you don't write MTD_ABSENT.
This is even more dangerous.
Maybe passing a NULL mtd_info_user pointer to flash_{read,write}_buf() instead of zero type (which happens to be MTD_ABSENT, and, as you pointed out, a misuse) would make more sense for a non-MTD file?
I don't have any real problems with using MTD_ABSENT - it even makes kind of sense.
Best regards,
Wolfgang Denk
In certain cases, memory device is present as flat file or block device (via mmc or mtdblock layer). Do not attempt MTD operations against it.
Signed-off-by: Lubomir Rintel lkundrak@v3.sk --- Changes for v2: - Coding Style cleanup - Clarified an error message Changes for v3: - Used MTD_ABSENT macro to represent non-MTD devices - Cleaned up places when zero was used instead of MTD_ABSENT
tools/env/fw_env.c | 32 ++++++++++++++++++++++---------- tools/env/fw_env.config | 3 +++ 2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 37b60b8..94a790d 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -836,9 +836,9 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
erase.start = blockstart; ioctl (fd, MEMUNLOCK, &erase); - - /* Dataflash does not need an explicit erase cycle */ - if (mtd_type != MTD_DATAFLASH) + /* These do not need an explicit erase cycle */ + if (mtd_type != MTD_ABSENT && + mtd_type != MTD_DATAFLASH) if (ioctl (fd, MEMERASE, &erase) != 0) { fprintf (stderr, "MTD erase error on %s: %s\n", DEVNAME (dev), @@ -949,19 +949,31 @@ static int flash_write (int fd_current, int fd_target, int dev_target) static int flash_read (int fd) { struct mtd_info_user mtdinfo; + struct stat st; int rc;
- rc = ioctl (fd, MEMGETINFO, &mtdinfo); + rc = fstat(fd, &st); if (rc < 0) { - perror ("Cannot get MTD information"); + perror("Cannot access the device file"); return -1; }
- if (mtdinfo.type != MTD_NORFLASH && - mtdinfo.type != MTD_NANDFLASH && - mtdinfo.type != MTD_DATAFLASH) { - fprintf (stderr, "Unsupported flash type %u\n", mtdinfo.type); - return -1; + if (S_ISCHR(st.st_mode)) { + rc = ioctl(fd, MEMGETINFO, &mtdinfo); + if (rc < 0) { + perror("Cannot get MTD information"); + return -1; + } + if (mtdinfo.type != MTD_NORFLASH && + mtdinfo.type != MTD_NANDFLASH && + mtdinfo.type != MTD_DATAFLASH) { + fprintf(stderr, "Unsupported flash type %u\n", + mtdinfo.type); + return -1; + } + } else { + memset(&mtdinfo, 0, sizeof(mtdinfo)); + mtdinfo.type = MTD_ABSENT; }
DEVTYPE(dev_current) = mtdinfo.type; diff --git a/tools/env/fw_env.config b/tools/env/fw_env.config index 8e21d5a..c086512 100644 --- a/tools/env/fw_env.config +++ b/tools/env/fw_env.config @@ -17,3 +17,6 @@
# NAND example #/dev/mtd0 0x4000 0x4000 0x20000 2 + +# Block device example +#/dev/mmcblk0 0xc0000 0x20000
participants (2)
-
Lubomir Rintel -
Wolfgang Denk