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

login
register
mail settings
Submitter Lubomir Rintel
Date Jan. 30, 2013, 11:46 p.m.
Message ID <1359589584-19846-1-git-send-email-lkundrak@v3.sk>
Download mbox | patch
Permalink /patch/217089/
State Changes Requested
Delegated to: Wolfgang Denk
Headers show

Comments

Lubomir Rintel - Jan. 30, 2013, 11:46 p.m.
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(-)
Wolfgang Denk - Jan. 31, 2013, 6:48 a.m.
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
Lubomir Rintel - Jan. 31, 2013, 11:02 a.m.
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
Wolfgang Denk - Jan. 31, 2013, 2:10 p.m.
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
Lubomir Rintel - Feb. 6, 2013, 11:04 p.m.
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
Wolfgang Denk - Feb. 6, 2013, 11:21 p.m.
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
Lubomir Rintel - Feb. 7, 2013, 1:42 p.m.
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
Wolfgang Denk - Feb. 7, 2013, 4:58 p.m.
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

Patch

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