env: Allow accessing non-mtd devices

PATCH New Lubomir Rintel 2 participants 12 messages Jan 30, 2013 – Feb 10, 2013

Share & tools

Lubomir Rintel

author

2013-01-30 23:46 UTC #0 lore raw

PATCH 1/1 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(-)

tools/env/fw_env.c +16 -5

index 37b60b8..0d8052d 100644
@@ -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);

tools/env/fw_env.config +3 -0

index 8e21d5a..c086512 100644
@@ -17,3 +17,6 @@
 
 # NAND example
 #/dev/mtd0		0x4000		0x4000		0x20000			2
+
+# Block device example
+#/dev/mmcblk0		0xc0000		0x20000

Signature · 1 line

1.7.1

Wolfgang Denk

2013-01-31 06:48 UTC #1 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

Dear Lubomir Rintel,

Quoted 2 lines · In message <[email protected]> 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?

tools/env/fw_env.c static int flash_write_buf (int dev, int fd, void *buf, size_t count, · line 841

Quoted 10 lines

tools/env/fw_env.c +1 -1

index 37b60b8..0d8052d 100644
@@ -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?

tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 957

Quoted 5 lines

tools/env/fw_env.c +2 -2

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

tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 974

Quoted 5 lines

tools/env/fw_env.c +2 -1

 	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.

tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 980

Quoted 5 lines

tools/env/fw_env.c +0 -1

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

tools/env/fw_env.config line 22

Quoted 11 lines

tools/env/fw_env.config +3 -0

index 8e21d5a..c086512 100644
@@ -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

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Conceptual integrity in turn dictates that the  design  must  proceed
from  one  mind,  or  from  a  very small number of agreeing resonant
minds.               - Frederick Brooks Jr., "The Mythical Man Month"

Lubomir Rintel

author

2013-01-31 11:02 UTC #2 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

Quoted 7 lines · Wolfgang Denk

Dear Lubomir Rintel,
Quoted 2 lines · In message <[email protected]> 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.

Quoted 3 lines

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.

Quoted 2 lines

and tells you that your Signed-off-by: line is
missing.

Noted, will fix.

Thank you!

Lubo

Wolfgang Denk

2013-01-31 14:10 UTC #3 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

Dear Lubomir Rintel,

Quoted 5 lines · 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

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The amount of time between slipping on the peel and  landing  on  the
pavement is precisely 1 bananosecond.

Lubomir Rintel

author

2013-02-06 23:04 UTC #4 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

Quoted 10 lines · Wolfgang Denk

Dear Lubomir Rintel,
Quoted 5 lines · 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?

Quoted 7 lines · Wolfgang Denk

Quoted 7 lines

tools/env/fw_env.c +0 -0

index 37b60b8..0d8052d 100644
@@ -838,7 +838,7 @@ static int flash_write_buf (int dev, int fd, void *buf, size_t count,
 		ioctl (fd, MEMUNLOCK, &erase);

Quoted 6 lines

tools/env/fw_env.c static int flash_write_buf (int dev, int fd, void *buf, size_t count, · line 841
Quoted 3 lines
tools/env/fw_env.c +1 -1
 		/* 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).

Quoted 9 lines

tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 957
Quoted 5 lines
tools/env/fw_env.c +2 -2
-	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).

Quoted 7 lines

tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 974
Quoted 5 lines
tools/env/fw_env.c +2 -1
 	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.

Quoted 8 lines

tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 980
Quoted 6 lines
tools/env/fw_env.c +0 -1
 	}
 
 	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

Lubomir Rintel

author

2013-02-06 23:05 UTC #5 lore raw

PATCH 1/1 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.

Signed-off-by Lubomir Rintel <[email protected]>

---
 tools/env/fw_env.c      |   20 ++++++++++++++++----
 tools/env/fw_env.config |    3 +++
 2 files changed, 19 insertions(+), 4 deletions(-)

tools/env/fw_env.c +16 -4

index 37b60b8..72d77a9 100644
@@ -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;
 	}

tools/env/fw_env.config +3 -0

index 8e21d5a..c086512 100644
@@ -17,3 +17,6 @@
 
 # NAND example
 #/dev/mtd0		0x4000		0x4000		0x20000			2
+
+# Block device example
+#/dev/mmcblk0		0xc0000		0x20000

Signature · 1 line

1.7.1

Wolfgang Denk

2013-02-06 23:27 UTC #6 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

Dear Lubomir Rintel,

Quoted 8 lines · In message <[email protected]> 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 <[email protected]>
---
 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:

tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 959

Quoted 7 lines

tools/env/fw_env.c +2 -2

-	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;
 	}

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

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A conservative is a man with two perfectly good legs  who  has  never
learned to walk.                              - Franklin D. Roosevelt

Lubomir Rintel

author

2013-02-10 11:02 UTC #7 lore raw

PATCH v4 1/1 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.

Signed-off-by Lubomir Rintel <[email protected]>

---
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(-)

tools/env/fw_env.c +23 ~~-11~~

index 0c68e24..bf30234 100644
@@ -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;

tools/env/fw_env.config +3 -0

index 8e21d5a..c086512 100644
@@ -17,3 +17,6 @@
 
 # NAND example
 #/dev/mtd0		0x4000		0x4000		0x20000			2
+
+# Block device example
+#/dev/mmcblk0		0xc0000		0x20000

Signature · 1 line

1.7.1

Wolfgang Denk

2013-02-06 23:21 UTC #8 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

In reply to Lubomir Rintel

Dear Lubomir Rintel,

Quoted 9 lines · In message <1360191866.3594.10.camel@unicorn> you wrote:

Quoted 6 lines
tools/env/fw_env.c static int flash_write_buf (int dev, int fd, void *buf, size_t count, · line 841
Quoted 3 lines
tools/env/fw_env.c +1 -1
-		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.

Quoted 8 lines

Quoted 6 lines
tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 957
Quoted 2 lines
tools/env/fw_env.c +1 -1
-		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.

Quoted 10 lines

Quoted 7 lines
tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 974
Quoted 5 lines
tools/env/fw_env.c +2 -1
 	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

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If A equals success, then the formula is A = X + Y + Z. X is work.  Y
is play. Z is keep your mouth shut.                 - Albert Einstein

Lubomir Rintel

author

2013-02-07 13:42 UTC #9 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

Quoted 10 lines · Wolfgang Denk

Dear Lubomir Rintel,
Quoted 7 lines · In message <1360191866.3594.10.camel@unicorn> you wrote:
Quoted 6 lines
tools/env/fw_env.c static int flash_write_buf (int dev, int fd, void *buf, size_t count, · line 841
Quoted 3 lines
tools/env/fw_env.c +1 -1
-		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?

Quoted 6 lines

Quoted 2 lines
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?

Quoted 11 lines

Quoted 9 lines
Quoted 7 lines
tools/env/fw_env.c static int flash_write (int fd_current, int fd_target, int dev_target) · line 957
Quoted 3 lines
tools/env/fw_env.c +1 -1
-		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

Wolfgang Denk

2013-02-07 16:58 UTC #10 lore raw

PATCH 1/1 env: Allow accessing non-mtd devices

Dear Lubomir,

Quoted 6 lines · In message <1360244552.29426.9.camel@hobbes> you wrote:

Quoted 2 lines
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.

Quoted 3 lines

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

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Beware of programmers carrying screwdrivers."      - Chip Salzenberg

Lubomir Rintel

author

2013-02-08 08:08 UTC #11 lore raw

PATCH v3 1/1 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.

Signed-off-by Lubomir Rintel <[email protected]>

 tools/env/fw_env.c      |   32 ++++++++++++++++++++++----------
 tools/env/fw_env.config |    3 +++
 2 files changed, 25 insertions(+), 10 deletions(-)

tools/env/fw_env.c +22 ~~-10~~

index 37b60b8..94a790d 100644
@@ -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;

tools/env/fw_env.config +3 -0

index 8e21d5a..c086512 100644
@@ -17,3 +17,6 @@
 
 # NAND example
 #/dev/mtd0		0x4000		0x4000		0x20000			2
+
+# Block device example
+#/dev/mmcblk0		0xc0000		0x20000

Signature · 1 line

1.7.1