From: Lubomir Rintel <lkundrak@v3.sk>
Date: Thu, 07 Feb 2013 14:42:32 +0100
Subject: [U-Boot] [PATCH] env: Allow accessing non-mtd devices
In-Reply-To: <20130206232121.2D31B200564@gemini.denx.de>
References: <1359589584-19846-1-git-send-email-lkundrak@v3.sk>
	<20130131064855.D072E20059E@gemini.denx.de>
	<1359630144.16475.6.camel@hobbes>
	<20130131141021.58DE520059E@gemini.denx.de>
	<1360191866.3594.10.camel@unicorn>
	<20130206232121.2D31B200564@gemini.denx.de>
Message-ID: <1360244552.29426.9.camel@hobbes>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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