mtd->size revisited

mtd->size revisited

[Bruce_Leonard]

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".

Well, I'm willing to try and change this, but I'm still learning my way 
around the kernel and have no where near the abilities of the folks who 
orginally wrote this stuff.  Before I take this on, my employer (who 
understandably wants the most bang for thier buck :-\ ) wants to know how 
long it's going to take.  My answer is a confidant "duhhhhhhhhhh".  What 
I'm looking for is a ballpark guestimate from someone familiar with MTD as 
to how long it would take them to change struct mtd_info->size to a 
u_int64_t, test it, and deal with ripple affects.

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?

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.

Thanks for any help and direction.

Bruce

Re: mtd->size revisited

[Vernon Sauder]

Bruce_Leonard@selinc.com wrote:
> 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?
>
> 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.
>
> Thanks for any help and direction.
>
> Bruce
>   

While I agree that it would be great to support larger devices, I'm 
going to ask a dumb question: does this have to grow to a 64-bit number? 
Can it be in KB or something? If device sizes are always divisible by 
1KB, we don't need the bottom 10 bits anyway. That would seem an easier 
change in some ways.

Vern

Re: mtd->size revisited

[Trent Piepho]

On Wed, 9 Apr 2008, Vernon Sauder wrote:
> Bruce_Leonard@selinc.com wrote:
>
> While I agree that it would be great to support larger devices, I'm
> going to ask a dumb question: does this have to grow to a 64-bit number?
> Can it be in KB or something? If device sizes are always divisible by
> 1KB, we don't need the bottom 10 bits anyway. That would seem an easier
> change in some ways.

It's more efficient that way too.

Instead of just changing the size to uint64_t, it might be better to create a
new type which can be 32 or 64.  Once you start using the right type
everywhere you usually don't have to change much code.  The only exception is
printf format strings, which can be a pain.

Or like Vern said, just shift off the bottom bits.  define MTD_SIZE_SHIFT to
the number of bits to shift off.  One can then make trivial macros/inline
functions mtd_size_to_kb(), mtd_size_to_bytes(), mtd_size_to_pages(), etc. 
when code actually cares about the units.  I imagine a lot of code doesn't
care if it's bytes or kilobytes, as long as everything uses the same units.

Re: mtd->size revisited

[Artem Bityutskiy]

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 (Битюцкий Артём)

Re: mtd->size revisited

[Bruce_Leonard]

Thanks Vernon, Trent, and Artem for the suggestions, there are some really 
good ones that look like they'll save me some pain that I wouldn't have 
thought of.  Artem, I'll definety try and get in touch with dwmw2, thanks 
for the POC.  I'm not entirely clear on your suggestion Artem, but (if you 
agree) I think we should probably take further discussions of it out of 
the mailing list rather than creating a lot of noise for the people on the 
list who do understand ;).

Thanks again to all.

Bruce

Re: mtd->size revisited

[Adrian Hunter]

Bruce_Leonard@selinc.com wrote:
> 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?

I would suggest making 64-bit size support a config option and not touching
any code you don't have to for your device.

This reduces the amount of work you have to do and leaves 32-bit people
unaffected.