mtd_info->size again (lengthy)

[Bruce_Leonard at selinc.com]

Jörn Engel <joern at logfs.org> wrote on 06/08/2008 05:10:20 AM:

> On Sat, 7 June 2008 22:38:21 -0700, Bruce_Leonard at selinc.com wrote:
> > 
> > But folks, I'm stuck.
> 
> Sometimes the best answer comes in diff -u form, see below.
> 
> This has been on my TODO list for a while.  Long-term we want to get rid

Jorn,

Thanks so much for the feedback.  I think I get what you're saying, but 
let me paraphrase it back to make sure.  My approach should be to make a 
new interface that is 64-bit clean, using one or more functions (I'm not 
sure I should shove ALL the functions that exist in mtd_info into a single 
submit_fio(), but I don't know).  For my driver I should write the new 
function(s) to be 64-bit clean and just use the new interface.  I can then 
change the core code that impacts my specific case to take advantage of 
the new interface (being carefull to make sure I don't break the old 
interface).  Existing drivers can continue to use the existing interface 
but will eventually migrate to the new interface.  If they can't be made 
64-bit clean for whatever reason, the new interface just becomes a wrapper 
to the old functions.  Sooner or later everything will be on the new 
interface and the old one can be removed.  Does that sound about right?

Just a few questions about the details of your (admitidley un-compiled and 
un-tested :)  ) solution:

> 
> +static int ram_submit_fio(struct mtd_info *mtd, struct fio *fio)
> +{

I gather fio is something that's to be filled in each time ram_submit_fio 
is called rather than being a static thing in mtd_info?  Sort of like 
struct mtd_oob_ops in the NAND layer gets filled in based on what's being 
called?

> +   int i, err = 0;
> +   u32 ofs;
> +   size_t *retlen;
> +   const u_char *buf;
> +   struct fio_vec *vec;
> +
> +   /* This driver cannot deal with device >32bit yet */
> +   if (fio->fi_blockno * mtd->erasesize + fio->block_ofs + fio->fi_len
> +         > 0xffffffff)
> +      return -EIO;
> +
> +   ofs = fio->fi_blockno * mtd->erasesize + fio->block_ofs;
> +
> +   if (fio->fi_command == FI_ERASE) {
> +      memset(mtd->priv + ofs, 0xff, fio->fi_len);
> +      goto out;
> +   }
> +
> +   for (i = 0; i < fio->fi_vcnt; i++) {
> +      vec = fio->fi_io_vec + i;
> +      buf = kmap_atomic(vec->fv_page, KM_USER0);
> +      switch (fio->fi_command) {
> +      case FI_READ:
> +         err = ram_read(mtd, ofs, vec->fv_len, &retlen,
> +               buf + vec->fv_offset);

I'm not famliar enough with block I/O to know what advantages or 
disadvantages is has, but since all the upper layers already allocate a 
buffer for putting date into or taking data out of, wouldn't it make more 
sense/be simpler to just keep using it?  For example, mtd_read() in 
mtdchar.c does a kmalloc to get a buffer that is then passed to 
mtd->read().  Wouldn't it be simpler to just keep using that rather than 
switching to struct page?  Or is there something that I'm missing that 
struct page brings to the game?

> +         break;
> +      case FI_WRITE:
> +         err = ram_write(mtd, ofs, vec->fv_len, &retlen,
> +               buf + vec->fv_offset);
> +      default:
> +         BUG();
> +      }
> +      kunmap_atomic(buf, KM_USER0);
> +      ofs += vec->fv_len;
> +      if (err)
> +         goto out;
> +   }
> +out:
> +   fio->fi_end_io(fio, err);

Not sure I follow the logic of the typedef here and what it is you're 
trying to accomplish.

<snip>

> +
> +typedef void (fio_end_io_t) (struct fio *, int);
> +/*

<snip>

>  struct mtd_info {
>     u_char type;
>     u_int32_t flags;
> -   u_int32_t size;    // Total size of the MTD
> +   /*
> +    * size is becoming redundant, as it can be calculated from
> +    * no_eraseblocks * erasesize.  There is a fair chance it will die
> +    * in the near future.  For flashes with variable erase sizes,
> +    * no_eraseblocks signifies the "normal" size, so above calculation
> +    * remains valid.
> +    */

Which calculation are you refering to?

> +   u_int64_t size;    // Total size of the MTD
> +   u_int64_t no_eraseblocks;
> 

In my original email I mentioned that I ran into a few problems makeing 
size a 64-bit value.  Did you have any problems with it?

Thanks again for the feedback.  It's given me something I think I can sink 
my teeth into and finally make some progress.  At least I hope so :)

Bruce