[PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

This is necessary to XIP applications because vm_insert_pfn() requires
a physical address.  Allows AXFS to get the physical address on the
flash of it's partition.  Second try, see
(www.infradead.org/pipermail/linux-mtd/2006-August/016342.html)

Signed-off-by: Jared Hulbert <jaredeh@gmail.com>


diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 9c62368..af0c1ff 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -20,6 +20,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/compatmac.h>
+#include <linux/mtd/map.h>

 /* Our partition linked list */
 static LIST_HEAD(mtd_partitions);
@@ -40,6 +41,17 @@ struct mtd_part {
  */
 #define PART(x)  ((struct mtd_part *)(x))

+/*
+ * Allow other modules to get the physical address of a given partition
+ */
+void *mtd_get_partition_physaddr(struct mtd_info *mtd)
+{
+	struct mtd_part *part = PART(mtd);
+	struct map_info *map = part->master->priv;
+	
+	return (void *)(map->phys + part->offset);
+}
+EXPORT_SYMBOL_GPL(mtd_get_partition_physaddr);

 /*
  * MTD methods which simply translate the effective address and pass through
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index da6b3d6..3e9ff2c 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -52,6 +52,7 @@ struct mtd_partition {

 int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int);
 int del_mtd_partitions(struct mtd_info *);
+void *mtd_get_partition_physaddr(struct mtd_info *);

 /*
  * Functions dealing with the various ways of partitioning the space

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Thu, 26 July 2007 17:04:56 -0700, Jared Hulbert wrote:
> 
> This is necessary to XIP applications because vm_insert_pfn() requires
> a physical address.  Allows AXFS to get the physical address on the
> flash of it's partition.  Second try, see
> (www.infradead.org/pipermail/linux-mtd/2006-August/016342.html)

I believe this is wrong.  For XIP you need two things:
o the physical address and
o play nicely with other (possibly rw) users.

Your patch still allows a JFFS2 on the other partition, corrupting any
XIP pages AXFS has handed out.  No cookies.

Is there a reason you haven't used point() and unpoint()?  What is wrong
with those functions?

Jörn

-- 
This above all: to thine own self be true.
-- Shakespeare

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> Is there a reason you haven't used point() and unpoint()?

No.  In fact, I do use them.  They give me a virtual address and
protect the AXFS volume from the inevitable impolite rw filesystems on
the same chip.

> What is wrong with those functions?

They don't get me the physical address.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Fri, 27 July 2007 10:05:22 -0700, Jared Hulbert wrote:
> 
> > Is there a reason you haven't used point() and unpoint()?
> 
> No.  In fact, I do use them.  They give me a virtual address and
> protect the AXFS volume from the inevitable impolite rw filesystems on
> the same chip.

Protection makes sense.  Do you also need the virtual address?

> > What is wrong with those functions?
> 
> They don't get me the physical address.

Might be an idea to extent point() to pass a physical address as well.
Not sure.

Jörn

-- 
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> Protection makes sense.  Do you also need the virtual address?

Yes all kernel domain read would use that address range.  In the end
it is only the vm_insert_pfn() call that needs the physical.

> > > What is wrong with those functions?
> >
> > They don't get me the physical address.
>
> Might be an idea to extent point() to pass a physical address as well.
> Not sure.

I thought about that.  I don't think it's worth it.  Point() does it's
job fine.  I could add a peer to point called physaddr() that returns
an address instead of a pointer.  That really doesn't doesn't help me.
 I named it mtd_get_partition_physaddr() because all I need is to get
the physical address of the mtd partition.  And if I get around to
doing an acceptable port of XIP cramfs thats all it would need too.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Fri, 27 July 2007 13:53:42 -0700, Jared Hulbert wrote:
> 
> I thought about that.  I don't think it's worth it.  Point() does it's
> job fine.  I could add a peer to point called physaddr() that returns
> an address instead of a pointer.  That really doesn't doesn't help me.
>  I named it mtd_get_partition_physaddr() because all I need is to get
> the physical address of the mtd partition.  And if I get around to
> doing an acceptable port of XIP cramfs thats all it would need too.

Well, you always need both point() and the physical address.  But I
guess your solution is good enough for now.

Jörn

-- 
Joern's library part 2:
http://www.art.net/~hopkins/Don/unix-haters/tirix/embarrassing-memo.html

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> > I thought about that.  I don't think it's worth it.  Point() does it's
> > job fine.  I could add a peer to point called physaddr() that returns
> > an address instead of a pointer.  That really doesn't doesn't help me.
> >  I named it mtd_get_partition_physaddr() because all I need is to get
> > the physical address of the mtd partition.  And if I get around to
> > doing an acceptable port of XIP cramfs thats all it would need too.
>
> Well, you always need both point() and the physical address.  But I
> guess your solution is good enough for now.

Exactly.  See I don't really _want_ the physical address and I only
need it because the current vm routines aren't set up to use virtual
addresses.  I'm hoping eventually not to need the physical address by
fixing the vm but that's going to require changes that will take some
time to get working.

Any other ideas?  Comments?  Can I get this patch in?

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[David Woodhouse]

On Thu, 2007-07-26 at 17:04 -0700, Jared Hulbert wrote:
> +/*
> + * Allow other modules to get the physical address of a given partition
> + */
> +void *mtd_get_partition_physaddr(struct mtd_info *mtd)
> +{
> +       struct mtd_part *part = PART(mtd);
> +       struct map_info *map = part->master->priv;
> +       
> +       return (void *)(map->phys + part->offset);
> +}
> +EXPORT_SYMBOL_GPL(mtd_get_partition_physaddr); 

The problem with this is that it expects the caller to 'know' that the
mtd device it's using is in fact a partition. If we have to do this, I'd
rather see it done generically. Jörn is probably right that it should be
available from (whatever replaces) the point() method.

-- 
dwmw2

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> The problem with this is that it expects the caller to 'know' that the
> mtd device it's using is in fact a partition.

Oh I hadn't considered that a device could not be a partition and
still have physaddr be relavant.  Can it?

> If we have to do this, I'd
> rather see it done generically.

Okay.  Can you give me a little nudge in the right direction?

> Jörn is probably right that it should be
> available from (whatever replaces) the point() method.

Like what? :)  I'm all for the kinds of ideas that Jörn and others are
tossing about as replacement for better XIP awareness in the MTD.  I'm
just not catching the vision enough to start coding.

One of the major problems with working on Flash based XIP stuff for
the kernel is that there are no mergeable filesystems to use the any
frameworks we develop.  The VM still doesn't handle this correctly,
point() needs work.  There are a lot of fronts to attack.  What I was
hoping for was to get the minimum requirements for me to get AXFS
supported by the kernel, then to come back and provide more educated
and correct interfaces in the vm and mtd layers.  That way we could
have the example of how it is being done in AXFS to frame the
discussion and give it more urgency.  I'm beginning to think this
isn't the correct approach.

If that won't work then can I propose a face to face meeting with
interested parties like a mini XIP summit or something.  There are
those that are informed and interested enough to want this done right
and have a definate opinion as to what is right and wrong here, but
that don't have time to develop this.  I on the other hand have the
ability to get this coded up but am more in the dark as to the
'correct' way to do this.  If we could sit down and sketch out how to
go about this I think it would be very productive.  Potential
interested parties could include me, Woodhouse, Engel, and Otte.
Anybody else come to mind?

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Tue, 31 July 2007 12:55:23 -0700, Jared Hulbert wrote:
> 
> > The problem with this is that it expects the caller to 'know' that the
> > mtd device it's using is in fact a partition.
> 
> Oh I hadn't considered that a device could not be a partition and
> still have physaddr be relavant.  Can it?

part->master should fit the bill.

Worse, your patch guesses that mtd->priv is actually a
(struct map_info *).  I don't want to be around when that guess goes
wrong.  No cookies for you today.

> > If we have to do this, I'd
> > rather see it done generically.
> 
> Okay.  Can you give me a little nudge in the right direction?
>
> > Jörn is probably right that it should be
> > available from (whatever replaces) the point() method.

point() doesn't have to be replaced.  My completely untested and
uncompiled patch below adds another parameter to point() to return the
physical address in.  Parameter is optional and only cfi_cmdset_0001.c
currently allows it.

The patch also changes the virtual parameter from (u_char **) to
(void __iomem **).  Not sure whether that makes sense.  So no cookies
for me either.

Jörn

-- 
They laughed at Galileo.  They laughed at Copernicus.  They laughed at
Columbus. But remember, they also laughed at Bozo the Clown.
-- unknown

--- point_phys/include/linux/mtd/mtd.h~point_phys	2007-05-16 02:01:55.000000000 +0200
+++ point_phys/include/linux/mtd/mtd.h	2007-08-01 13:49:30.000000000 +0200
@@ -136,10 +136,14 @@ struct mtd_info {
 	int (*erase) (struct mtd_info *mtd, struct erase_info *instr);
 
 	/* This stuff for eXecute-In-Place */
-	int (*point) (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char **mtdbuf);
+	/* phys is optional and may be set to NULL */
+	int (*point) (struct mtd_info *mtd, loff_t from, size_t len,
+			size_t *retlen, void __iomem **virt,
+			resource_size_t *phys);
 
 	/* We probably shouldn't allow XIP if the unpoint isn't a NULL */
-	void (*unpoint) (struct mtd_info *mtd, u_char * addr, loff_t from, size_t len);
+	void (*unpoint) (struct mtd_info *mtd, void *addr, loff_t from,
+			size_t len);
 
 
 	int (*read) (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf);
--- point_phys/drivers/mtd/chips/cfi_cmdset_0001.c~point_phys	2007-05-16 02:01:27.000000000 +0200
+++ point_phys/drivers/mtd/chips/cfi_cmdset_0001.c	2007-08-01 13:42:05.000000000 +0200
@@ -81,7 +81,7 @@ static struct mtd_info *cfi_intelext_set
 static int cfi_intelext_partition_fixup(struct mtd_info *, struct cfi_private **);
 
 static int cfi_intelext_point (struct mtd_info *mtd, loff_t from, size_t len,
-		     size_t *retlen, u_char **mtdbuf);
+		     size_t *retlen, void __iomem**virt, resource_size_t *phys);
 static void cfi_intelext_unpoint (struct mtd_info *mtd, u_char *addr, loff_t from,
 			size_t len);
 
@@ -1162,7 +1162,8 @@ static int do_point_onechip (struct map_
 	return ret;
 }
 
-static int cfi_intelext_point (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char **mtdbuf)
+static int cfi_intelext_point (struct mtd_info *mtd, loff_t from, size_t len,
+		size_t *retlen, void __iomem **virt, resource_size_t *phys)
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
@@ -1173,7 +1174,9 @@ static int cfi_intelext_point (struct mt
 	if (!map->virt || (from + len > mtd->size))
 		return -EINVAL;
 
-	*mtdbuf = (void *)map->virt + from;
+	*virt = map->virt + from;
+	if (phys)
+		*phys = map->phys + from;
 	*retlen = 0;
 
 	/* Now lock the chip(s) to POINT state */
--- point_phys/drivers/mtd/mtdpart.c~point_phys	2007-05-16 02:01:27.000000000 +0200
+++ point_phys/drivers/mtd/mtdpart.c	2007-08-01 13:37:22.000000000 +0200
@@ -68,7 +68,8 @@ static int part_read (struct mtd_info *m
 }
 
 static int part_point (struct mtd_info *mtd, loff_t from, size_t len,
-			size_t *retlen, u_char **buf)
+			size_t *retlen, void __iomem **virt,
+			resource_size_t *phys)
 {
 	struct mtd_part *part = PART(mtd);
 	if (from >= mtd->size)
@@ -76,7 +77,7 @@ static int part_point (struct mtd_info *
 	else if (from + len > mtd->size)
 		len = mtd->size - from;
 	return part->master->point (part->master, from + part->offset,
-				    len, retlen, buf);
+				    len, retlen, virt, phys);
 }
 
 static void part_unpoint (struct mtd_info *mtd, u_char *addr, loff_t from, size_t len)
--- point_phys/drivers/mtd/devices/pmc551.c~point_phys	2007-07-10 16:28:31.000000000 +0200
+++ point_phys/drivers/mtd/devices/pmc551.c	2007-08-01 13:32:50.000000000 +0200
@@ -134,7 +134,7 @@ static int pmc551_erase(struct mtd_info 
 	eoff_lo = end & (priv->asize - 1);
 	soff_lo = instr->addr & (priv->asize - 1);
 
-	pmc551_point(mtd, instr->addr, instr->len, &retlen, &ptr);
+	pmc551_point(mtd, instr->addr, instr->len, &retlen, &ptr, NULL);
 
 	if (soff_hi == eoff_hi || mtd->size == priv->asize) {
 		/* The whole thing fits within one access, so just one shot
@@ -154,7 +154,7 @@ static int pmc551_erase(struct mtd_info 
 			}
 			soff_hi += priv->asize;
 			pmc551_point(mtd, (priv->base_map0 | soff_hi),
-				     priv->asize, &retlen, &ptr);
+				     priv->asize, &retlen, &ptr, NULL);
 		}
 		memset(ptr, 0xff, eoff_lo);
 	}
@@ -170,12 +170,16 @@ static int pmc551_erase(struct mtd_info 
 }
 
 static int pmc551_point(struct mtd_info *mtd, loff_t from, size_t len,
-			size_t * retlen, u_char ** mtdbuf)
+		size_t * retlen, void __iomem **virt, resource_size_t *phys)
 {
 	struct mypriv *priv = mtd->priv;
 	u32 soff_hi;
 	u32 soff_lo;
 
+	/* anyone needing this has to fix the driver, sorry */
+	if (phys)
+		return -EINVAL;
+
 #ifdef CONFIG_MTD_PMC551_DEBUG
 	printk(KERN_DEBUG "pmc551_point(%ld, %ld)\n", (long)from, (long)len);
 #endif
@@ -198,7 +202,7 @@ static int pmc551_point(struct mtd_info 
 		priv->curr_map0 = soff_hi;
 	}
 
-	*mtdbuf = priv->start + soff_lo;
+	*virt = priv->start + soff_lo;
 	*retlen = len;
 	return 0;
 }
@@ -242,7 +246,7 @@ static int pmc551_read(struct mtd_info *
 	soff_lo = from & (priv->asize - 1);
 	eoff_lo = end & (priv->asize - 1);
 
-	pmc551_point(mtd, from, len, retlen, &ptr);
+	pmc551_point(mtd, from, len, retlen, &ptr, NULL);
 
 	if (soff_hi == eoff_hi) {
 		/* The whole thing fits within one access, so just one shot
@@ -263,7 +267,7 @@ static int pmc551_read(struct mtd_info *
 				goto out;
 			}
 			soff_hi += priv->asize;
-			pmc551_point(mtd, soff_hi, priv->asize, retlen, &ptr);
+			pmc551_point(mtd, soff_hi, priv->asize, retlen, &ptr, NULL);
 		}
 		memcpy(copyto, ptr, eoff_lo);
 		copyto += eoff_lo;
@@ -308,7 +312,7 @@ static int pmc551_write(struct mtd_info 
 	soff_lo = to & (priv->asize - 1);
 	eoff_lo = end & (priv->asize - 1);
 
-	pmc551_point(mtd, to, len, retlen, &ptr);
+	pmc551_point(mtd, to, len, retlen, &ptr, NULL);
 
 	if (soff_hi == eoff_hi) {
 		/* The whole thing fits within one access, so just one shot
@@ -329,7 +333,7 @@ static int pmc551_write(struct mtd_info 
 				goto out;
 			}
 			soff_hi += priv->asize;
-			pmc551_point(mtd, soff_hi, priv->asize, retlen, &ptr);
+			pmc551_point(mtd, soff_hi, priv->asize, retlen, &ptr, NULL);
 		}
 		memcpy(ptr, copyfrom, eoff_lo);
 		copyfrom += eoff_lo;
--- point_phys/drivers/mtd/devices/phram.c~point_phys	2007-02-12 11:33:26.000000000 +0100
+++ point_phys/drivers/mtd/devices/phram.c	2007-08-01 13:34:18.000000000 +0200
@@ -57,14 +57,17 @@ static int phram_erase(struct mtd_info *
 }
 
 static int phram_point(struct mtd_info *mtd, loff_t from, size_t len,
-		size_t *retlen, u_char **mtdbuf)
+		size_t *retlen, void __iomem **virt, resource_size_t *phys)
 {
 	u_char *start = mtd->priv;
 
+	if (phys)
+		return -EINVAL; /* FIXME */
+
 	if (from + len > mtd->size)
 		return -EINVAL;
 
-	*mtdbuf = start + from;
+	*virt = start + from;
 	*retlen = len;
 	return 0;
 }
--- point_phys/drivers/mtd/devices/mtdram.c~point_phys	2006-10-13 15:58:41.000000000 +0200
+++ point_phys/drivers/mtd/devices/mtdram.c	2007-08-01 13:34:57.000000000 +0200
@@ -47,12 +47,12 @@ static int ram_erase(struct mtd_info *mt
 }
 
 static int ram_point(struct mtd_info *mtd, loff_t from, size_t len,
-		size_t *retlen, u_char **mtdbuf)
+		size_t *retlen, void __iomem **virt, resource_size_t *phys)
 {
 	if (from + len > mtd->size)
 		return -EINVAL;
 
-	*mtdbuf = mtd->priv + from;
+	*virt = mtd->priv + from;
 	*retlen = len;
 	return 0;
 }
--- point_phys/drivers/mtd/devices/slram.c~point_phys	2007-02-19 23:16:28.000000000 +0100
+++ point_phys/drivers/mtd/devices/slram.c	2007-08-01 13:35:33.000000000 +0200
@@ -104,14 +104,17 @@ static int slram_erase(struct mtd_info *
 }
 
 static int slram_point(struct mtd_info *mtd, loff_t from, size_t len,
-		size_t *retlen, u_char **mtdbuf)
+		size_t *retlen, void __iomem **virt, resource_size_t *phys)
 {
 	slram_priv_t *priv = mtd->priv;
 
+	if (phys)
+		return -EINVAL;
+
 	if (from + len > mtd->size)
 		return -EINVAL;
 
-	*mtdbuf = priv->start + from;
+	*virt = priv->start + from;
 	*retlen = len;
 	return(0);
 }
--- point_phys/drivers/mtd/maps/uclinux.c~point_phys	2007-07-10 16:28:31.000000000 +0200
+++ point_phys/drivers/mtd/maps/uclinux.c	2007-08-01 13:39:22.000000000 +0200
@@ -40,10 +40,12 @@ struct mtd_partition uclinux_romfs[] = {
 /****************************************************************************/
 
 int uclinux_point(struct mtd_info *mtd, loff_t from, size_t len,
-	size_t *retlen, u_char **mtdbuf)
+	size_t *retlen, void __iomem **virt, resource_size_t *phys)
 {
 	struct map_info *map = mtd->priv;
-	*mtdbuf = (u_char *) (map->virt + ((int) from));
+	*virt = map->virt + from;
+	if (phys)
+		*phys = map->phys + from;
 	*retlen = len;
 	return(0);
 }
--- point_phys/fs/jffs2/scan.c~point_phys	2007-05-16 02:01:39.000000000 +0200
+++ point_phys/fs/jffs2/scan.c	2007-08-01 13:40:37.000000000 +0200
@@ -97,7 +97,7 @@ int jffs2_scan_medium(struct jffs2_sb_in
 	size_t pointlen;
 
 	if (c->mtd->point) {
-		ret = c->mtd->point (c->mtd, 0, c->mtd->size, &pointlen, &flashbuf);
+		ret = c->mtd->point (c->mtd, 0, c->mtd->size, &pointlen, &flashbuf, NULL);
 		if (!ret && pointlen < c->mtd->size) {
 			/* Don't muck about if it won't let us point to the whole flash */
 			D1(printk(KERN_DEBUG "MTD point returned len too short: 0x%zx\n", pointlen));
--- point_phys/fs/jffs2/readinode.c~point_phys	2007-07-10 16:28:35.000000000 +0200
+++ point_phys/fs/jffs2/readinode.c	2007-08-01 13:41:34.000000000 +0200
@@ -62,7 +62,7 @@ static int check_node_data(struct jffs2_
 	/* TODO: instead, incapsulate point() stuff to jffs2_flash_read(),
 	 * adding and jffs2_flash_read_end() interface. */
 	if (c->mtd->point) {
-		err = c->mtd->point(c->mtd, ofs, len, &retlen, &buffer);
+		err = c->mtd->point(c->mtd, ofs, len, &retlen, &buffer, NULL);
 		if (!err && retlen < tn->csize) {
 			JFFS2_WARNING("MTD point returned len too short: %zu instead of %u.\n", retlen, tn->csize);
 			c->mtd->unpoint(c->mtd, buffer, ofs, len);

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> --- point_phys/include/linux/mtd/mtd.h~point_phys       2007-05-16 02:01:55.000000000 +0200
> +++ point_phys/include/linux/mtd/mtd.h  2007-08-01 13:49:30.000000000 +0200
> @@ -136,10 +136,14 @@ struct mtd_info {
>         int (*erase) (struct mtd_info *mtd, struct erase_info *instr);
>
>         /* This stuff for eXecute-In-Place */
> -       int (*point) (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char **mtdbuf);
> +       /* phys is optional and may be set to NULL */
> +       int (*point) (struct mtd_info *mtd, loff_t from, size_t len,
> +                       size_t *retlen, void __iomem **virt,
> +                       resource_size_t *phys);

Okay so I think __iomem is correct but I'm not sure I understand all
the ramifications downstream from here.

Why resource_size_t?

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Thu, 2 August 2007 18:56:46 -0700, Jared Hulbert wrote:
> 
> > --- point_phys/include/linux/mtd/mtd.h~point_phys       2007-05-16 02:01:55.000000000 +0200
> > +++ point_phys/include/linux/mtd/mtd.h  2007-08-01 13:49:30.000000000 +0200
> > @@ -136,10 +136,14 @@ struct mtd_info {
> >         int (*erase) (struct mtd_info *mtd, struct erase_info *instr);
> >
> >         /* This stuff for eXecute-In-Place */
> > -       int (*point) (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char **mtdbuf);
> > +       /* phys is optional and may be set to NULL */
> > +       int (*point) (struct mtd_info *mtd, loff_t from, size_t len,
> > +                       size_t *retlen, void __iomem **virt,
> > +                       resource_size_t *phys);
> 
> Okay so I think __iomem is correct but I'm not sure I understand all
> the ramifications downstream from here.

Interestingly __iomem makes less sense for some RAM-based drivers.  So
maybe that part should get removed.  The whole patch was just a quick
hack to get an opinion.

> Why resource_size_t?

Because struct map_info has that type for phys.

Jörn

-- 
Joern's library part 2:
http://www.art.net/~hopkins/Don/unix-haters/tirix/embarrassing-memo.html

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> Interestingly __iomem makes less sense for some RAM-based drivers.  So
> maybe that part should get removed.

(http://marc.info/?l=linux-arm-kernel&m=118193919321290&w=4)
  Russel King said:
-->
-->   const void *start = virt + offset;
-->
--> This will generate a sparse warning (since you haven't used sparse you
--> won't know about this.)  The __iomem annotation does nothing as far as
--> the C language goes, but sparse uses it to annotate variables as to
--> which address space they refer to, and issues warnings if you try to
--> cast between different address spaces.  However, it does allow code
--> which knows what it's doing to do such casts.
-->
-->   const void *start = (void __force *)virt + offset;
-->
--> MTD people may like to make a note that the 'cached' element of struct
--> map_info is assigned __iomem pointers (from ioremap_cached) but is
--> itself not marked with an __iomem pointer.  This will also produce
--> sparse warnings.

Now I think maybe it should be "void **virt" not "void __iomem **virt"
and then just __force virt.  Anybody know?

> The whole patch was just a quick hack to get an opinion.

Trying to move it forward...

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Thu, 2 August 2007 22:23:47 -0700, Jared Hulbert wrote:
> 
> > Interestingly __iomem makes less sense for some RAM-based drivers.  So
> > maybe that part should get removed.
> 
> (http://marc.info/?l=linux-arm-kernel&m=118193919321290&w=4)
>   Russel King said:
> -->
> -->   const void *start = virt + offset;
> -->
> --> This will generate a sparse warning (since you haven't used sparse you
> --> won't know about this.)  The __iomem annotation does nothing as far as
> --> the C language goes, but sparse uses it to annotate variables as to
> --> which address space they refer to, and issues warnings if you try to
> --> cast between different address spaces.  However, it does allow code
> --> which knows what it's doing to do such casts.
> -->
> -->   const void *start = (void __force *)virt + offset;
> -->
> --> MTD people may like to make a note that the 'cached' element of struct
> --> map_info is assigned __iomem pointers (from ioremap_cached) but is
> --> itself not marked with an __iomem pointer.  This will also produce
> --> sparse warnings.
> 
> Now I think maybe it should be "void **virt" not "void __iomem **virt"
> and then just __force virt.  Anybody know?

Just remove it.  Even if the change made sense it should follow in a
seperate patch.

> > The whole patch was just a quick hack to get an opinion.
> 
> Trying to move it forward...

Great!

Jörn

-- 
It does not matter how slowly you go, so long as you do not stop.
-- Confucius

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> point() doesn't have to be replaced.  My completely untested and
> uncompiled patch below adds another parameter to point() to return the
> physical address in.  Parameter is optional and only cfi_cmdset_0001.c
> currently allows it.

Hey I just thought of a different approach.  What about something like:

int mtd_insert_mem(struct mtd_info *mtd, struct vm_area_struct *vma,
unsigned long addr, unsigned long offset)
{
     struct map_info *map = mtd->priv;
     unsigned long pfn;

     pfn = (map->phys + offset) >> PAGE_SHIFT;
     return vm_insert_pfn(vma, addr, pfn);
}



This way we don't need to export the physical address and we keep the
vm calls in the driver world.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Thu, 2 August 2007 23:42:06 -0700, Jared Hulbert wrote:
> 
> > point() doesn't have to be replaced.  My completely untested and
> > uncompiled patch below adds another parameter to point() to return the
> > physical address in.  Parameter is optional and only cfi_cmdset_0001.c
> > currently allows it.
> 
> Hey I just thought of a different approach.  What about something like:
> 
> int mtd_insert_mem(struct mtd_info *mtd, struct vm_area_struct *vma,
> unsigned long addr, unsigned long offset)
> {
>      struct map_info *map = mtd->priv;
>      unsigned long pfn;
> 
>      pfn = (map->phys + offset) >> PAGE_SHIFT;
>      return vm_insert_pfn(vma, addr, pfn);
> }
> 
> This way we don't need to export the physical address and we keep the
> vm calls in the driver world.

Hard to tell without looking at the AXFS code as well.  And it might
trigger some janitor to remove this "completely unused function" shortly
after introducing it.

How impolite would it be to ask for AXFS again? :)

Jörn

-- 
"Security vulnerabilities are here to stay."
-- Scott Culp, Manager of the Microsoft Security Response Center, 2001

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

On 8/3/07, Jörn Engel <joern@logfs.org> wrote:
> On Thu, 2 August 2007 23:42:06 -0700, Jared Hulbert wrote:
> >
> > > point() doesn't have to be replaced.  My completely untested and
> > > uncompiled patch below adds another parameter to point() to return the
> > > physical address in.  Parameter is optional and only cfi_cmdset_0001.c
> > > currently allows it.
> >
> > Hey I just thought of a different approach.  What about something like:
> >
> > int mtd_insert_mem(struct mtd_info *mtd, struct vm_area_struct *vma,
> > unsigned long addr, unsigned long offset)
> > {
> >      struct map_info *map = mtd->priv;
> >      unsigned long pfn;
> >
> >      pfn = (map->phys + offset) >> PAGE_SHIFT;
> >      return vm_insert_pfn(vma, addr, pfn);
> > }
> >
> > This way we don't need to export the physical address and we keep the
> > vm calls in the driver world.
>
> Hard to tell without looking at the AXFS code as well.  And it might
> trigger some janitor to remove this "completely unused function" shortly
> after introducing it.

Of course. I don't mind tracking it out of tree for a few weeks as
long after we agree on the solution here.

> How impolite would it be to ask for AXFS again? :)

I missed the cue the first time around, sorry.  Just keep asking until
it gets through my thick head....

I'll put a clean drop together on http://axfs.sf.net

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

I ignored this in my previous reply.

On Tue, 31 July 2007 12:55:23 -0700, Jared Hulbert wrote:
> 
> Like what? :)  I'm all for the kinds of ideas that Jörn and others are
> tossing about as replacement for better XIP awareness in the MTD.  I'm
> just not catching the vision enough to start coding.

Neither am I.  My missing puzzle piece is how to zap all page mappings
for the chip in question.  Maybe the fs/vm meeting at lce will clear
things up.

> One of the major problems with working on Flash based XIP stuff for
> the kernel is that there are no mergeable filesystems to use the any
> frameworks we develop.  The VM still doesn't handle this correctly,
> point() needs work.  There are a lot of fronts to attack.  What I was
> hoping for was to get the minimum requirements for me to get AXFS
> supported by the kernel, then to come back and provide more educated
> and correct interfaces in the vm and mtd layers.  That way we could
> have the example of how it is being done in AXFS to frame the
> discussion and give it more urgency.  I'm beginning to think this
> isn't the correct approach.

If I started working on this I'd probably want AXFS on one partition and
JFFS2 on another.  Being able to write to JFFS2 while reading XIP files
from AXFS without corruptions is the major milestone.  When that works
we can think about the other 90% of having both write support and XIP in
the same filesystem.

How serious is Intel about merging AXFS in mainline, btw?

> If that won't work then can I propose a face to face meeting with
> interested parties like a mini XIP summit or something.  There are
> those that are informed and interested enough to want this done right
> and have a definate opinion as to what is right and wrong here, but
> that don't have time to develop this.  I on the other hand have the
> ability to get this coded up but am more in the dark as to the
> 'correct' way to do this.  If we could sit down and sketch out how to
> go about this I think it would be very productive.  Potential
> interested parties could include me, Woodhouse, Engel, and Otte.
> Anybody else come to mind?

Maybe another vm person or two.  The vm part is the critical missing
piece and needs all the brainpower we can get.

Jörn

-- 
Joern's library part 1:
http://lwn.net/Articles/2.6-kernel-api/

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Carsten Otte]

Jörn Engel wrote:
> Neither am I.  My missing puzzle piece is how to zap all page mappings
> for the chip in question.  Maybe the fs/vm meeting at lce will clear
> things up.
I don't think that one is particular hard to do. All you need is:
- count refernces in the driver
- make the xip code return its references after use
- a callback that the driver can call to get its references that are
   mapped to userland back which would simply boil down to 
filemap_xip_unmap()
- the driver should stop handing out new references for file 
operations when calling the callback, and the reference count should 
become zero pretty soon.
I'd be happy to do the xip infrastructure parts of this.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> I don't think that one is particular hard to do. All you need is:
> - count refernces in the driver

References to what? pages? pfn's?

> - make the xip code return its references after use
> - a callback that the driver can call to get its references that are
>    mapped to userland back which would simply boil down to
>    filemap_xip_unmap()

Hmm.  See problem is we don't always want to just 'zap' pages, as
such.  Though that would be the brute force straight forward approach.
 I think there is an optimization to target.

Assume we need to erase a block, this definately requires that all
pages in that block be 'zapped'.

With most flashes the erase operation takes chunks of the array larger
than the effected block offline temporarily.  Right now XIP support in
MTD just spins waiting for an interrupt.  When one is recieved the
erase is suspended and normal execution continues with just the block
being erased in an unknown state.  Then when the coast is clear the
erase is resumed.

So we have a large number of pages that are not being changed but that
are not availiable for some time.  I think it would be much better to
identify those pages that will be taken offline and change the page
tables such that a fault can be directed to the FS and then to the MTD
to do the suspend.

The reason would be fault avoidance.  See once the erase completes all
the old valid pages should be mapped back as they were.  Why not allow
the kernel to continue (assuming it isn't taken offline!) executing as
normal and why interrupt the erase until you know for sure you need to
access effected pages?

> - the driver should stop handing out new references for file
> operations when calling the callback, and the reference count should
> become zero pretty soon.
> I'd be happy to do the xip infrastructure parts of this.

Great!

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Wed, 1 August 2007 13:37:57 -0700, Jared Hulbert wrote:
> 
> > I don't think that one is particular hard to do. All you need is:
> > - count refernces in the driver
> 
> References to what? pages? pfn's?
> 
> > - make the xip code return its references after use
> > - a callback that the driver can call to get its references that are
> >    mapped to userland back which would simply boil down to
> >    filemap_xip_unmap()

How does filemap_xip_unmap() work and what arguments will it take?

The only implementation I can visualize right now would be to go through
all page mappings for all processes and removing the mapping if it
points to the physical address range in question.  Extremely slow and
racing with fork() and clone().

So we would need something better.  Just what?  Do you have a better
idea already, Carsten?

Jörn

-- 
Don't patch bad code, rewrite it.
-- Kernigham and Pike, according to Rusty

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Carsten Otte]

Jared Hulbert wrote:
> References to what? pages? pfn's?
Currently we use struct page in our address space operation, but 
moving to pfn seems advisable to me.

> Assume we need to erase a block, this definately requires that all
> pages in that block be 'zapped'.
True.

> With most flashes the erase operation takes chunks of the array larger
> than the effected block offline temporarily.  Right now XIP support in
> MTD just spins waiting for an interrupt.  When one is recieved the
> erase is suspended and normal execution continues with just the block
> being erased in an unknown state.  Then when the coast is clear the
> erase is resumed.
That does'nt work on smp machines, does it? Sounds like a hack to me.

> So we have a large number of pages that are not being changed but that
> are not availiable for some time.  I think it would be much better to
> identify those pages that will be taken offline and change the page
> tables such that a fault can be directed to the FS and then to the MTD
> to do the suspend.
> 
> The reason would be fault avoidance.  See once the erase completes all
> the old valid pages should be mapped back as they were.  Why not allow
> the kernel to continue (assuming it isn't taken offline!) executing as
> normal and why interrupt the erase until you know for sure you need to
> access effected pages?
How do you know a user app that has a valid pte to your flash media 
accesses it? Userspace may do so at any time it wants, and as far as I 
understand Joerns and Davids explanations on flash it will just see 
invalid data rather than raising a page fault.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> That does'nt work on smp machines, does it?

Probably not.  That code is machine specific, and only implemented for
a couple of ARM targets.

> Sounds like a hack to me.

:)  Yes but it works where it needs to.  It's done a lot in the cellular world.

> How do you know a user app that has a valid pte to your flash media
> accesses it? Userspace may do so at any time it wants, and as far as I
> understand Joerns and Davids explanations on flash it will just see
> invalid data rather than raising a page fault.

Maybe I wasn't so clear.  Assuming the MTD is going to do an erase you would:

(a) zap all page affected by block erase
(b) identify all pte's mapped to the affected region
(c) modify these pte's so they now trigger a fault
(d) wait for completion of erase
(c) reestablish valid pte mappings


If you get a fault it gets routed to the filesystem .fault or
something like that.  The .fault routine can:
(a) suspend the erase
(b) copy the page to RAM
(c) update the pte to point to RAM
(d) resume the erase
-or-
(a) suspend the erase
(b) reenable the pte
(c) wait a short time
(d) disable pte again
(e) resume the erase

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Carsten Otte]

Jared Hulbert wrote:
> Maybe I wasn't so clear.  Assuming the MTD is going to do an erase you would:
> 
> (a) zap all page affected by block erase
> (b) identify all pte's mapped to the affected region
> (c) modify these pte's so they now trigger a fault
> (d) wait for completion of erase
> (c) reestablish valid pte mappings
Ah. Now I can see the light. Sounds like an acceptable approach to me.

> If you get a fault it gets routed to the filesystem .fault or
> something like that.  The .fault routine can:
> (a) suspend the erase
> (b) copy the page to RAM
> (c) update the pte to point to RAM
> (d) resume the erase
> -or-
> (a) suspend the erase
> (b) reenable the pte
> (c) wait a short time
> (d) disable pte again
> (e) resume the erase
Why don't we wait til the erase is completed?

so long,
Carsten

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Fri, 3 August 2007 09:59:17 +0200, Carsten Otte wrote:
> Jared Hulbert wrote:
> 
> >If you get a fault it gets routed to the filesystem .fault or
> >something like that.  The .fault routine can:
> >(a) suspend the erase
> >(b) copy the page to RAM
> >(c) update the pte to point to RAM
> >(d) resume the erase
> >-or-
> >(a) suspend the erase
> >(b) reenable the pte
> >(c) wait a short time
> >(d) disable pte again
> >(e) resume the erase
> Why don't we wait til the erase is completed?

An erase can take up to 5 seconds.

Jörn

-- 
Anything that can go wrong, will.
-- Finagle's Law

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Carsten Otte]

Jörn Engel wrote:
> An erase can take up to 5 seconds.
I see. Trouble for "real time" applications ;-). But once we're 
receiving a lot of page faults we end up suspending the erase forever. 
We need a deterministic end of the erase cycle as well, don't we?

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Fri, 3 August 2007 13:03:49 +0200, Carsten Otte wrote:
> Jörn Engel wrote:
> >An erase can take up to 5 seconds.
> I see. Trouble for "real time" applications ;-). But once we're 
> receiving a lot of page faults we end up suspending the erase forever. 
> We need a deterministic end of the erase cycle as well, don't we?

Absolutely.

Jörn

-- 
Beware of bugs in the above code; I have only proved it correct, but
not tried it.
-- Donald Knuth

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Carsten Otte]

Jörn Engel wrote:
> On Fri, 3 August 2007 13:03:49 +0200, Carsten Otte wrote:
>> I see. Trouble for "real time" applications ;-). But once we're 
>> receiving a lot of page faults we end up suspending the erase forever. 
>> We need a deterministic end of the erase cycle as well, don't we?
> Absolutely.
Hmm. At least this does'nt seem like something that the xip 
infrastructure should be concerned with: it will ask for 
get_xip_page() [or get_xip_pfn()], and return temporary references via 
put_xip_page/pfn(). The mtd layer has to decide when to pospone 
get_xip_page() calls and when to ask for references being returned. On 
the other hand, I think we should make sure get_xip_page() targets may 
call schedule() in case the page we ask for is currently unavailable 
while an erase operation is in progress.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Fri, 3 August 2007 14:21:54 +0200, Carsten Otte wrote:
> 
> Hmm. At least this does'nt seem like something that the xip 
> infrastructure should be concerned with: it will ask for 
> get_xip_page() [or get_xip_pfn()], and return temporary references via 
> put_xip_page/pfn(). The mtd layer has to decide when to pospone 
> get_xip_page() calls and when to ask for references being returned. On 
> the other hand, I think we should make sure get_xip_page() targets may 
> call schedule() in case the page we ask for is currently unavailable 
> while an erase operation is in progress.

Why schedule()?  I believe we should not wait but process the fault
immediatly via one of the methods Jared mentioned:

>> If you get a fault it gets routed to the filesystem .fault or
>> something like that.  The .fault routine can:
>> (a) suspend the erase
>> (b) copy the page to RAM
>> (c) update the pte to point to RAM
>> (d) resume the erase
>> -or-
>> (a) suspend the erase
>> (b) reenable the pte
>> (c) wait a short time
>> (d) disable pte again
>> (e) resume the erase

The second variant could be a bit hairy.  How can one ensure that (c)
only waits a short while and not much longer?  So I tend to prefer the
first variant.

Jörn

-- 
Courage is not the absence of fear, but rather the judgement that
something else is more important than fear.
-- Ambrose Redmoon

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[David Woodhouse]

On Thu, 2007-08-02 at 14:55 -0700, Jared Hulbert wrote:
> > That does'nt work on smp machines, does it?
> 
> Probably not.  That code is machine specific, and only implemented for
> a couple of ARM targets.
> 
> > Sounds like a hack to me.
> 
> :)  Yes but it works where it needs to.  It's done a lot in the cellular world.
> 
> > How do you know a user app that has a valid pte to your flash media
> > accesses it? Userspace may do so at any time it wants, and as far as I
> > understand Joerns and Davids explanations on flash it will just see
> > invalid data rather than raising a page fault.
> 
> Maybe I wasn't so clear.  Assuming the MTD is going to do an erase you would:
> 
> (a) zap all page affected by block erase
> (b) identify all pte's mapped to the affected region
> (c) modify these pte's so they now trigger a fault
> (d) wait for completion of erase
> (c) reestablish valid pte mappings

No need to reestablish them -- they'll fault when they need to, which
means less work next time you erase. And it'll be a minor page fault,
which is quite cheap -- it only needs to set up the pte and return.

> If you get a fault it gets routed to the filesystem .fault or
> something like that.  The .fault routine can:
> (a) suspend the erase
> (b) copy the page to RAM
> (c) update the pte to point to RAM
> (d) resume the erase

This could lead to all your data pages ending up in RAM, which kind of
defeats the object of XIP.

> -or-
> (a) suspend the erase
> (b) reenable the pte
> (c) wait a short time
> (d) disable pte again
> (e) resume the erase

Alternatively, just wait for the chip to become available. Or increment
a count of 'waiters' and only interrupt the erase when there are a
certain number of people waiting.

Perhaps we want to set it up so that _if_ there is a process which can
continue, it'll do so. Only when there's nothing runnable in userspace
do we suspend an erase? In that case, perhaps we'd want some kind of
hook in the idle loop?

-- 
dwmw2

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Fri, 3 August 2007 14:09:03 +0100, David Woodhouse wrote:
> 
> > If you get a fault it gets routed to the filesystem .fault or
> > something like that.  The .fault routine can:
> > (a) suspend the erase
> > (b) copy the page to RAM
> > (c) update the pte to point to RAM
> > (d) resume the erase
> 
> This could lead to all your data pages ending up in RAM, which kind of
> defeats the object of XIP.
> 
> > -or-
> > (a) suspend the erase
> > (b) reenable the pte
> > (c) wait a short time
> > (d) disable pte again
> > (e) resume the erase
> 
> Alternatively, just wait for the chip to become available. Or increment
> a count of 'waiters' and only interrupt the erase when there are a
> certain number of people waiting.
> 
> Perhaps we want to set it up so that _if_ there is a process which can
> continue, it'll do so. Only when there's nothing runnable in userspace
> do we suspend an erase? In that case, perhaps we'd want some kind of
> hook in the idle loop?

Or we could just always suspend the erase and resume it the next time we
enter the kernel, either via system call or interrupt.  Should allow the
process to make some progress without starving the erase.  Would need
hooks as well, just in different places.

Jörn

-- 
This above all: to thine own self be true.
-- Shakespeare

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> > > (a) suspend the erase
> > > (b) reenable the pte
> > > (c) wait a short time
> > > (d) disable pte again
> > > (e) resume the erase
> >
> > Alternatively, just wait for the chip to become available. Or increment
> > a count of 'waiters' and only interrupt the erase when there are a
> > certain number of people waiting.

We can't very well just wait seconds for the chip to become available.
 We could count waiting processes, but even it there is one process in
the queue, seconds is a long time to wait.

> > Perhaps we want to set it up so that _if_ there is a process which can
> > continue, it'll do so. Only when there's nothing runnable in userspace
> > do we suspend an erase? In that case, perhaps we'd want some kind of
> > hook in the idle loop?
>
> Or we could just always suspend the erase and resume it the next time we
> enter the kernel, either via system call or interrupt.  Should allow the
> process to make some progress without starving the erase.  Would need
> hooks as well, just in different places.

Hmmm, we'd definitely benefit from having a hook in idle to trigger
the erase to resume.  Having hooks for entry into kernel space sounds
interesting, but if you leave it too quickly and suspend the erase you
don't make forward progress.  However, I'm not sure how you'd
determine whether a given process can continue without suspending the
erase.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jörn Engel]

On Fri, 3 August 2007 12:45:45 -0700, Jared Hulbert wrote:
> 
> > > Perhaps we want to set it up so that _if_ there is a process which can
> > > continue, it'll do so. Only when there's nothing runnable in userspace
> > > do we suspend an erase? In that case, perhaps we'd want some kind of
> > > hook in the idle loop?
> >
> > Or we could just always suspend the erase and resume it the next time we
> > enter the kernel, either via system call or interrupt.  Should allow the
> > process to make some progress without starving the erase.  Would need
> > hooks as well, just in different places.
> 
> Hmmm, we'd definitely benefit from having a hook in idle to trigger
> the erase to resume.  Having hooks for entry into kernel space sounds
> interesting, but if you leave it too quickly and suspend the erase you
> don't make forward progress.  However, I'm not sure how you'd
> determine whether a given process can continue without suspending the
> erase.

Does the system ever idle in a userspace process?  If not then you'd
have to come by one of my hooks before going into idle.

Even better, if the process does a system call, or an interrupt happens,
it will not access the XIP pages while the syscall/interrupt is
processed.  So the erase may as well resume for the time being.  And
potentially it can even run a little longer if no XIP faults happen.

The big question is how such a scheme would perform.  It may turn out so
bad that we should rather copy the data, hand out RAM and wait for
memory pressure to remove those pages again.

Jörn

-- 
I don't understand it. Nobody does.
-- Richard P. Feynman

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[David Woodhouse]

On Fri, 2007-08-03 at 12:45 -0700, Jared Hulbert wrote:
> We can't very well just wait seconds for the chip to become available.
>  We could count waiting processes, but even it there is one process in
> the queue, seconds is a long time to wait. 

Even if there's another runnable process which _isn't_ blocked on the
chip?

-- 
dwmw2

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

On 8/4/07, David Woodhouse <dwmw2@infradead.org> wrote:
> On Fri, 2007-08-03 at 12:45 -0700, Jared Hulbert wrote:
> > We can't very well just wait seconds for the chip to become available.
> >  We could count waiting processes, but even it there is one process in
> > the queue, seconds is a long time to wait.
>
> Even if there's another runnable process which _isn't_ blocked on the
> chip?

Sure.  You can't just block a process that just wants a page full of
it's code, that has no dependencies to the file/page/block being
manipulated on chip, for several seconds.  You'd have all kinds of
potential badness.  I can picture someone trying to select an option
on a menu and waiting several seconds for a response.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Carsten Otte]

Jared Hulbert wrote:
> Hmmm, we'd definitely benefit from having a hook in idle to trigger
> the erase to resume.  Having hooks for entry into kernel space sounds
> interesting, but if you leave it too quickly and suspend the erase you
> don't make forward progress.  However, I'm not sure how you'd
> determine whether a given process can continue without suspending the
> erase.

*shrug*. I'd rather want to go a path without hooks, like the one 
David's suggesting. Hooks are known to be highly flammable on lkml.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> > Maybe I wasn't so clear.  Assuming the MTD is going to do an erase you would:
> >
> > (a) zap all page affected by block erase
> > (b) identify all pte's mapped to the affected region
> > (c) modify these pte's so they now trigger a fault
> > (d) wait for completion of erase
> > (c) reestablish valid pte mappings
>
> No need to reestablish them -- they'll fault when they need to, which
> means less work next time you erase. And it'll be a minor page fault,
> which is quite cheap -- it only needs to set up the pte and return.

Correct you don't _need_ to.  I'm not sure how cheap that fault is,
this is definitely just an optimization.

> > If you get a fault it gets routed to the filesystem .fault or
> > something like that.  The .fault routine can:
> > (a) suspend the erase
> > (b) copy the page to RAM
> > (c) update the pte to point to RAM
> > (d) resume the erase
>
> This could lead to all your data pages ending up in RAM, which kind of
> defeats the object of XIP.

Right, the assumption is that the number of fault would be low (not
sure how good that assumption is) and that you would push out all
those pages out of RAM at the end of the erase.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Carsten Otte]

David Woodhouse wrote:
> Alternatively, just wait for the chip to become available. Or increment
> a count of 'waiters' and only interrupt the erase when there are a
> certain number of people waiting.
Sounds reasonable to me. Maybe we should even think about the block 
device plug approach: set up a timer when the first reader shows up, 
and suspend when either a certain amount of readers shows up or the 
timer has run down.

Re: [PATCH][MTD] mtdpart.c: allow other drivers to get physical address of partition

[Jared Hulbert]

> Neither am I.  My missing puzzle piece is how to zap all page mappings
> for the chip in question.  Maybe the fs/vm meeting at lce will clear
> things up.

I hope so.

> If I started working on this I'd probably want AXFS on one partition and
> JFFS2 on another.  Being able to write to JFFS2 while reading XIP files
> from AXFS without corruptions is the major milestone.  When that works
> we can think about the other 90% of having both write support and XIP in
> the same filesystem.

No problem.  point()'ing to the whole AXFS image lets you do that.
Just turn on XIP support in MTD.

> How serious is Intel about merging AXFS in mainline, btw?

Very.  We've just been super busy helping customers evaluating and I
rearchitected it.  Also I've got lots of these dependancies to work
out.  You think this get_partition_physaddr() was a hack, wait until
you see what I do to mm/memory.c :)

> Maybe another vm person or two.  The vm part is the critical missing
> piece and needs all the brainpower we can get.

Is this worth doing?