MTD git repository status

MTD git repository status

[Thomas Gleixner]

Hi folks,

I've updated the mtd git repository hopefully in time for 2.6.15.

http://www.kernel.org/git/?p=linux/kernel/git/tglx/mtd-2.6.git;a=summary

	tglx

Following changes are not included into the update:

- [JFFS2]  Erase Block Header(EBH)
	Needs proper testing and stabilizing first

- [MTD] NAND: OOB changes
	Needs more thoughts. I'm particularly unhappy with the introduction of new 
functions and the still pending problem of the automatic placement/retrieval 
of oob data. The change should subsume and/or extend the oob_info 
functionality rather than introducing a parallel solution.

- [MTD] NAND: nandsim
	There is still the big Kconfig clutter. For testing environments it _is_ 
sufficient to provide all this information as module parameters. No need to 
bloat the config file

- [MTD] ONENAND: Simulator
	Is depending on arch/??? probably ARM and uses some obscure __iomem* 
constructs.

Can the authors please shed some light on the status of following drivers:

- [MTD] maps mphysram.c
	It needs a bunch of coding style cleanups.
	
- [MTD] devices ramtd.c
	Needs probably a go on the fixed size allocation already marked with 
	/* FIXME */

- [MTD] ssfdc.c
	AFAIK the driver is mostly functional. What's missing ?
	It needs a bunch of coding style cleanups.

Re: MTD git repository status

[Thomas Gleixner]

On Monday 07 November 2005 19:15, Thomas Gleixner wrote:
> I've updated the mtd git repository hopefully in time for 2.6.15.

Some rant about MTD CVS commits

I asked some time ago for some discipline regarding the CVS usage.

Some MTD developers gracefully provide

- separate commits for non related changes
- patches without trailing white spaces
- syntactically correct commit messages
- understandable short and long comments

	Thanks


Some others use the CVS down to their whim.

Is it so hard to comply to some simple rules ?

1. Commit after testing and do not abuse the CVS as your private trash bin

If you have the urge to document your hacking sessions, do this in your 
private CVS or create a branch in the MTD-CVS preferably named 
mtd-yourname-tinkerbox

2. Check for trailing white space _before_ you commit.

3. Do _not_ commit changes which add 
   a)	// old code or
	/* old code */
   b)	#if 0
		old code
	#endif

   a) is a no no
   b) use when such an exclude is necessary:
	/* Comment why this is disabled */
	#ifdef ENABLE_BROKEN_FEATURE
	#endif

4. Commit unrelated changes in separate commits

5. Use the requested commit message format
   [MTD] <core, chips, maps, nand, onenand> <file>: Short description
   or
   [JFFS2] Short description
   <Blank line>
   <Long description>	

6. Write understandable comments, which give an information what the commit is 
doing, rather than some sloppy blurb. Using cryptic abbreviations is also not 
really helpful. See also linux/Documentation/SubmittingPatches


Some example commits for illustration:

---------------------------------------------------------

[JFFS2] minor refinements
[JFFS2] remove forgotten stuff

Really descriptive !

---------------------------------------------------------

Add support for "4G Systems MTX-1 Flash device", better known as meshcube.
Modified Files:
        Kconfig Makefile.common mphysmap.c
...

This is obviously a patch for non kernel parts of the CVS. Err, wait....

Depends mphysmap.c on MTX-1 support or is it the other way round ?

---------------------------------------------------------

[JFFS2] Fix debugging stuff here as well

Here ? In Timbuktu ?

---------------------------------------------------------

Commits with commit messages consisting of 3 empty lines

Is this a guessing game ?

---------------------------------------------------------

[JFFS2] roll-back the last fix. JFFS2 uses strange technique - upper callers 
clean the shit which the lower function leave... In theory, everybody should 
free its own crap... Would be nice to fix - i believe, not everything is 
cleaned well.

Excellent short description! Perfectly fitting in a line of 80 chars. And the 
careful wording is extraordinary.

It definitely leads to extended eruptions of swearing when somebody has to 
review and clean up that #§$%&@*!

---------------------------------------------------------

Five consecutive commits changing one file forth and back within 10 minutes !

Really simple to review, especially when the commit messages are so clear and 
the commit flood changes random files aside the one which is the main working 
target.

---------------------------------------------------------

[OneNAND] More fancy handling of DDP
<END of commit message>

Is [OneNAND] a new subsystem ?

DDP ? 
I know what DDP means in this context, but when somebody else tries to 
understand the message, he might try Google first:

Some Google explanantions from the first page
Delivery Duty Paid
Distributed Data Processing
Data Delivery Protocol
Decentralized Development Planning

---------------------------------------------------------

Trailing white spaces introduced since the last git update

diffstat whitespace.diff
 drivers/mtd/Kconfig                 |    4 -
 drivers/mtd/chips/cfi_cmdset_0001.c |    6 -
 drivers/mtd/inftlmount.c            |    2
 drivers/mtd/maps/ixp2000.c          |    2
 drivers/mtd/maps/mtx-1_flash.c      |    2
 drivers/mtd/maps/pq2fads.c          |   14 +--
 drivers/mtd/maps/tqm834x.c          |    8 +-
 drivers/mtd/maps/ts5500_flash.c     |    2
 drivers/mtd/nand/au1550nd.c         |    6 -
 drivers/mtd/nand/s3c2410.c          |    2
 drivers/mtd/nand/sharpsl.c          |    6 -
 drivers/mtd/onenand/onenand_base.c  |   24 +++---
 drivers/mtd/rfd_ftl.c               |  112 ++++++++++++++---------------
 fs/jffs2/build.c                    |    2
 fs/jffs2/debug.c                    |   28 +++----
 fs/jffs2/debug.h                    |    8 +-
 fs/jffs2/gc.c                       |    4 -
 fs/jffs2/nodelist.c                 |  136 
++++++++++++++++++------------------
 fs/jffs2/nodemgmt.c                 |    6 -
 fs/jffs2/readinode.c                |   64 ++++++++--------
 fs/jffs2/summary.c                  |    8 +-
 fs/jffs2/wbuf.c                     |   14 +--
 include/linux/jffs2.h               |    2
 include/linux/jffs2_fs_sb.h         |    2
 include/linux/mtd/bbm.h             |    2
 include/linux/mtd/cfi.h             |    2
 include/linux/mtd/onenand.h         |    2
 27 files changed, 235 insertions(+), 235 deletions(-)

Figure yourself, who earns the most credits for being sloppy

	tglx

Re: MTD git repository status

[Charles Manning]

Hi Thomas

As an "mtd user outside of mtd" I have an opinion on the nand stuff.

On Tuesday 08 November 2005 07:15, Thomas Gleixner wrote:
> Hi folks,
>
> I've updated the mtd git repository hopefully in time for 2.6.15.
>
> http://www.kernel.org/git/?p=linux/kernel/git/tglx/mtd-2.6.git;a=summary
>
> 	tglx
>
> Following changes are not included into the update:
>
> - [JFFS2]  Erase Block Header(EBH)
> 	Needs proper testing and stabilizing first
>
> - [MTD] NAND: OOB changes
> 	Needs more thoughts. I'm particularly unhappy with the introduction of new
> functions and the still pending problem of the automatic
> placement/retrieval of oob data. The change should subsume and/or extend
> the oob_info
> functionality rather than introducing a parallel solution.

There are two parts to this, I think:
1) The client (ie fs) side: 
More and more I think that the client should just call read_ecc etc with null 
data params in the spirit of my semi-patch proposal. This is pretty straight 
forward and obvious as to what is intended. read_oob is ambiguous. I expect 
that with time all fs/clients will move all oob handling to AUTOPLACE because 
it is too messy having oob_sel hanging through the interface. ( I know I have 
small intestines, I want to have small intestines, but I don't want to see 
them). From an OS perspective (ie outside of mtd), getting a simple interface 
that is well defined and will be consistent over time is pretty important.

2) I think that oob_info-driven nand_base has got about as complex as it can - 
perhaps gone too far. You cannot handle all possible cases with nand_base 
anyway(eg. dma-based solutions). If you try, then do too many things with the 
same code you end up with so many ifs and branches that performance goes to 
hell, as well as very convoluted code where bugs can hide with ease. I agree 
therefore that using functions to replace functionality is a better idea. It 
is perhaps time for a bit of modularity too.


>
> - [MTD] NAND: nandsim
> 	There is still the big Kconfig clutter. For testing environments it _is_
> sufficient to provide all this information as module parameters. No need to
> bloat the config file

Agree. At present nandsim is quite a chore to configure and doing this through 
Kconfig is almost pointless.

As an example, I recently created a bogus 32MB 2k page device.  I did not want 
to go much bigger for various reasons. I thus had to create a bogus nand_id 
first and then set up the Kconfig Id bytes to match.

>
> - [MTD] ONENAND: Simulator
> 	Is depending on arch/??? probably ARM and uses some obscure __iomem*
> constructs.
>
> Can the authors please shed some light on the status of following drivers:
>
> - [MTD] maps mphysram.c
> 	It needs a bunch of coding style cleanups.
>
> - [MTD] devices ramtd.c
> 	Needs probably a go on the fixed size allocation already marked with
> 	/* FIXME */
>
> - [MTD] ssfdc.c
> 	AFAIK the driver is mostly functional. What's missing ?
> 	It needs a bunch of coding style cleanups.
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: MTD git repository status

[Vitaly Wool]

Hi Thomas et al,

>- [MTD] NAND: OOB changes
>	Needs more thoughts. I'm particularly unhappy with the introduction of new 
>functions and the still pending problem of the automatic placement/retrieval 
>of oob data. The change should subsume and/or extend the oob_info 
>functionality rather than introducing a parallel solution.
>  
>
Yeah I do agree that this stuff has not been verified enough to go into 
git tree.
And I do understand your concerns, so let me elaborate on all that once 
more.

First, there's a hardware-driven need to support the OOB data 
distributed across the page. The fact is that nand_oobinfo *can't hold 
the info needed* for that since it implies that OOB data is contiguous. 
The adequate patcure can be derived only from nand_oobinfo plus 
eccsteps, but anyway handling all that in read/write functions will lead 
to the introduction of the structure similar to page_layout_item.
So, rather then trying to revise nand_oobinfo, I was moving towards 
creation of a new data structure, more say 'linear' and straightforward. 
I think that layout structure well fits.
Let's look at nand_oobinfo from the POV that page_layout_item has been 
introduced and autoplacement is the only option.
In this case the whole nand_oobinfo becomes redundant and will go away, 
and that's what I'm targeting.

I guess that the new functions introduced you're talking about are 
nand_read_oob_hwecc and nand_write_oob_hwecc.
I think that nand_read_oob and nand_write_oob will soon go away, as soon 
as the two new functions prove they are working in all the HW ECC cases. 
Extending them to support SW ECC case is trivial.
On the other hand, I do think that handling the layout in 
read_ecc/write_page should go to the separate function as now these two 
functions have become almost unreadable due to lotsa nesting levels.

Then, I'd like to point out once more that the approach introduced was 
intended to be absolutely transparent to both MTD users and particular 
NAND device drivers. This approach is capable of automated 
placement/retrieval of OOB data but that's gonna be a new stage which I 
deliberately wanted to separate from the approach introduction.
I can create a sort of design proposal for the OOB data handling and 
you/dedekind/Charles/whoever else will tear it apart :) We do need a 
good co-ordination on the changes that alter the behavior of MTD functions.

Hope that all makes sense,

Best regards,
   Vitaly

Re: MTD git repository status ==> oob_info & other methods

[Charles Manning]

On Tuesday 08 November 2005 09:43, Vitaly Wool wrote:
> Hi Thomas et al,
>
> >- [MTD] NAND: OOB changes
> >	Needs more thoughts. I'm particularly unhappy with the introduction of
> > new functions and the still pending problem of the automatic
> > placement/retrieval of oob data. The change should subsume and/or extend
> > the oob_info functionality rather than introducing a parallel solution.
>
> Yeah I do agree that this stuff has not been verified enough to go into
> git tree.
> And I do understand your concerns, so let me elaborate on all that once
> more.
>
> First, there's a hardware-driven need to support the OOB data
> distributed across the page. The fact is that nand_oobinfo *can't hold
> the info needed* for that since it implies that OOB data is contiguous.
> The adequate patcure can be derived only from nand_oobinfo plus
> eccsteps, but anyway handling all that in read/write functions will lead
> to the introduction of the structure similar to page_layout_item.
> So, rather then trying to revise nand_oobinfo, I was moving towards
> creation of a new data structure, more say 'linear' and straightforward.
> I think that layout structure well fits.
> Let's look at nand_oobinfo from the POV that page_layout_item has been
> introduced and autoplacement is the only option.
> In this case the whole nand_oobinfo becomes redundant and will go away,
> and that's what I'm targeting.

Yes, that is definitely the first prize goal,IMHO.

>
> I guess that the new functions introduced you're talking about are
> nand_read_oob_hwecc and nand_write_oob_hwecc.
> I think that nand_read_oob and nand_write_oob will soon go away, as soon
> as the two new functions prove they are working in all the HW ECC cases.
> Extending them to support SW ECC case is trivial.
> On the other hand, I do think that handling the layout in
> read_ecc/write_page should go to the separate function as now these two
> functions have become almost unreadable due to lotsa nesting levels.

The only way I see to cure this is to go toward better modularity. No actual 
fiddling with the low level reading in the do_read_ecc etc functions, but 
rather have do_read_ecc call read_data(), do_autoplace() etc functions. These 
worker-bee functions should have the understanding of nand_oobinfo or 
whatever.

A benefit of doing things this way is that it becomes easier to accomodate 
special cases or alternative strategies. You could still use current 
nand_oobinfo by plugging in worker-bee funtions that understand nand_oobinfo. 
To work with some other strategy (eg. hwecc or dma or something), you just 
plug in worker-bee functions that support that.

With a model like that you do not keep on trying to fit yet another strategy 
into a switch statement in nand_base. Instead you just write another few 
worker-bee functions. This has numerous benefits:
1) Cleaner code. Easier to write, less places for bugs to hide, faster 
execution, easier debugging.
2) Modularity. Adding a new strategy does not break an old one.

>
> Then, I'd like to point out once more that the approach introduced was
> intended to be absolutely transparent to both MTD users and particular
> NAND device drivers. 

OK, I'm biased, but I think the emphasis should be the other way around :-). A 
change that impacts on drivers is way smaller than one that is visible to 
others.

> This approach is capable of automated 
> placement/retrieval of OOB data but that's gonna be a new stage which I
> deliberately wanted to separate from the approach introduction.
> I can create a sort of design proposal for the OOB data handling and
> you/dedekind/Charles/whoever else will tear it apart :) We do need a
> good co-ordination on the changes that alter the behavior of MTD functions.

Sure, this is very important stuff and I am keen to review.

>
> Hope that all makes sense,

Yup.

Re: MTD git repository status

[Artem B. Bityutskiy]

Thomas Gleixner wrote:
> - [MTD] NAND: OOB changes
> 	Needs more thoughts. I'm particularly unhappy with the introduction of new 
> functions and the still pending problem of the automatic placement/retrieval 
> of oob data. The change should subsume and/or extend the oob_info 
> functionality rather than introducing a parallel solution.

As for me, the ideal would be to have:
1. a method to fetch the number of OOB bytes available for user;
2. a method of read/write a buffer of that length to/from OOB; that 
space may be non-contiguous, but this should be hidden from the user.

Complications like the need to supply oob_info look unreasonable to me. 
Most (may be all?) people don't care about the OOB layout and just want 
to read/write data from/to it. So, I agree with Charles.

Any example of when users want to know the oob layout? May be nanddump 
utilities? Not sure.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: MTD git repository status

[Charles Manning]

On Wednesday 09 November 2005 03:10, Artem B. Bityutskiy wrote:
> Thomas Gleixner wrote:
> > - [MTD] NAND: OOB changes
> > 	Needs more thoughts. I'm particularly unhappy with the introduction of
> > new functions and the still pending problem of the automatic
> > placement/retrieval of oob data. The change should subsume and/or extend
> > the oob_info functionality rather than introducing a parallel solution.
>
> As for me, the ideal would be to have:
> 1. a method to fetch the number of OOB bytes available for user;

Agree. At present YAFFS2 using oob_size to allocate local oob buffers etc, but 
that is not really the right thing to be doing, AFAIK.

> 2. a method of read/write a buffer of that length to/from OOB; that
> space may be non-contiguous, but this should be hidden from the user.

I think what is missing most is a definitive specification. read_oob is 
ambiguous since there is no specification of what it provides (AUTOPLACEd or 
raw or something else).

Personally, I think we can do without read_oob() and instead just use 
read/write_ecc() with dataptr == NULL.

>
> Complications like the need to supply oob_info look unreasonable to me.
> Most (may be all?) people don't care about the OOB layout and just want
> to read/write data from/to it. So, I agree with Charles.
>
> Any example of when users want to know the oob layout? May be nanddump
> utilities? Not sure.

Probably more important is just being able to get a raw nanddump so that an 
image can be taken for manufacturing purposes or debug dumping etc.

YAFFS1 (or YAFFS2 running YAFFS1 backwad compatability) still uses an oobinfo 
to pass SmartMedia style ECC placement. It does this to be able to do bad 
block marking itself rather than use the more recent mtd bad block marking 
stuff. If this has to change to make things better I won't cry.

-- Charles

Re: MTD git repository status

[Vitaly Wool]

Hi Charles et al,

>>- [MTD] NAND: OOB changes
>>	Needs more thoughts. I'm particularly unhappy with the introduction of new
>>functions and the still pending problem of the automatic
>>placement/retrieval of oob data. The change should subsume and/or extend
>>the oob_info
>>functionality rather than introducing a parallel solution.
>>    
>>
>
>There are two parts to this, I think:
>1) The client (ie fs) side: 
>More and more I think that the client should just call read_ecc etc with null 
>data params in the spirit of my semi-patch proposal. This is pretty straight 
>forward and obvious as to what is intended. read_oob is ambiguous. I expect 
>that with time all fs/clients will move all oob handling to AUTOPLACE because 
>it is too messy having oob_sel hanging through the interface. ( I know I have 
>small intestines, I want to have small intestines, but I don't want to see 
>them). From an OS perspective (ie outside of mtd), getting a simple interface 
>that is well defined and will be consistent over time is pretty important.
>  
>
This may be implemented but will lead to more complicated do_read_ecc 
function so I suggest not to do that.
Reasons:
1. nand_read_oob is capable of reading the oob data from a given offset. 
nand_do_read_ecc is not. The need for that may go away as we move 
forward with the new approach to the oob, but still...
2. There may be a need to read oob data from more than one page. If we 
want to have this functionality supported in do_read_ecc, it will lead 
to introduction of yet another parameter of this function and 
complicated function finish criteria.

>2) I think that oob_info-driven nand_base has got about as complex as it can - 
>perhaps gone too far. You cannot handle all possible cases with nand_base 
>anyway(eg. dma-based solutions). If you try, then do too many things with the 
>same code you end up with so many ifs and branches that performance goes to 
>hell, as well as very convoluted code where bugs can hide with ease. I agree 
>therefore that using functions to replace functionality is a better idea. It 
>is perhaps time for a bit of modularity too.
>  
>
I propose to get rid of nand_oobinfo completely and introduce the only 
one 'oobfree' integer which will denote spare (i. e. not used by the MTD 
layer) OOB data size per page.
This should be enough for the MTD users, as far as I can see. On the 
other hand, it will provide better modularity.
This also shows the advantage of layout-based NAND page handling. The 
layouts remain inner part of the MTD layer so MTD users like JFFSx or 
YAFFSx don't need to know anything about that as opposed to nand_oobinfo 
which needs to be exported.

Details:
Currently we have the following layout item structure:

struct page_layout_item {
        int length;
        enum {
                ITEM_TYPE_DATA,
                ITEM_TYPE_OOB,
                ITEM_TYPE_ECC,
        } type;
};

This reflects the current nand_base implementation, but in order to 
implement what's proposed above, this needs to be

struct page_layout_item {
        int length;
        enum {
                ITEM_TYPE_DATA,
                ITEM_TYPE_OOB_MTD,
                ITEM_TYPE_OOB_SPARE,
                ITEM_TYPE_ECC,
        } type;
};

Then it's going to be as simple as follows. do_read_ecc/read_oob will 
just copy only the OOB_SPARE data to the data pointer provided. 
write_page/write_oob will treat the incoming oob data as data that is 
needed to be written into the OOB_SPARE parts.
OOB_MTD will define OOB bytes used for bad block markers and whatever 
else MTD layer wants.

Best regards,
   Vitaly