Re: mtd->size revisited

[Artem Bityutskiy <dedekind@infradead.org>]

Hi,

On Wed, 2008-04-09 at 15:39 -0700, Bruce_Leonard@selinc.com wrote:
> I asked about this a few weeks ago, came up with an alternative solution, 
> but that solution came up against a 30% performance hit on accessing my 
> flash, so we're back to a kernel change.  Let me recap a bit.  The issue I 
> have is that I'm trying to put together a NAND flash driver for an 8GiB 
> part.  The struct mtd_info->size field is a u_int32_t, which is not big 
> enough to hold 0x200000000 (i.e., 8GiB).  The question I originally asked 
> was "am I understanding this correctly and is it an issue for anyone else" 
> and got the answer "it's either a bug or an enhancement depending on your 
> viewpoint, patches welcome".

It is actually issue, just nobody was really eager to solve this.

> I know that just changing it isn't all that's involved.  I've alredy gone 
> through some of the pain of changing 32-bit division to 64-bit shifts and 
> masks.  A larger concern of mine is ripple effects.  I know that this 
> field is referenced in a lot of places, most of which are pretty benign, 
> and others that are less so.  A good example is in .../mtd/mtdblock.c 
> where this happens: dev->size = mtd->size >> 9.  I don't know right off 
> the top of my head what struct mtd_blktrans_dev->size is used for, but I 
> can pretty well bet that a 64-bit value shifted right by nine ain't gonna 
> fit.  So then this ripples into making changes to mtd_blktrans_dev.  Wow. 
> This could end up going a long way.  Plus, I know there are a couple of 
> drivers that make use of this field and I expect that if I make this 
> change, I'll be responsible for makeing the changes to the drivers. 
> However, I can't test those changes since I don't have the HW.  How is 
> that going to be received by the community and the maintainers of those 
> drivers?

What you should do is to talk to dwmw2, who is the original author of
this. I have not seen him in the mailing list for long time, but you
might talk to him at #mtd IRC channel at OFTC.

Vs.struct mtd_blktrans_dev->size, I am sure this is just size of block
device in 512-byte sectors. You may quite safely assume it should fit
32-bits I think. You may add a check even.

> So, as I said, I'm willing to tackle this, but I need to get a sense of 
> how long it's going to take me.  I need an idea of how long it would take 
> someone with experiance with MTD and I can then translate that (roughly :) 
>  ) into how long it should take me.  I also need guidence on how to deal 
> with things that I can't test.

My opinion on this is that the best way to address flash is
eraseblock:offset pair, not a 64-bit offset. Indeed, it is natural for
flash to consist of eraseblocks. And in opposite, it is not natural to
address as a 64-bit address. I mead, flash _does_ consist of
eraseblocks, why not to reflect this in the MTD interface?

What I would do is I'd invent a new interface with EB:offset addressing.
I'd preserve the old interface for the older stuff and wouldn't even try
to change them. I'd just turn the stuff I need to use the new interface.

Something like:

ret = mtd->read64(mtd, eraseblock, offset, buf, len).

But anyway, better talk to dwmw2, because he will accept/reject your
work :-)

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)