[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Lubomir Rintel]

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Lubomir Rintel]

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Lubomir Rintel]

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Lubomir Rintel]

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Wolfgang Denk]

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

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Wolfgang Denk]

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");
>  		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

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Lubomir Rintel]

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

[U-Boot] [PATCH] env: Allow accessing non-mtd devices

[Wolfgang Denk]

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

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

[U-Boot] [PATCH v3] env: Allow accessing non-mtd devices

[Lubomir Rintel]

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

[U-Boot] [PATCH v4] env: Allow accessing non-mtd devices

[Lubomir Rintel]

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