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