2008-08-22 02:01:41

by Bruce Leonard

[permalink] [raw]
Subject: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

>From 3e33a3cfc289e9baa41f6e7cf4d612d41b88e19b Mon Sep 17 00:00:00 2001
From: brucle <[email protected]>
Date: Wed, 13 Aug 2008 18:07:19 -0700
Subject: [PATCH] Add support for > 2GiB MTD devices

Signed-off-by: Bruce D. Leonard <[email protected]>

---
drivers/mtd/mtdchar.c | 16 ++++++++--------
drivers/mtd/mtdcore.c | 6 +++---
drivers/mtd/nand/nand_base.c | 36 ++++++++++++++++++++++++------------
drivers/mtd/nand/nand_bbt.c | 30 +++++++++++++++---------------
drivers/mtd/ubi/build.c | 5 +++--
include/linux/mtd/mtd.h | 27 ++++++++++++++++++++++-----
include/mtd/mtd-abi.h | 4 ++--
7 files changed, 77 insertions(+), 47 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index d2f3318..2829faa 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -70,13 +70,13 @@ static loff_t mtd_lseek (struct file *file, loff_t offset, int orig)
offset += file->f_pos;
break;
case SEEK_END:
- offset += mtd->size;
+ offset += device_size(mtd);
break;
default:
return -EINVAL;
}

- if (offset >= 0 && offset <= mtd->size)
+ if (offset >= 0 && offset <= device_size(mtd))
return file->f_pos = offset;

return -EINVAL;
@@ -173,8 +173,8 @@ static ssize_t mtd_read(struct file *file, char __user *buf, size_t count,loff_t

DEBUG(MTD_DEBUG_LEVEL0,"MTD_read\n");

- if (*ppos + count > mtd->size)
- count = mtd->size - *ppos;
+ if (*ppos + count > device_size(mtd))
+ count = device_size(mtd) - *ppos;

if (!count)
return 0;
@@ -266,11 +266,11 @@ static ssize_t mtd_write(struct file *file, const char __user *buf, size_t count

DEBUG(MTD_DEBUG_LEVEL0,"MTD_write\n");

- if (*ppos == mtd->size)
+ if (*ppos == device_size(mtd))
return -ENOSPC;

- if (*ppos + count > mtd->size)
- count = mtd->size - *ppos;
+ if (*ppos + count > device_size(mtd))
+ count = device_size(mtd) - *ppos;

if (!count)
return 0;
@@ -426,7 +426,7 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
case MEMGETINFO:
info.type = mtd->type;
info.flags = mtd->flags;
- info.size = mtd->size;
+ info.size = device_size(mtd);
info.erasesize = mtd->erasesize;
info.writesize = mtd->writesize;
info.oobsize = mtd->oobsize;
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index a9d2469..98bc81f 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -60,7 +60,7 @@ int add_mtd_device(struct mtd_info *mtd)
/* Some chips always power up locked. Unlock them now */
if ((mtd->flags & MTD_WRITEABLE)
&& (mtd->flags & MTD_POWERUP_LOCK) && mtd->unlock) {
- if (mtd->unlock(mtd, 0, mtd->size))
+ if (mtd->unlock(mtd, 0, device_size(mtd)))
printk(KERN_WARNING
"%s: unlock failed, "
"writes may not work\n",
@@ -344,8 +344,8 @@ static inline int mtd_proc_info (char *buf, int i)
if (!this)
return 0;

- return sprintf(buf, "mtd%d: %8.8x %8.8x \"%s\"\n", i, this->size,
- this->erasesize, this->name);
+ return sprintf(buf, "mtd%d: %16.16llx %8.8x \"%s\"\n", i,
+ device_size(this), this->erasesize, this->name);
}

static int mtd_read_proc (char *page, char **start, off_t off, int count,
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d5ac675..2463250 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1179,7 +1179,7 @@ static int nand_read(struct mtd_info *mtd, loff_t from, size_t len,
int ret;

/* Do not allow reads past end of device */
- if ((from + len) > mtd->size)
+ if ((from + len) > device_size(mtd))
return -EINVAL;
if (!len)
return 0;
@@ -1371,8 +1371,8 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
}

/* Do not allow reads past end of device */
- if (unlikely(from >= mtd->size ||
- ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
+ if (unlikely(from >= device_size(mtd) ||
+ ops->ooboffs + readlen > ((device_size(mtd) >> chip->page_shift) -
(from >> chip->page_shift)) * len)) {
DEBUG(MTD_DEBUG_LEVEL0, "nand_read_oob: "
"Attempt read beyond end of device\n");
@@ -1448,7 +1448,7 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
ops->retlen = 0;

/* Do not allow reads past end of device */
- if (ops->datbuf && (from + ops->len) > mtd->size) {
+ if (ops->datbuf && (from + ops->len) > device_size(mtd)) {
DEBUG(MTD_DEBUG_LEVEL0, "nand_read_oob: "
"Attempt read beyond end of device\n");
return -EINVAL;
@@ -1813,7 +1813,7 @@ static int nand_write(struct mtd_info *mtd, loff_t to, size_t len,
int ret;

/* Do not allow reads past end of device */
- if ((to + len) > mtd->size)
+ if ((to + len) > device_size(mtd))
return -EINVAL;
if (!len)
return 0;
@@ -1869,9 +1869,9 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
}

/* Do not allow reads past end of device */
- if (unlikely(to >= mtd->size ||
+ if (unlikely(to >= device_size(mtd) ||
ops->ooboffs + ops->ooblen >
- ((mtd->size >> chip->page_shift) -
+ ((device_size(mtd) >> chip->page_shift) -
(to >> chip->page_shift)) * len)) {
DEBUG(MTD_DEBUG_LEVEL0, "nand_read_oob: "
"Attempt write beyond end of device\n");
@@ -1928,7 +1928,7 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to,
ops->retlen = 0;

/* Do not allow writes past end of device */
- if (ops->datbuf && (to + ops->len) > mtd->size) {
+ if (ops->datbuf && (to + ops->len) > device_size(mtd)) {
DEBUG(MTD_DEBUG_LEVEL0, "nand_read_oob: "
"Attempt read beyond end of device\n");
return -EINVAL;
@@ -2019,8 +2019,8 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
int rewrite_bbt[NAND_MAX_CHIPS]={0};
unsigned int bbt_masked_page = 0xffffffff;

- DEBUG(MTD_DEBUG_LEVEL3, "nand_erase: start = 0x%08x, len = %i\n",
- (unsigned int)instr->addr, (unsigned int)instr->len);
+ DEBUG(MTD_DEBUG_LEVEL3, "nand_erase: start = 0x%016llx, len = %i\n",
+ instr->addr, (unsigned int)instr->len);

/* Start address must align on block boundary */
if (instr->addr & ((1 << chip->phys_erase_shift) - 1)) {
@@ -2036,7 +2036,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
}

/* Do not allow erase past end of device */
- if ((instr->len + instr->addr) > mtd->size) {
+ if ((instr->len + instr->addr) > device_size(mtd)) {
DEBUG(MTD_DEBUG_LEVEL0, "nand_erase: "
"Erase past end of device\n");
return -EINVAL;
@@ -2208,7 +2208,7 @@ static void nand_sync(struct mtd_info *mtd)
static int nand_block_isbad(struct mtd_info *mtd, loff_t offs)
{
/* Check for invalid offset */
- if (offs > mtd->size)
+ if (offs > device_size(mtd))
return -EINVAL;

return nand_block_checkbad(mtd, offs, 1, 0);
@@ -2502,6 +2502,18 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips)
chip->numchips = i;
mtd->size = i * chip->chipsize;

+ /* Because mtd->size is 32 bits, if the total 'device size'
+ * is greater than 2GiB it will overflow mtd->size and signal
+ * that we need to use the new MTD mio interface.
+ */
+ if (mtd->size == 0) {
+ mtd->num_eraseblocks = (i * (__u64)(chip->chipsize)) >>
+ chip->phys_erase_shift;
+ } else {
+ /* Can't guarantee mtd was kzalloc'ed */
+ mtd->num_eraseblocks = 0;
+ }
+
return 0;
}

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 2f9f0f5..ae6e1a5 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -171,16 +171,16 @@ static int read_bbt(struct mtd_info *mtd, uint8_t *buf, int page, int num,
if (tmp == msk)
continue;
if (reserved_block_code && (tmp == reserved_block_code)) {
- printk(KERN_DEBUG "nand_read_bbt: Reserved block at 0x%08x\n",
- ((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
+ printk(KERN_DEBUG "nand_read_bbt: Reserved block at 0x%016llx\n",
+ (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
mtd->ecc_stats.bbtblocks++;
continue;
}
/* Leave it for now, if its matured we can move this
* message to MTD_DEBUG_LEVEL0 */
- printk(KERN_DEBUG "nand_read_bbt: Bad block at 0x%08x\n",
- ((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
+ printk(KERN_DEBUG "nand_read_bbt: Bad block at 0x%016llx\n",
+ (loff_t)((offs << 2) + (act >> 1)) << this->bbt_erase_shift);
/* Factory marked bad or worn out ? */
if (tmp == 0)
this->bbt[offs + (act >> 3)] |= 0x3 << (act & 0x06);
@@ -399,7 +399,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,
if (chip == -1) {
/* Note that numblocks is 2 * (real numblocks) here, see i+=2
* below as it makes shifting and masking less painful */
- numblocks = mtd->size >> (this->bbt_erase_shift - 1);
+ numblocks = device_size(mtd) >> (this->bbt_erase_shift - 1);
startblock = 0;
from = 0;
} else {
@@ -428,8 +428,8 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf,

if (ret) {
this->bbt[i >> 3] |= 0x03 << (i & 0x6);
- printk(KERN_WARNING "Bad eraseblock %d at 0x%08x\n",
- i >> 1, (unsigned int)from);
+ printk(KERN_WARNING "Bad eraseblock %d at 0x%016llx\n",
+ i >> 1, from);
mtd->ecc_stats.badblocks++;
}

@@ -467,7 +467,7 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr

/* Search direction top -> down ? */
if (td->options & NAND_BBT_LASTBLOCK) {
- startblock = (mtd->size >> this->bbt_erase_shift) - 1;
+ startblock = (device_size(mtd) >> this->bbt_erase_shift) - 1;
dir = -1;
} else {
startblock = 0;
@@ -481,7 +481,7 @@ static int search_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr
startblock &= bbtblocks - 1;
} else {
chips = 1;
- bbtblocks = mtd->size >> this->bbt_erase_shift;
+ bbtblocks = device_size(mtd) >> this->bbt_erase_shift;
}

/* Number of bits for each erase block in the bbt */
@@ -587,7 +587,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
chip = chipsel;
}
} else {
- numblocks = (int)(mtd->size >> this->bbt_erase_shift);
+ numblocks = (int)(device_size(mtd) >> this->bbt_erase_shift);
nrchips = 1;
}

@@ -719,7 +719,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,

memset(&einfo, 0, sizeof(einfo));
einfo.mtd = mtd;
- einfo.addr = (unsigned long)to;
+ einfo.addr = to;
einfo.len = 1 << this->bbt_erase_shift;
res = nand_erase_nand(mtd, &einfo, 1);
if (res < 0)
@@ -729,8 +729,8 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
if (res < 0)
goto outerr;

- printk(KERN_DEBUG "Bad block table written to 0x%08x, version "
- "0x%02X\n", (unsigned int)to, td->version[chip]);
+ printk(KERN_DEBUG "Bad block table written to 0x%016llx, version "
+ "0x%02X\n", to, td->version[chip]);

/* Mark it as used */
td->pages[chip] = page;
@@ -896,7 +896,7 @@ static void mark_bbt_region(struct mtd_info *mtd, struct nand_bbt_descr *td)
nrblocks = (int)(this->chipsize >> this->bbt_erase_shift);
} else {
chips = 1;
- nrblocks = (int)(mtd->size >> this->bbt_erase_shift);
+ nrblocks = (int)(device_size(mtd) >> this->bbt_erase_shift);
}

for (i = 0; i < chips; i++) {
@@ -957,7 +957,7 @@ int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)
struct nand_bbt_descr *td = this->bbt_td;
struct nand_bbt_descr *md = this->bbt_md;

- len = mtd->size >> (this->bbt_erase_shift + 2);
+ len = device_size(mtd) >> (this->bbt_erase_shift + 2);
/* Allocate memory (2bit per block) and clear the memory bad block table */
this->bbt = kzalloc(len, GFP_KERNEL);
if (!this->bbt) {
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index c7630a2..6a576d8 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -561,8 +561,9 @@ static int io_init(struct ubi_device *ubi)
*/

ubi->peb_size = ubi->mtd->erasesize;
- ubi->peb_count = ubi->mtd->size / ubi->mtd->erasesize;
- ubi->flash_size = ubi->mtd->size;
+ ubi->peb_count = device_size(ubi->mtd) >>
+ (ffs(ubi->mtd->erasesize) - 1);
+ ubi->flash_size = device_size(ubi->mtd);

if (ubi->mtd->block_isbad && ubi->mtd->block_markbad)
ubi->bad_allowed = 1;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 9226365..ffa24b6 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -26,13 +26,13 @@
#define MTD_ERASE_FAILED 0x10

/* If the erase fails, fail_addr might indicate exactly which block failed. If
- fail_addr = 0xffffffff, the failure was not at the device level or was not
+ fail_addr = 0xffffffffffffffff, the failure was not at the device level or was not
specific to any particular block. */
struct erase_info {
struct mtd_info *mtd;
- u_int32_t addr;
+ u_int64_t addr;
u_int32_t len;
- u_int32_t fail_addr;
+ u_int64_t fail_addr;
u_long time;
u_long retries;
u_int dev;
@@ -101,6 +101,14 @@ struct mtd_info {
u_int32_t flags;
u_int32_t size; // Total size of the MTD

+ /* 'size' is becoming problematic as flash densities increase. Since
+ * the device's size can be calculated by multiplying the number of
+ * erase blocks by the size of the erase block, I've added a new
+ * field 'num_eraseblocks', and wrapped it up in a inline function
+ * (see below).
+ */
+ u_int64_t num_eraseblocks;
+
/* "Major" erase size for the device. Naïve users may take this
* to be the only erase size available, or may use the more detailed
* information below if they desire
@@ -188,8 +196,8 @@ struct mtd_info {
void (*sync) (struct mtd_info *mtd);

/* Chip-supported device locking */
- int (*lock) (struct mtd_info *mtd, loff_t ofs, size_t len);
- int (*unlock) (struct mtd_info *mtd, loff_t ofs, size_t len);
+ int (*lock) (struct mtd_info *mtd, loff_t ofs, u_int64_t len);
+ int (*unlock) (struct mtd_info *mtd, loff_t ofs, u_int64_t len);

/* Power Management functions */
int (*suspend) (struct mtd_info *mtd);
@@ -219,6 +227,15 @@ struct mtd_info {
void (*put_device) (struct mtd_info *mtd);
};

+/*
+ * Inline function for determining the size of the MTD device, independant
+ * of old or new way of doing things.
+ *
+ */
+static inline u_int64_t device_size(struct mtd_info *a)
+{
+ return a->num_eraseblocks == 0 ? a->size : a->num_eraseblocks * a->erasesize;
+}

/* Kernel-side ioctl definitions */

diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index c6c61cd..86347cf 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -6,7 +6,7 @@
#define __MTD_ABI_H__

struct erase_info_user {
- uint32_t start;
+ uint64_t start;
uint32_t length;
};

@@ -50,7 +50,7 @@ struct mtd_oob_buf {
struct mtd_info_user {
uint8_t type;
uint32_t flags;
- uint32_t size; // Total size of the MTD
+ uint64_t size; // Total size of the MTD
uint32_t erasesize;
uint32_t writesize;
uint32_t oobsize; // Amount of OOB data per block (e.g. 16)
--
1.5.3.4


2008-08-26 23:55:51

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
Bruce Leonard <[email protected]> wrote:

> --- a/include/mtd/mtd-abi.h
> +++ b/include/mtd/mtd-abi.h
> @@ -6,7 +6,7 @@
> #define __MTD_ABI_H__
>
> struct erase_info_user {
> - uint32_t start;
> + uint64_t start;
> uint32_t length;
> };
>
> @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> struct mtd_info_user {
> uint8_t type;
> uint32_t flags;
> - uint32_t size; // Total size of the MTD
> + uint64_t size; // Total size of the MTD
> uint32_t erasesize;
> uint32_t writesize;
> uint32_t oobsize; // Amount of OOB data per block (e.g. 16)

This changes the kernel<->userspace ABI and is hence a big no-no. I
assume that this change will cause old userspace to malfunction on new
kernels, and vice versa.

Supporting >2Gb MTD devices sounds useful (I'm surprised that we don't
already do so).

Please cc [email protected] (at least) on MTD-related
patches, thanks.

2008-08-27 01:41:07

by Tim Anderson

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

Bruce,

Since you don't want to change the ABI you need to extend it. Andrew, should
he add an new ioctl that passes a new structure definition? That way the
original code works and you have to do a new ioctl to get the new size data.

> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of
> [email protected]
> Sent: Tuesday, August 26, 2008 6:21 PM
> To: Andrew Morton
> Cc: [email protected];
> [email protected]; David Woodhouse;
> [email protected]; Bruce Leonard
> Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
>
> [email protected] wrote on 08/26/2008 04:55:36 PM:
>
> > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > Bruce Leonard <[email protected]> wrote:
> >
> > > --- a/include/mtd/mtd-abi.h
> > > +++ b/include/mtd/mtd-abi.h
> > > @@ -6,7 +6,7 @@
> > > #define __MTD_ABI_H__
> > >
> > > struct erase_info_user {
> > > - uint32_t start;
> > > + uint64_t start;
> > > uint32_t length;
> > > };
> > >
> > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > > struct mtd_info_user {
> > > uint8_t type;
> > > uint32_t flags;
> > > - uint32_t size; // Total size of the MTD
> > > + uint64_t size; // Total size of the MTD
> > > uint32_t erasesize;
> > > uint32_t writesize;
> > > uint32_t oobsize; // Amount of OOB data per block (e.g. 16)
> >
> > This changes the kernel<->userspace ABI and is hence a big no-no. I
> > assume that this change will cause old userspace to
> malfunction on new
> > kernels, and vice versa.
> >
>
> Well, in my posting I noted that the mtd-utils were broken
> because of this
> but I didn't really have any idea as to how to fix things. I
> can see why
> it would be a big no-no to change this. Do you have any
> suggestions on
> what I could do differently to prevent making that change?
>
> > Supporting >2Gb MTD devices sounds useful (I'm surprised
> that we don't
> > already do so).
> >
>
> There was a LOT of interest in this over the last few months
> while I was
> working on it, but a very suprising silence has developed
> since I posted
> the patches. I guess I'm more cutting edge than I thought :).
>
> > Please cc [email protected] (at least) on MTD-related
> > patches, thanks.
>
> I started with the MTD list and then also posted to lkml when
> I realized I
> had forgotten to CC it.
>
> Thanks.
>
> Bruce
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

2008-08-27 01:44:51

by Bruce_Leonard

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[email protected] wrote on 08/26/2008 04:55:36 PM:

> On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> Bruce Leonard <[email protected]> wrote:
>
> > --- a/include/mtd/mtd-abi.h
> > +++ b/include/mtd/mtd-abi.h
> > @@ -6,7 +6,7 @@
> > #define __MTD_ABI_H__
> >
> > struct erase_info_user {
> > - uint32_t start;
> > + uint64_t start;
> > uint32_t length;
> > };
> >
> > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > struct mtd_info_user {
> > uint8_t type;
> > uint32_t flags;
> > - uint32_t size; // Total size of the MTD
> > + uint64_t size; // Total size of the MTD
> > uint32_t erasesize;
> > uint32_t writesize;
> > uint32_t oobsize; // Amount of OOB data per block (e.g. 16)
>
> This changes the kernel<->userspace ABI and is hence a big no-no. I
> assume that this change will cause old userspace to malfunction on new
> kernels, and vice versa.
>

Well, in my posting I noted that the mtd-utils were broken because of this
but I didn't really have any idea as to how to fix things. I can see why
it would be a big no-no to change this. Do you have any suggestions on
what I could do differently to prevent making that change?

> Supporting >2Gb MTD devices sounds useful (I'm surprised that we don't
> already do so).
>

There was a LOT of interest in this over the last few months while I was
working on it, but a very suprising silence has developed since I posted
the patches. I guess I'm more cutting edge than I thought :).

> Please cc [email protected] (at least) on MTD-related
> patches, thanks.

I started with the MTD list and then also posted to lkml when I realized I
had forgotten to CC it.

Thanks.

Bruce

2008-08-27 02:13:57

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Tue, 26 Aug 2008 18:40:26 -0700 "Tim Anderson" <[email protected]> wrote:

> > -----Original Message-----
> > From: [email protected]
> > [mailto:[email protected]] On Behalf Of
> > [email protected]
> > Sent: Tuesday, August 26, 2008 6:21 PM
> > To: Andrew Morton
> > Cc: [email protected];
> > [email protected]; David Woodhouse;
> > [email protected]; Bruce Leonard
> > Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
> >
> > [email protected] wrote on 08/26/2008 04:55:36 PM:
> >
> > > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > > Bruce Leonard <[email protected]> wrote:
> > >
> > > > --- a/include/mtd/mtd-abi.h
> > > > +++ b/include/mtd/mtd-abi.h
> > > > @@ -6,7 +6,7 @@
> > > > #define __MTD_ABI_H__
> > > >
> > > > struct erase_info_user {
> > > > - uint32_t start;
> > > > + uint64_t start;
> > > > uint32_t length;
> > > > };
> > > >
> > > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > > > struct mtd_info_user {
> > > > uint8_t type;
> > > > uint32_t flags;
> > > > - uint32_t size; // Total size of the MTD
> > > > + uint64_t size; // Total size of the MTD
> > > > uint32_t erasesize;
> > > > uint32_t writesize;
> > > > uint32_t oobsize; // Amount of OOB data per block (e.g. 16)
> > >
> > > This changes the kernel<->userspace ABI and is hence a big no-no. I
> > > assume that this change will cause old userspace to
> > malfunction on new
> > > kernels, and vice versa.
> > >
> >
> > Well, in my posting I noted that the mtd-utils were broken
> > because of this
> > but I didn't really have any idea as to how to fix things. I
> > can see why
> > it would be a big no-no to change this. Do you have any
> > suggestions on
> > what I could do differently to prevent making that change?
> >
> > > Supporting >2Gb MTD devices sounds useful (I'm surprised
> > that we don't
> > > already do so).
> > >
> >
> > There was a LOT of interest in this over the last few months
> > while I was
> > working on it, but a very suprising silence has developed
> > since I posted
> > the patches. I guess I'm more cutting edge than I thought :).
> >

(top-posting repaired. Please don't.)

> Bruce,
>
> Since you don't want to change the ABI you need to extend it. Andrew, should
> he add an new ioctl that passes a new structure definition? That way the
> original code works and you have to do a new ioctl to get the new size data.
>

Yes, I expect that a new ioctl would be best. Sometimes it is possible
to extend an existing one (and its associated payload) but that
requires versioning info which usually wasn't thought of on day one.

But David's the man.

2008-08-27 02:47:56

by Bruce_Leonard

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

Now there's something I hadn't thought of. Thanks, I'll look into doing
it that way.


"Tim Anderson" <[email protected]> wrote on 08/26/2008 06:40:26 PM:

> Bruce,
>
> Since you don't want to change the ABI you need to extend it. Andrew,
should
> he add an new ioctl that passes a new structure definition? That way the
> original code works and you have to do a new ioctl to get the new size
data.
>
> > -----Original Message-----
> > From: [email protected]
> > [mailto:[email protected]] On Behalf Of
> > [email protected]
> > Sent: Tuesday, August 26, 2008 6:21 PM
> > To: Andrew Morton
> > Cc: [email protected];
> > [email protected]; David Woodhouse;
> > [email protected]; Bruce Leonard
> > Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
> >
> > [email protected] wrote on 08/26/2008 04:55:36 PM:
> >
> > > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > > Bruce Leonard <[email protected]> wrote:
> > >
> > > > --- a/include/mtd/mtd-abi.h
> > > > +++ b/include/mtd/mtd-abi.h
> > > > @@ -6,7 +6,7 @@
> > > > #define __MTD_ABI_H__
> > > >
> > > > struct erase_info_user {
> > > > - uint32_t start;
> > > > + uint64_t start;
> > > > uint32_t length;
> > > > };
> > > >
> > > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > > > struct mtd_info_user {
> > > > uint8_t type;
> > > > uint32_t flags;
> > > > - uint32_t size; // Total size of the MTD
> > > > + uint64_t size; // Total size of the MTD
> > > > uint32_t erasesize;
> > > > uint32_t writesize;
> > > > uint32_t oobsize; // Amount of OOB data per block (e.g. 16)
> > >
> > > This changes the kernel<->userspace ABI and is hence a big no-no. I
> > > assume that this change will cause old userspace to
> > malfunction on new
> > > kernels, and vice versa.
> > >
> >
> > Well, in my posting I noted that the mtd-utils were broken
> > because of this
> > but I didn't really have any idea as to how to fix things. I
> > can see why
> > it would be a big no-no to change this. Do you have any
> > suggestions on
> > what I could do differently to prevent making that change?
> >
> > > Supporting >2Gb MTD devices sounds useful (I'm surprised
> > that we don't
> > > already do so).
> > >
> >
> > There was a LOT of interest in this over the last few months
> > while I was
> > working on it, but a very suprising silence has developed
> > since I posted
> > the patches. I guess I'm more cutting edge than I thought :).
> >
> > > Please cc [email protected] (at least) on MTD-related
> > > patches, thanks.
> >
> > I started with the MTD list and then also posted to lkml when
> > I realized I
> > had forgotten to CC it.
> >
> > Thanks.
> >
> > Bruce
> >
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
>

2008-08-27 06:00:10

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

Hi Bruce,

On Tue, 2008-08-26 at 18:21 -0700, [email protected] wrote:
> > This changes the kernel<->userspace ABI and is hence a big no-no. I
> > assume that this change will cause old userspace to malfunction on new
> > kernels, and vice versa.
> >
>
> Well, in my posting I noted that the mtd-utils were broken because of this
> but I didn't really have any idea as to how to fix things. I can see why
> it would be a big no-no to change this. Do you have any suggestions on
> what I could do differently to prevent making that change?

Unfortunately, it is unacceptable to break existing stuff, even old
crappy mtd utils :-). That's simply how we work in the kernel community.

Stuff which you do is always not simple because you have to make sure
all legacy stuff does not break. So you'll need to invest more time into
your work.

> There was a LOT of interest in this over the last few months while I was
> working on it, but a very suprising silence has developed since I posted
> the patches. I guess I'm more cutting edge than I thought :).

I understand your frustration. Apologies for not reacting faster.

> > Please cc [email protected] (at least) on MTD-related
> > patches, thanks.
>
> I started with the MTD list and then also posted to lkml when I realized I
> had forgotten to CC it.

Yeah, sometimes MTD ML just ignores patches, I am in the same boat, but
even if you've decided to send stuff to lkml, please, CC MTD ML anyway.

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

2008-08-27 06:01:51

by Tim Anderson

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

Bruce,

I have been experimenting with this. I think there is at least 3 maybe 4
IOCTLS that are going to need to have a LARGE mode. The 2 most obvious are
MEMGETINFO and MEMERASE. I was starting to code something up that passes the
size as blocks as opposed to bytes. That may be more consistent with UBI. I am
doing a little experimenting if you want some help there. In any case, it can
be done where the old utility can work (for 2GB and less size devices) and a
new call not supported by the kernel will let the utility try the new
interface and revert to the old if the kernel does not support block mode
interfaces.

Then we can either re-use the existing structures passed through another
ioctl, changing the definition of START and SIZE to be blocks or define new
erase_info_user and mtd_info_user structures for the large call.

David, would you thing re-using the same structure would be to obscure?

I have started looking at a MEMGETLINFO defining size as blocks instead of
bytes.


> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of
> [email protected]
> Sent: Tuesday, August 26, 2008 6:21 PM
> To: Andrew Morton
> Cc: [email protected];
> [email protected]; David Woodhouse;
> [email protected]; Bruce Leonard
> Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
>
> [email protected] wrote on 08/26/2008 04:55:36 PM:
>
> > On Thu, 21 Aug 2008 19:00:55 -0700 (GMT-07:00)
> > Bruce Leonard <[email protected]> wrote:
> >
> > > --- a/include/mtd/mtd-abi.h
> > > +++ b/include/mtd/mtd-abi.h
> > > @@ -6,7 +6,7 @@
> > > #define __MTD_ABI_H__
> > >
> > > struct erase_info_user {
> > > - uint32_t start;
> > > + uint64_t start;
> > > uint32_t length;
> > > };
> > >
> > > @@ -50,7 +50,7 @@ struct mtd_oob_buf {
> > > struct mtd_info_user {
> > > uint8_t type;
> > > uint32_t flags;
> > > - uint32_t size; // Total size of the MTD
> > > + uint64_t size; // Total size of the MTD
> > > uint32_t erasesize;
> > > uint32_t writesize;
> > > uint32_t oobsize; // Amount of OOB data per block (e.g. 16)
> >
> > This changes the kernel<->userspace ABI and is hence a big no-no. I
> > assume that this change will cause old userspace to
> malfunction on new
> > kernels, and vice versa.
> >
>
> Well, in my posting I noted that the mtd-utils were broken
> because of this
> but I didn't really have any idea as to how to fix things. I
> can see why
> it would be a big no-no to change this. Do you have any
> suggestions on
> what I could do differently to prevent making that change?
>
> > Supporting >2Gb MTD devices sounds useful (I'm surprised
> that we don't
> > already do so).
> >
>
> There was a LOT of interest in this over the last few months
> while I was
> working on it, but a very suprising silence has developed
> since I posted
> the patches. I guess I'm more cutting edge than I thought :).
>
> > Please cc [email protected] (at least) on MTD-related
> > patches, thanks.
>
> I started with the MTD list and then also posted to lkml when
> I realized I
> had forgotten to CC it.
>
> Thanks.
>
> Bruce
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

2008-08-27 06:12:18

by Artem Bityutskiy

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Tue, 2008-08-26 at 23:01 -0700, Tim Anderson wrote:
> I have been experimenting with this. I think there is at least 3 maybe 4
> IOCTLS that are going to need to have a LARGE mode. The 2 most obvious are
> MEMGETINFO and MEMERASE.

dwmw2 vetoed any MEMGETINFO ioctl changes, or new similar ioctls, and it
kind of make sense, because we should use sysfs instead. But this, in
turn, means implementing sysfs support for MTD, because MTD is not
LinuxDeviceModel-enabled at the moment.

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

2008-08-27 06:21:25

by Tim Anderson

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

Artem,

I see your point. Ioctls are going away after all.

So we should have something like:
/sys/class/mtd/mtd0/

Or just under device or platform?

> -----Original Message-----
> From: Artem Bityutskiy [mailto:[email protected]]
> Sent: Tuesday, August 26, 2008 11:04 PM
> To: Tim Anderson
> Cc: [email protected]; 'Andrew Morton';
> [email protected]; 'David Woodhouse';
> [email protected]; [email protected];
> 'Bruce Leonard'
> Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices
>
> On Tue, 2008-08-26 at 23:01 -0700, Tim Anderson wrote:
> > I have been experimenting with this. I think there is at
> least 3 maybe 4
> > IOCTLS that are going to need to have a LARGE mode. The 2
> most obvious are
> > MEMGETINFO and MEMERASE.
>
> dwmw2 vetoed any MEMGETINFO ioctl changes, or new similar
> ioctls, and it
> kind of make sense, because we should use sysfs instead. But this, in
> turn, means implementing sysfs support for MTD, because MTD is not
> LinuxDeviceModel-enabled at the moment.
>
> --
> Best regards,
> Artem Bityutskiy (???????? ?????)
>

2008-08-27 06:35:59

by David Woodhouse

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Tue, 2008-08-26 at 23:20 -0700, Tim Anderson wrote:
> I see your point. Ioctls are going away after all.
>
> So we should have something like:
> /sys/class/mtd/mtd0/
>
> Or just under device or platform?

Tim, please stop top-posting.

I'm not sure I'd say I _vetoed_ new ioctls, but I certainly expressed a
desire to do all the information stuff through sysfs. We would need a
new MEMERASE64 ioctl though.

--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation


2008-08-27 06:47:19

by Artem Bityutskiy

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Tue, 2008-08-26 at 23:20 -0700, Tim Anderson wrote:
> Artem,
>
> I see your point. Ioctls are going away after all.
>
> So we should have something like:
> /sys/class/mtd/mtd0/
>
> Or just under device or platform?

I am not very good in LDM, but it does not look like MTD stuff should go
to /sys/class/. I'd say it should be in /sys/devices/.

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

2008-08-27 06:55:49

by Ricard Wanderlof

[permalink] [raw]
Subject: RE: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices


On Wed, 27 Aug 2008, David Woodhouse wrote:

> On Tue, 2008-08-26 at 23:20 -0700, Tim Anderson wrote:
>> I see your point. Ioctls are going away after all.
>>
>> So we should have something like:
>> /sys/class/mtd/mtd0/
>>
>> Or just under device or platform?
>
> Tim, please stop top-posting.
>
> I'm not sure I'd say I _vetoed_ new ioctls, but I certainly expressed a
> desire to do all the information stuff through sysfs. We would need a
> new MEMERASE64 ioctl though.

If the (lack of) sysfs stuff is standing between us and a nice >4GiB mtd I
for one would think that a set of 64-bit ioctls would be a reasonable
halfway house.

/Ricard
--
Ricard Wolf Wanderl?f ricardw(at)axis.com
Axis Communications AB, Lund, Sweden http://www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30

2008-08-27 07:11:17

by Bruce Leonard

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices


On Aug 26, 2008, at 11:49 PM, Ricard Wanderlof wrote:
>
> If the (lack of) sysfs stuff is standing between us and a nice >4GiB
> mtd I for one would think that a set of 64-bit ioctls would be a
> reasonable halfway house.
>

Having started this mess, I'll throw my half euros worth in :). While
I agree that sysfs is the right way to go, I'm not going to be able to
pull it off. Not withstanding my total lack of knowledge about sysfs,
I've been very fortunate in that I've been doing all of this work on
company time. Due to project constraints, that company time is coming
to an end. I could eek out another week or two and get a set of 64-
bit ioctls, but I will not be able to spend a couple more months of
company time learning sysfs and implementing it. And I just don't
have any personal time to spare right now. I'm sorry, if it's
anything other than ioctl I just can't do it.

That being said, if a set of ioctls is acceptable, I'm more than happy
to take that on as soon as we reach a consensus.

Bruce

2008-08-27 07:17:41

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

Ricard Wanderlof wrote:
>> I'm not sure I'd say I _vetoed_ new ioctls, but I certainly expressed a
>> desire to do all the information stuff through sysfs. We would need a
>> new MEMERASE64 ioctl though.
>
> If the (lack of) sysfs stuff is standing between us and a nice >4GiB mtd
> I for one would think that a set of 64-bit ioctls would be a reasonable
> halfway house.

On the other hand we'll stay with half-solutions forever if we allow them.

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

2008-08-27 09:02:23

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Wed, 2008-08-27 at 09:39 +0100, Alan Cox wrote:
> On Tue, 26 Aug 2008 23:20:46 -0700
> "Tim Anderson" <[email protected]> wrote:
>
> > Artem,
> >
> > I see your point. Ioctls are going away after all.
>
> I don't know where that stupid story keeps coming from. Ioctl is alive
> and well and there are more not less of them. There are lots of things
> you *cannot* do with sysfs, including synchronization and handling
> many kinds of changes to objects that can appear and disappear. Ditto
> there are problems with getting a consistent snapshot via sysfs because
> you can't atomically read multiple fields.
>
> So please stop this 'ioctls are going away' stuff, its bunkum.

True, and we'll definitely need a new MEMERASE64 ioctl. But for the
_informational_ parts, those can happily be done through sysfs.

--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation


2008-08-27 09:58:18

by Alan

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Tue, 26 Aug 2008 23:20:46 -0700
"Tim Anderson" <[email protected]> wrote:

> Artem,
>
> I see your point. Ioctls are going away after all.

I don't know where that stupid story keeps coming from. Ioctl is alive
and well and there are more not less of them. There are lots of things
you *cannot* do with sysfs, including synchronization and handling
many kinds of changes to objects that can appear and disappear. Ditto
there are problems with getting a consistent snapshot via sysfs because
you can't atomically read multiple fields.

So please stop this 'ioctls are going away' stuff, its bunkum.

Alan

2008-08-27 14:34:54

by Jörn Engel

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Wed, 27 August 2008 10:01:32 +0100, David Woodhouse wrote:
>
> True, and we'll definitely need a new MEMERASE64 ioctl. But for the
> _informational_ parts, those can happily be done through sysfs.

They certainly can, but should they? There may be some reason to prefer
sysfs that should be self-evident - except that I'm a bit thick and seem
to need it spelled out. Or maybe I've just become disillusioned with
the practice of replacing crappy interfaces (ioctl here) with other
crappy interfaces (sysfs here) and having to support both for all
eternity. sysctl, ioctl, proc, sysfs, debugfs, netlink, ... -
individually they all suck in their own peculiar way. But together they
create a mess I no longer dare to name.

So what was the reason again why mtd needs two userspace interfaces
instead of just one?

Jörn

--
It does not require a majority to prevail, but rather an irate,
tireless minority keen to set brush fires in people's minds.
-- Samuel Adams

2008-08-27 14:57:28

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Wed, 2008-08-27 at 16:34 +0200, Jörn Engel wrote:
> They certainly can, but should they? There may be some reason to prefer
> sysfs that should be self-evident - except that I'm a bit thick and seem
> to need it spelled out. Or maybe I've just become disillusioned with
> the practice of replacing crappy interfaces (ioctl here) with other
> crappy interfaces (sysfs here) and having to support both for all
> eternity. sysctl, ioctl, proc, sysfs, debugfs, netlink, ... -
> individually they all suck in their own peculiar way. But together they
> create a mess I no longer dare to name.

The plus of sysfs I see is that I can add more files to expose more
information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
need to expose sub-page size to user-space, and I cannot do this with
MEMGETINFO.

> So what was the reason again why mtd needs two userspace interfaces
> instead of just one?

I would like to make udev creating MTD devices, instead of creating them
by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
files, right? This means we would have one more interface anyway.

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

2008-08-27 15:25:57

by Jörn Engel

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Wed, 27 August 2008 17:47:43 +0300, Artem Bityutskiy wrote:
>
> The plus of sysfs I see is that I can add more files to expose more
> information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
> need to expose sub-page size to user-space, and I cannot do this with
> MEMGETINFO.

sysfs makes adding new attributes easier, yes. But once added you
cannot remove the attribute again - ever. Which means that either way
you need to tread carefully and think twice before making a rash
decision.

> > So what was the reason again why mtd needs two userspace interfaces
> > instead of just one?
>
> I would like to make udev creating MTD devices, instead of creating them
> by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
> files, right? This means we would have one more interface anyway.

Could be useful, I don't mind you sending a patch. However, does this
means that MEMGETINFO64 or some other ioctl should not be done? Should
flash_erase open, read and close 8 seperate files instead of doing a
single ioctl? And should our support for large devices wait for the
sysfs support that has been talked about and not done for about two
years already?

Call me a sceptic.

Jörn

--
The cheapest, fastest and most reliable components of a computer
system are those that aren't there.
-- Gordon Bell, DEC labratories

2008-08-27 19:51:00

by Trent Piepho

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Wed, 27 Aug 2008, Artem Bityutskiy wrote:
> > On Wed, 2008-08-27 at 16:34 +0200, Jörn Engel wrote:
> > They certainly can, but should they? There may be some reason to prefer
> > sysfs that should be self-evident - except that I'm a bit thick and seem
> > to need it spelled out. Or maybe I've just become disillusioned with
> > the practice of replacing crappy interfaces (ioctl here) with other
> > crappy interfaces (sysfs here) and having to support both for all
> > eternity. sysctl, ioctl, proc, sysfs, debugfs, netlink, ... -
> > individually they all suck in their own peculiar way. But together they
> > create a mess I no longer dare to name.
>
> The plus of sysfs I see is that I can add more files to expose more
> information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
> need to expose sub-page size to user-space, and I cannot do this with
> MEMGETINFO.

You can do this with ioctls, if you designed them with extra space and
versioning from the beginning.

2008-08-29 06:02:24

by Artem Bityutskiy

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote:
> > I would like to make udev creating MTD devices, instead of creating them
> > by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
> > files, right? This means we would have one more interface anyway.
>
> Could be useful, I don't mind you sending a patch. However, does this
> means that MEMGETINFO64 or some other ioctl should not be done? Should
> flash_erase open, read and close 8 seperate files instead of doing a
> single ioctl? And should our support for large devices wait for the
> sysfs support that has been talked about and not done for about two
> years already?

Up to dwmw2, but I do not mind if the answer to all the above questions
is "yes" :-)

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

2008-08-29 10:23:54

by Jörn Engel

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Fri, 29 August 2008 08:48:07 +0300, Artem Bityutskiy wrote:
> On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote:
> >
> > Could be useful, I don't mind you sending a patch. However, does this
> > means that MEMGETINFO64 or some other ioctl should not be done? Should
> > flash_erase open, read and close 8 seperate files instead of doing a
> > single ioctl? And should our support for large devices wait for the
> > sysfs support that has been talked about and not done for about two
> > years already?
>
> Up to dwmw2, but I do not mind if the answer to all the above questions
> is "yes" :-)

Well, I personally think a "yes" to the last question would be utter
madness. Whoever answers that should better come up with an alternative
patch for sysfs support pronto.

Large flashes are not a one-off cases where a single manufacturer had a
rather bizarre design. Their numbers will continually increase. And
not supporting an ever-increasing class of hardware is simply not an
option.

Jörn

--
on a false concept. This concept is that
people actually want to look at source code.
-- Rob Enderle

2008-08-29 14:52:51

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote:
> On Wed, 27 August 2008 17:47:43 +0300, Artem Bityutskiy wrote:
> >
> > The plus of sysfs I see is that I can add more files to expose more
> > information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
> > need to expose sub-page size to user-space, and I cannot do this with
> > MEMGETINFO.
>
> sysfs makes adding new attributes easier, yes. But once added you
> cannot remove the attribute again - ever. Which means that either way
> you need to tread carefully and think twice before making a rash
> decision.

That's not _necessarily_ true, although it should certainly be done with
care. Attributes in sysfs can be optional (in a way that they can't
really be optional if they're part of a binary ioctl payload), and
userspace can cope with them being absent. The sub-page size attribute
is something which wouldn't always be present, and we could happily just
drop it and forget about it in future if we really wanted to.

> > > So what was the reason again why mtd needs two userspace interfaces
> > > instead of just one?
> >
> > I would like to make udev creating MTD devices, instead of creating them
> > by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
> > files, right? This means we would have one more interface anyway.
>
> Could be useful, I don't mind you sending a patch. However, does this
> means that MEMGETINFO64 or some other ioctl should not be done? Should
> flash_erase open, read and close 8 seperate files instead of doing a
> single ioctl?

It's hardly a fast path. And we don't have to worry about the fact that
it's non-atomic -- these things aren't exactly _changing_ over time.

> And should our support for large devices wait for the sysfs support
> that has been talked about and not done for about two years already?

You whine too much, Jörn. It doesn't take very long, as a proof of
concept, to add some attributes to the existing class support in
mtdchar.c ...

--- drivers/mtd/mtdchar.c~ 2008-07-13 22:51:29.000000000 +0100
+++ drivers/mtd/mtdchar.c 2008-08-29 13:15:08.000000000 +0100
@@ -22,12 +22,32 @@

static struct class *mtd_class;

+static ssize_t mtd_show_size(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct mtd_info *mtd = dev_get_drvdata(dev);
+ return sprintf(buf, "0x%x\n", mtd->size);
+}
+static ssize_t mtd_show_erasesize(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct mtd_info *mtd = dev_get_drvdata(dev);
+ return sprintf(buf, "0x%x\n", mtd->erasesize);
+}
+
+static DEVICE_ATTR(size, S_IRUGO, mtd_show_size, NULL);
+static DEVICE_ATTR(erasesize, S_IRUGO, mtd_show_erasesize, NULL);
+
static void mtd_notify_add(struct mtd_info* mtd)
{
+ struct device *dev;
if (!mtd)
return;

- device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index);
+ dev = device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index);
+ dev_set_drvdata(dev, mtd);
+ device_create_file(dev, &dev_attr_size);
+ device_create_file(dev, &dev_attr_erasesize);

device_create(mtd_class, NULL,
MKDEV(MTD_CHAR_MAJOR, mtd->index*2+1), "mtd%dro", mtd->index);


Ok, so it shouldn't be only for mtdchar -- it should be generic, so we
should shift some of that into the mtd core code. And we should let
people hook up the 'parent' correctly, and there are a few other things
we should do to tidy it up. But it isn't exactly hard.

--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation