[PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

This patch is 90% uncontroversial.  The remaining 10% introduce a new
field to struct mtd_info.  Looks like we need something similar to
this patch, but probably not identical to it.

Comments?

Jörn

-- 
Audacity augments courage; hesitation, fear.
-- Publilius Syrus 

[MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

Apart from the text substitution, the STMicro ECC-NOR and Intel Sibley
flashes need special treatment.  This patch introduces a special
kernel-only flag for them that jffs2 uses to call special code.  It
should be noted that special code for these devices should not be
used.  But until jffs2 is clean in that respect, the flag serves as a
reminder that a cleanup is still required at some point.

This also changes the safety guard in rfd_ftl.c.  We can use the ftl
on all devices that can behave like NOR flash now.  This adds RAM and
disk devices, but removes the mentionend ECC-NOR and Sibley flashes.

Signed-off-by: Jörn Engel <joern@wohnheim.fh-wedel.de>
---

 drivers/mtd/chips/amd_flash.c       |    2 +-
 drivers/mtd/chips/cfi_cmdset_0001.c |    3 ++-
 drivers/mtd/chips/cfi_cmdset_0002.c |    2 +-
 drivers/mtd/chips/cfi_cmdset_0020.c |    3 ++-
 drivers/mtd/chips/jedec.c           |    2 +-
 drivers/mtd/chips/sharp.c           |    2 +-
 drivers/mtd/devices/lart.c          |    2 +-
 drivers/mtd/devices/m25p80.c        |    2 +-
 drivers/mtd/rfd_ftl.c               |    2 +-
 fs/jffs2/os-linux.h                 |    2 +-
 include/linux/mtd/mtd.h             |    9 +++++++++
 include/mtd/mtd-abi.h               |    1 -
 12 files changed, 21 insertions(+), 11 deletions(-)

--- mtd_type/include/linux/mtd/mtd.h~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/include/linux/mtd/mtd.h	2006-04-23 14:04:49.000000000 +0200
@@ -56,9 +56,18 @@ struct mtd_erase_region_info {
 	u_int32_t numblocks;		/* Number of blocks of erasesize in this region */
 };
 
+/**
+ * Kernel flags.  Each of the flags below is necessary for one in-kernel
+ * user to special-case some flash type.  It is obvious that those
+ * special cases need to go, along with the associated flags.
+ */
+/* special case for nor_wbuf flashes, used by fs/jffs2/ */
+#define MTD_KF_NOR_WBUF_FLASH	0x8000000
+
 struct mtd_info {
 	u_char type;
 	u_int32_t flags;
+	u_int32_t kernel_flags; /* TODO: every one of these needs to go */
 	u_int32_t size;	 // Total size of the MTD
 
 	/* "Major" erase size for the device. Naïve users may take this
--- mtd_type/include/mtd/mtd-abi.h~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/include/mtd/mtd-abi.h	2006-04-23 14:04:49.000000000 +0200
@@ -24,7 +24,6 @@ struct mtd_oob_buf {
 };
 
 #define MTD_ABSENT		0
-#define MTD_NORFLASH		3
 #define MTD_NANDFLASH		4
 #define MTD_DATAFLASH		6
 #define MTD_GENERIC_TYPE	7
--- mtd_type/drivers/mtd/chips/amd_flash.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/chips/amd_flash.c	2006-04-23 14:04:49.000000000 +0200
@@ -736,7 +736,7 @@ static struct mtd_info *amd_flash_probe(
 		}
 		offset += dev_size;
 	}
-	mtd->type = MTD_NORFLASH;
+	mtd->type = MTD_GENERIC_TYPE;
 	mtd->flags = MTD_CAP_NORFLASH;
 	mtd->name = map->name;
 	mtd->erase = amd_flash_erase;
--- mtd_type/drivers/mtd/chips/cfi_cmdset_0001.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/chips/cfi_cmdset_0001.c	2006-04-23 14:04:49.000000000 +0200
@@ -351,7 +351,7 @@ struct mtd_info *cfi_cmdset_0001(struct 
 	}
 	memset(mtd, 0, sizeof(*mtd));
 	mtd->priv = map;
-	mtd->type = MTD_NORFLASH;
+	mtd->type = MTD_GENERIC_TYPE;
 
 	/* Fill in the default mtd operations */
 	mtd->erase   = cfi_intelext_erase_varsize;
@@ -551,6 +551,7 @@ static int cfi_intelext_partition_fixup(
 			MTD_PROGREGION_CTRLMODE_VALID(mtd) = cfi->interleave * prinfo->ControlValid;
 			MTD_PROGREGION_CTRLMODE_INVALID(mtd) = cfi->interleave * prinfo->ControlInvalid;
 			mtd->flags &= ~MTD_BIT_WRITEABLE;
+			mtd->kernel_flags |= MTD_KF_NOR_WBUF_FLASH;
 			printk(KERN_DEBUG "%s: program region size/ctrl_valid/ctrl_inval = %d/%d/%d\n",
 			       map->name, mtd->writesize,
 			       MTD_PROGREGION_CTRLMODE_VALID(mtd),
--- mtd_type/drivers/mtd/chips/cfi_cmdset_0002.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/chips/cfi_cmdset_0002.c	2006-04-23 14:04:49.000000000 +0200
@@ -225,7 +225,7 @@ struct mtd_info *cfi_cmdset_0002(struct 
 	}
 	memset(mtd, 0, sizeof(*mtd));
 	mtd->priv = map;
-	mtd->type = MTD_NORFLASH;
+	mtd->type = MTD_GENERIC_TYPE;
 
 	/* Fill in the default mtd operations */
 	mtd->erase   = cfi_amdstd_erase_varsize;
--- mtd_type/drivers/mtd/chips/cfi_cmdset_0020.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/chips/cfi_cmdset_0020.c	2006-04-23 14:04:49.000000000 +0200
@@ -182,7 +182,7 @@ static struct mtd_info *cfi_staa_setup(s
 
 	memset(mtd, 0, sizeof(*mtd));
 	mtd->priv = map;
-	mtd->type = MTD_NORFLASH;
+	mtd->type = MTD_GENERIC_TYPE;
 	mtd->size = devsize * cfi->numchips;
 
 	mtd->numeraseregions = cfi->cfiq->NumEraseRegions * cfi->numchips;
@@ -238,6 +238,7 @@ static struct mtd_info *cfi_staa_setup(s
 	mtd->suspend = cfi_staa_suspend;
 	mtd->resume = cfi_staa_resume;
 	mtd->flags = MTD_CAP_NORFLASH & ~MTD_BIT_WRITEABLE;
+	mtd->kernel_flags |= MTD_KF_NOR_WBUF_FLASH;
 	mtd->writesize = 8; /* FIXME: Should be 0 for STMicro flashes w/out ECC */
 	map->fldrv = &cfi_staa_chipdrv;
 	__module_get(THIS_MODULE);
--- mtd_type/drivers/mtd/chips/jedec.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/chips/jedec.c	2006-04-23 14:04:49.000000000 +0200
@@ -254,7 +254,7 @@ static struct mtd_info *jedec_probe(stru
    memset(MTD,0,sizeof(*MTD));
   // strlcpy(MTD->name,Part,sizeof(MTD->name));
    MTD->name = map->name;
-   MTD->type = MTD_NORFLASH;
+   MTD->type = MTD_GENERIC_TYPE;
    MTD->flags = MTD_CAP_NORFLASH;
    MTD->erasesize = SectorSize*(map->buswidth);
    //   printk("MTD->erasesize is %x\n",(unsigned int)MTD->erasesize);
--- mtd_type/drivers/mtd/chips/sharp.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/chips/sharp.c	2006-04-23 14:04:49.000000000 +0200
@@ -131,7 +131,7 @@ static struct mtd_info *sharp_probe(stru
 	}
 
 	mtd->priv = map;
-	mtd->type = MTD_NORFLASH;
+	mtd->type = MTD_GENERIC_TYPE;
 	mtd->erase = sharp_erase;
 	mtd->read = sharp_read;
 	mtd->write = sharp_write;
--- mtd_type/drivers/mtd/devices/lart.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/devices/lart.c	2006-04-23 14:04:49.000000000 +0200
@@ -634,7 +634,7 @@ int __init lart_flash_init (void)
 	 }
    printk ("%s: This looks like a LART board to me.\n",module_name);
    mtd.name = module_name;
-   mtd.type = MTD_NORFLASH;
+   mtd.type = MTD_GENERIC_TYPE;
    mtd.flags = MTD_CAP_NORFLASH;
    mtd.size = FLASH_BLOCKSIZE_PARAM * FLASH_NUMBLOCKS_16m_PARAM + FLASH_BLOCKSIZE_MAIN * FLASH_NUMBLOCKS_16m_MAIN;
    mtd.erasesize = FLASH_BLOCKSIZE_MAIN;
--- mtd_type/drivers/mtd/devices/m25p80.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/devices/m25p80.c	2006-04-23 14:04:49.000000000 +0200
@@ -464,7 +464,7 @@ static int __devinit m25p_probe(struct s
 	else
 		flash->mtd.name = spi->dev.bus_id;
 
-	flash->mtd.type = MTD_NORFLASH;
+	flash->mtd.type = MTD_GENERIC_TYPE;
 	flash->mtd.flags = MTD_CAP_NORFLASH;
 	flash->mtd.size = info->sector_size * info->n_sectors;
 	flash->mtd.erasesize = info->sector_size;
--- mtd_type/drivers/mtd/rfd_ftl.c~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/drivers/mtd/rfd_ftl.c	2006-04-23 14:04:49.000000000 +0200
@@ -761,7 +761,7 @@ static void rfd_ftl_add_mtd(struct mtd_b
 {
 	struct partition *part;
 
-	if (mtd->type != MTD_NORFLASH)
+	if ( !(mtd->flags & MTD_WRITEABLE) || !(mtd->flags & MTD_BIT_WRITEABLE))
 		return;
 
 	part = kcalloc(1, sizeof(struct partition), GFP_KERNEL);
--- mtd_type/fs/jffs2/os-linux.h~type_nor	2006-04-23 14:04:41.000000000 +0200
+++ mtd_type/fs/jffs2/os-linux.h	2006-04-23 14:04:49.000000000 +0200
@@ -131,7 +131,7 @@ void jffs2_nand_flash_cleanup(struct jff
 int jffs2_dataflash_setup(struct jffs2_sb_info *c);
 void jffs2_dataflash_cleanup(struct jffs2_sb_info *c);
 
-#define jffs2_nor_wbuf_flash(c) (c->mtd->type == MTD_NORFLASH && ! (c->mtd->flags & MTD_BIT_WRITEABLE))
+#define jffs2_nor_wbuf_flash(c) (c->mtd->flags & MTD_KF_NOR_WBUF_FLASH)
 int jffs2_nor_wbuf_flash_setup(struct jffs2_sb_info *c);
 void jffs2_nor_wbuf_flash_cleanup(struct jffs2_sb_info *c);
 

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

Jörn Engel wrote:
> This patch is 90% uncontroversial.  The remaining 10% introduce a new
> field to struct mtd_info.  Looks like we need something similar to
> this patch, but probably not identical to it.

Please, explain what does this MTD_GENERIC_TYPE mean? MTD_NORFLASH means 
that this is NOR flash. What's MTD_GENERIC_TYPE?

And why it "looks like we need something similar to this patch"?

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[David Woodhouse]

On Tue, 2006-05-30 at 17:24 +0200, Jörn Engel wrote:
> Apart from the text substitution, the STMicro ECC-NOR and Intel Sibley
> flashes need special treatment.  This patch introduces a special
> kernel-only flag for them that jffs2 uses to call special code.  It
> should be noted that special code for these devices should not be
> used.  But until jffs2 is clean in that respect, the flag serves as a
> reminder that a cleanup is still required at some point.

Explain. Why shouldn't the existing code be used? What should we do
instead?

∃ only a certain number of combinations of features in _real_ hardware
-- fairly much covered by 'ROM, RAM, NOR, NAND and Sibley/ECC-NOR'.
Enumerating those, and coping specifically with those, makes as much
sense as handling arbitrary combinations of feature flags.

-- 
dwmw2

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 16:40:20 +0100, David Woodhouse wrote:
> On Tue, 2006-05-30 at 17:24 +0200, Jörn Engel wrote:
> > Apart from the text substitution, the STMicro ECC-NOR and Intel Sibley
> > flashes need special treatment.  This patch introduces a special
> > kernel-only flag for them that jffs2 uses to call special code.  It
> > should be noted that special code for these devices should not be
> > used.  But until jffs2 is clean in that respect, the flag serves as a
> > reminder that a cleanup is still required at some point.
> 
> Explain. Why shouldn't the existing code be used? What should we do
> instead?
> 
> ??? only a certain number of combinations of features in _real_ hardware
> -- fairly much covered by 'ROM, RAM, NOR, NAND and Sibley/ECC-NOR'.
> Enumerating those, and coping specifically with those, makes as much
> sense as handling arbitrary combinations of feature flags.

Until recently, we had four different variants of jffs2 wbuf code,
depending on the flash type.  I fail to see the difference between
those four and have already merged two of them.  Unless I am missing
something important, the other three can also be combined into one.

But doing so would result in something like
	if ((mtd->type == MTD_NAND) || (mtd->type == MTD_SIBLEY) ||
			(mtd->type == MTD_ECCNOR) ||
			(mtd->type == MTD_DATAFLASH))

Or, more realistically, in something like the recent situation, where
each one of those cases had its own copy of special code because
people don't realize that their type is no different from another
long-merged type.  Note that Nico didn't notice and copied the ECC_NOR
code almost verbatim.  Considering that he is a solid developer
otherwise, what would an average or below-average hacker come up with?

Jörn

-- 
Victory in war is not repetitious.
-- Sun Tzu

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[David Woodhouse]

On Tue, 2006-05-30 at 17:50 +0200, Jörn Engel wrote:
> Until recently, we had four different variants of jffs2 wbuf code,
> depending on the flash type.  I fail to see the difference between
> those four and have already merged two of them.  Unless I am missing
> something important, the other three can also be combined into one.
> 
> But doing so would result in something like
>         if ((mtd->type == MTD_NAND) || (mtd->type == MTD_SIBLEY) ||
>                         (mtd->type == MTD_ECCNOR) ||
>                         (mtd->type == MTD_DATAFLASH))

That's why we _also_ have the flags, and why we abstract that still
further in application-specific code, like jffs2_can_mark_obsolete()

But you seem to want to kill _both_ the type _and_ the flag -- you say
that you want the MTD_KF_NOR_WBUF_FLASH flag to go away. So explain --
what do you want to do instead?

-- 
dwmw2

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 16:55:02 +0100, David Woodhouse wrote:
> On Tue, 2006-05-30 at 17:50 +0200, Jörn Engel wrote:
> > Until recently, we had four different variants of jffs2 wbuf code,
> > depending on the flash type.  I fail to see the difference between
> > those four and have already merged two of them.  Unless I am missing
> > something important, the other three can also be combined into one.
> > 
> > But doing so would result in something like
> >         if ((mtd->type == MTD_NAND) || (mtd->type == MTD_SIBLEY) ||
> >                         (mtd->type == MTD_ECCNOR) ||
> >                         (mtd->type == MTD_DATAFLASH))
> 
> That's why we _also_ have the flags, and why we abstract that still
> further in application-specific code, like jffs2_can_mark_obsolete()
> 
> But you seem to want to kill _both_ the type _and_ the flag -- you say
> that you want the MTD_KF_NOR_WBUF_FLASH flag to go away. So explain --
> what do you want to do instead?

	if (mtd->writesize > 1)

If the device does not allow writes to happen in arbitrary hunks, we
need wbuf.  That's all.

The currently three different variants of initializing wbuf need to be
converged into one generic method.  That won't happen just now, so I
introduced a temporary flag for one variant of initialization code.
The real gain comes with merging the three remaining variants,
obviously.

Jörn

-- 
It's just what we asked for, but not what we want!
-- anonymous

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

On Tue, 2006-05-30 at 17:50 +0200, Jörn Engel wrote:
> Until recently, we had four different variants of jffs2 wbuf code,
> depending on the flash type.  I fail to see the difference between
> those four and have already merged two of them.  Unless I am missing
> something important, the other three can also be combined into one.
> 
> But doing so would result in something like
> 	if ((mtd->type == MTD_NAND) || (mtd->type == MTD_SIBLEY) ||
> 			(mtd->type == MTD_ECCNOR) ||
> 			(mtd->type == MTD_DATAFLASH))
> 
> Or, more realistically, in something like the recent situation, where
> each one of those cases had its own copy of special code because
> people don't realize that their type is no different from another
> long-merged type.  Note that Nico didn't notice and copied the ECC_NOR
> code almost verbatim.  Considering that he is a solid developer
> otherwise, what would an average or below-average hacker come up with?

I'm absolutely against this trend. You'll soon make all flashes of
MTD_GENERIC_TYPE, then you'll remove mtd->type altogether.

We debated about this a lot. And you still didn't explain why I have no
right to know the  flash type.

Please, introduce your capabilities stuff, but please, stop removing
mtd->type implicitly or explicitly. 

AFAICS, what you're doing now is you're introducing reasonable
"capabilities" stuff, and slowly removing the mtd->type stuff. Please,
do one thing.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 20:01:33 +0400, Artem B. Bityutskiy wrote:
> 
> I'm absolutely against this trend. You'll soon make all flashes of
> MTD_GENERIC_TYPE, then you'll remove mtd->type altogether.
> 
> We debated about this a lot. And you still didn't explain why I have no
> right to know the  flash type.
> 
> Please, introduce your capabilities stuff, but please, stop removing
> mtd->type implicitly or explicitly. 
> 
> AFAICS, what you're doing now is you're introducing reasonable
> "capabilities" stuff, and slowly removing the mtd->type stuff. Please,
> do one thing.

Artem, for some reason I can't seem to either understand you or change
your mind.  You firmly want to keep mtd->type for a reason I cannot
understand.  And I firmly believe that mtd->type is useless at best
and outright harmful at worst.  Most likely one of us is wrong, but
since we can't reach common grounds, I stopped arguing with you.  It
is pointless, sadly.

If it is me, who is wrong, I hope dwmw2 will show just that.  If my
plans are sound, I hope he will agree and continue to merge patches.

Jörn

-- 
Man darf nicht das, was uns unwahrscheinlich und unnatürlich erscheint,
mit dem verwechseln, was absolut unmöglich ist.
-- Carl Friedrich Gauß

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1623 bytes --]

On Tue, 30 May 2006, Jörn Engel wrote:

> On Tue, 30 May 2006 20:01:33 +0400, Artem B. Bityutskiy wrote:
> > 
> > I'm absolutely against this trend. You'll soon make all flashes of
> > MTD_GENERIC_TYPE, then you'll remove mtd->type altogether.
> > 
> > We debated about this a lot. And you still didn't explain why I have no
> > right to know the  flash type.
> > 
> > Please, introduce your capabilities stuff, but please, stop removing
> > mtd->type implicitly or explicitly. 
> > 
> > AFAICS, what you're doing now is you're introducing reasonable
> > "capabilities" stuff, and slowly removing the mtd->type stuff. Please,
> > do one thing.
> 
> Artem, for some reason I can't seem to either understand you or change
> your mind.  You firmly want to keep mtd->type for a reason I cannot
> understand.  And I firmly believe that mtd->type is useless at best
> and outright harmful at worst.  Most likely one of us is wrong, but
> since we can't reach common grounds, I stopped arguing with you.  It
> is pointless, sadly.
> 
> If it is me, who is wrong, I hope dwmw2 will show just that.  If my
> plans are sound, I hope he will agree and continue to merge patches.

I'm firmly with you and so far I agree 100% with your effort.  The 
cleanups you're providing are much appreciated.

However Artem might have a point.  You probably should leave mtd->type 
alone, at least for now.  It is good to not depend on it anymore but it 
probably still has its informative value, just like name string attached 
to MTD partitions.  If it should go then it should be removed at the 
very end when no one uses it anymore.


Nicolas

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

On Tue, 2006-05-30 at 12:32 -0400, Nicolas Pitre wrote:
> I'm firmly with you and so far I agree 100% with your effort.  The 
> cleanups you're providing are much appreciated.
> 
> However Artem might have a point.  You probably should leave mtd->type 
> alone, at least for now.  It is good to not depend on it anymore but it 
> probably still has its informative value, just like name string attached 
> to MTD partitions.  If it should go then it should be removed at the 
> very end when no one uses it anymore.
> 
Fair enough.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 12:32:58 -0400, Nicolas Pitre wrote:
> 
> I'm firmly with you and so far I agree 100% with your effort.  The 
> cleanups you're providing are much appreciated.
> 
> However Artem might have a point.  You probably should leave mtd->type 
> alone, at least for now.  It is good to not depend on it anymore but it 
> probably still has its informative value, just like name string attached 
> to MTD partitions.  If it should go then it should be removed at the 
> very end when no one uses it anymore.

In a way, that is what I do.  mtd->type still exists, although only
for those types that are still used.  MTD_RAM and MTD_ROM have been
converted to MTD_GENERIC_TYPE.  In part, that helps me to easily see
how far I've come with a simple grep.  And in part it prevents others
from reintroducing new dependencies.  My plan is to remove mtd->type
completely when every device is of type MTD_GENERIC_TYPE.

But if there is a general consensus, I can leave that field alone for
now and hope that people don't introduce new dependencies.

Jörn

-- 
It's just what we asked for, but not what we want!
-- anonymous

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1363 bytes --]

On Tue, 30 May 2006, Jörn Engel wrote:

> On Tue, 30 May 2006 12:32:58 -0400, Nicolas Pitre wrote:
> > 
> > I'm firmly with you and so far I agree 100% with your effort.  The 
> > cleanups you're providing are much appreciated.
> > 
> > However Artem might have a point.  You probably should leave mtd->type 
> > alone, at least for now.  It is good to not depend on it anymore but it 
> > probably still has its informative value, just like name string attached 
> > to MTD partitions.  If it should go then it should be removed at the 
> > very end when no one uses it anymore.
> 
> In a way, that is what I do.  mtd->type still exists, although only
> for those types that are still used.  MTD_RAM and MTD_ROM have been
> converted to MTD_GENERIC_TYPE.  In part, that helps me to easily see
> how far I've come with a simple grep.  And in part it prevents others
> from reintroducing new dependencies.  My plan is to remove mtd->type
> completely when every device is of type MTD_GENERIC_TYPE.
> 
> But if there is a general consensus, I can leave that field alone for
> now and hope that people don't introduce new dependencies.

Like you discovered yourself, people tend to duplicate existing code.  
So once enough of the current code is moved away from using mtd->type 
then the probability for new usage added to the tree will go down as 
well.


Nicolas

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 14:13:49 -0400, Nicolas Pitre wrote:
> 
> Like you discovered yourself, people tend to duplicate existing code.  
> So once enough of the current code is moved away from using mtd->type 
> then the probability for new usage added to the tree will go down as 
> well.

Good point. :)

Jörn

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

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

On Tue, 2006-05-30 at 18:10 +0200, Jörn Engel wrote:
> Artem, for some reason I can't seem to either understand you or change
> your mind.  You firmly want to keep mtd->type for a reason I cannot
> understand.  And I firmly believe that mtd->type is useless at best
> and outright harmful at worst.  Most likely one of us is wrong, but
> since we can't reach common grounds, I stopped arguing with you.  It
> is pointless, sadly.

As you're removing an already existing field, *you* should prove why it
is useless, not me. I provide you a silly use-case: i want to print a
type of flash my JFFS2 file system is working on - why mustn't I?

Again, I'm not against your claen-up, OK? But do one thing at a time
please, not 2 at once. Introduce capabilities, but don't touch mtd->type
please, why isn't this possible?

At last, if you hate mtd->type, remove it later by one patch (I hope
this won't happen :-) ).

Do you see my point?

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[David Woodhouse]

On Tue, 2006-05-30 at 18:10 +0200, Jörn Engel wrote:
> If it is me, who is wrong, I hope dwmw2 will show just that.  If my
> plans are sound, I hope he will agree and continue to merge patches. 

Your plans don't seem to exist yet... I'm awaiting an answer to the
question of what you want to replace the MTD_KF_NOR_WBUF_FLASH flag
with.

-- 
dwmw2

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 17:57:13 +0100, David Woodhouse wrote:
> On Tue, 2006-05-30 at 18:10 +0200, Jörn Engel wrote:
> > If it is me, who is wrong, I hope dwmw2 will show just that.  If my
> > plans are sound, I hope he will agree and continue to merge patches. 
> 
> Your plans don't seem to exist yet... I'm awaiting an answer to the
> question of what you want to replace the MTD_KF_NOR_WBUF_FLASH flag
> with.

http://lists.infradead.org/pipermail/linux-mtd/2006-May/015750.html

Jörn

-- 
Unless something dramatically changes, by 2015 we'll be largely
wondering what all the fuss surrounding Linux was really about.
-- Rob Enderle

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 581 bytes --]

On Tue, 30 May 2006, Jörn Engel wrote:

> Or, more realistically, in something like the recent situation, where
> each one of those cases had its own copy of special code because
> people don't realize that their type is no different from another
> long-merged type.  Note that Nico didn't notice and copied the ECC_NOR
> code almost verbatim.  Considering that he is a solid developer
> otherwise, what would an average or below-average hacker come up with?

Well, given I'm not really intimate with the JFFS2 code I'm most 
probably in the below-average category here.


Nicolas

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 17:24:06 +0200, Jörn Engel wrote:
> 
> This patch is 90% uncontroversial.  The remaining 10% introduce a new
> field to struct mtd_info.  Looks like we need something similar to
> this patch, but probably not identical to it.
> 
> Comments?

/me just noticed how stupid the introduction of the kernel flags was.
Here is another migration path, that will likely receive Artems
objections as well.

Invariant:
In the end, initialization of the wbuf code will be identical for all
devices that require wbuf.  Whether a device requires wbuf is simply
checked by looking at mtd->size.  If mtd->size is greater than 1, we
need wbuf.

Step 1:
Turn MTD_NOR into MTD_GENERIC_TYPE and call the current initialization
for NOR the generic one.

Step 2:
Turn MTD_DATAFLASH into MTD_GENERIC_TYPE, remove its special cased
wbuf initialization and make sure the generic one is usable for
DATAFLASH as well.

Step 3:
Turn MTD_NAND into MTD_GENERIC_TYPE, remove its special cased wbuf
initialization and make sure the generic one is usable for NAND as
well.

Since step 3 isn't that simple, it need to be broken down further.

Step 3a:
Poke tglx to add his double-page-write code.  This should allow
mtd->writesize to go down for NAND flashes.

Step 3b:
Remove the OOB erasemarkers and move them in-band, just as for the
other types.

Step 3c:
See above


Again, Artem won't like this.  But this time I would like to see
better objections than before.  Just to show how silly they are:

	"*you* should prove why it is useless, not me"
That is what these patches are doing.  I remove every existing user of
mtd->type and thereby prove that mtd->type was useless in this
particular case.  When I'm finished and have dealt with every case,
the proof is complete.

	"i want to print a type of flash my JFFS2 file system is
	working on"
Show me the code.  If you have out-of-tree patches that may collide
with my work - tough luck.  I have out-of-tree patches as well and as
long as they are not merged, it is _my_ duty to deal with any breakage
created by other's changes in the kernel.  That has always been the
general rule and for a good reason.

Jörn

-- 
The strong give up and move away, while the weak give up and stay.
-- unknown

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[David Woodhouse]

On Tue, 2006-05-30 at 20:50 +0200, Jörn Engel wrote:
> Step 3b:
> Remove the OOB erasemarkers and move them in-band, just as for the
> other types.

And waste a whole page for the cleanmarkers instead of putting them in
the OOB area? No.

-- 
dwmw2

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 19:52:15 +0100, David Woodhouse wrote:
> On Tue, 2006-05-30 at 20:50 +0200, Jörn Engel wrote:
> > Step 3b:
> > Remove the OOB erasemarkers and move them in-band, just as for the
> > other types.
> 
> And waste a whole page for the cleanmarkers instead of putting them in
> the OOB area? No.

Did you notice 3a?  Double-page-write allows you to write to pages
more than once.

Plus, there is the long-term solution as well.  Jffs2 can write the
information anywhere, not just into the same physical eraseblock.  For
logfs, I will store that information in a special file.  JFFS2 doesn't
even need a special file but can introduce a new node type.  Basically
an erase marker for a _different_ erase block, with some versioning to
make sure it doesn't get deleted (through GC) before the erased block
is used.

Jörn

-- 
The story so far:
In the beginning the Universe was created.  This has made a lot
of people very angry and been widely regarded as a bad move.
-- Douglas Adams

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1012 bytes --]

On Tue, 30 May 2006, Jörn Engel wrote:

> Here is another migration path, that will likely receive Artems
> objections as well.
> 
> Invariant:
> In the end, initialization of the wbuf code will be identical for all
> devices that require wbuf.  Whether a device requires wbuf is simply
> checked by looking at mtd->size.  If mtd->size is greater than 1, we
> need wbuf.
> 
> Step 1:
> Turn MTD_NOR into MTD_GENERIC_TYPE and call the current initialization
> for NOR the generic one.
> 
> Step 2:
> Turn MTD_DATAFLASH into MTD_GENERIC_TYPE, remove its special cased
> wbuf initialization and make sure the generic one is usable for
> DATAFLASH as well.
> 
> Step 3:
> Turn MTD_NAND into MTD_GENERIC_TYPE, remove its special cased wbuf
> initialization and make sure the generic one is usable for NAND as
> well.

If presented that way you're indeed up for more protests.

Just don't "turn MTD_* into MTD_GENERIC_TYPE".  Instead, try to "move 
MTD_* away from using mtd->type".  Or something like that.


Nicolas

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Tue, 30 May 2006 15:10:08 -0400, Nicolas Pitre wrote:
> 
> If presented that way you're indeed up for more protests.
> 
> Just don't "turn MTD_* into MTD_GENERIC_TYPE".  Instead, try to "move 
> MTD_* away from using mtd->type".  Or something like that.

*sigh*

Ok, I'll get back to drinking coffee and playing minesweeper then.
Sorry for the disturbance.

Jörn

-- 
Unless something dramatically changes, by 2015 we'll be largely
wondering what all the fuss surrounding Linux was really about.
-- Rob Enderle

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

On Tue, 2006-05-30 at 20:50 +0200, Jörn Engel wrote:
> Again, Artem won't like this.  But this time I would like to see
> better objections than before.  Just to show how silly they are:
> 
> 	"*you* should prove why it is useless, not me"
> That is what these patches are doing.  I remove every existing user of
> mtd->type and thereby prove that mtd->type was useless in this
> particular case.  When I'm finished and have dealt with every case,
> the proof is complete.
> 
> 	"i want to print a type of flash my JFFS2 file system is
> 	working on"
> Show me the code.  If you have out-of-tree patches that may collide
> with my work - tough luck.  I have out-of-tree patches as well and as
> long as they are not merged, it is _my_ duty to deal with any breakage
> created by other's changes in the kernel.  That has always been the
> general rule and for a good reason.

Please, stop this, there is no reason for greening. You was several
times said about my real reasons. This was just an answer for your "I
don't want to delve into your explanation, I just let David judge".

Once more.

1. Having an mtd->field field and capabilities flags a bad idea. Why?

1a: Who initializes this mtd->flags field? If I want to add 20 new
capabilities, how many places do I have to change? As many as there are
drivers in MTD? Insane.

2a. In general, the capabilities thing is application-dependent. And it
must stay in application's area, not deep in drivers.

3a: If I'm a user of MTD, and I want to add 20 new capability, do I have
to change MTD itself? It's not my business to do this.

4a. How wide is the mtd->flags field? 32 bits? So, you restrict me by 32
capabilities? And if I need more, I have to combine the existing ones?
Why is it better then using mtd->type?

So, this is not an extensible solution.

2. OK, what do you offer instead (the question nobody asked yet)?

Forget about mtd->flags. Leave alone mtd->type. Add an
include/linux/mtd/flash_capabilities.h header which looks like this:

/* If the flash allows to clear individual bits */
#define mtd_can_clear_bits(mtd) (mtd->type == MTD_NORFLASH)
/* If the goat may be milked */
#define mtd_can_milk_a_goat(mtd) (bah)
....
etc.

So, you have your set of capabilities. You use this header in JFFS2 and
clean it up. Your task is solved.

If I want to add a new flash type, I fix the flash_capabilities.h
header. If I add a new capability - I fix the header as well. I don't
get my dirty forepaws into other drivers. It's extensible.

To put it differently, I offer to add the capabilities stuff as a thin
layer over MTD, not to jab it deeply into the MTD core.

3. It's difficult to understand what is MTD_GENERIC_TYPE stuff. In
reality, this is just a temporary junk, which is supposed to go later.
But temporary junk usually tends to stay forever. I can't understand why
you cannot avoid introducing it. 

4. OK, if you still think that mtd->flags is better, please, go ahead.
After all, it may be fixed later if needed. And may be I'm just wrong.
But, please, do it without this strange MTD_GENERIC_TYPE stuff. Why
can't you?

5. Introducing capabilities and removing mtd->type are 2 completely
different activities. No need to mix them, IMO.

I appreciate your work, and offer another way to do this. I think it is
better, and didn't so far hear any good argument against it. Thanks for
reading this :-)

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Nicolas Pitre]

On Wed, 31 May 2006, Artem B. Bityutskiy wrote:

> 1. Having an mtd->field field and capabilities flags a bad idea. Why?
> 
> 1a: Who initializes this mtd->flags field?

The chip driver providing anMTD interface to said chip.

> If I want to add 20 new
> capabilities, how many places do I have to change? As many as there are
> drivers in MTD? Insane.

Don't be ridiculous.  There is no way you can find 20 capabilities total 
for a generic MTD interface.

> 2a. In general, the capabilities thing is application-dependent. And it
> must stay in application's area, not deep in drivers.

Not true.  The capabilities are function of the hardware not the 
application.

> 3a: If I'm a user of MTD, and I want to add 20 new capability, do I have
> to change MTD itself? It's not my business to do this.

If you want to add new capabilities, you only have to care about users 
of those capabilities and of course users that worked without them 
before will just continue to work if they ignore them.

> 4a. How wide is the mtd->flags field? 32 bits? So, you restrict me by 32
> capabilities? And if I need more, I have to combine the existing ones?
> Why is it better then using mtd->type?

Who said capabilities have to be only single bits?

And even if the capability field as you insist to see it has only 32 
bits, what prevents you from extending it in the unlikely event more 
bits might be needed?

> So, this is not an extensible solution.

Artem you're more brilliant than that.

> 2. OK, what do you offer instead (the question nobody asked yet)?
> 
> Forget about mtd->flags. Leave alone mtd->type. Add an
> include/linux/mtd/flash_capabilities.h header which looks like this:
> 
> /* If the flash allows to clear individual bits */
> #define mtd_can_clear_bits(mtd) (mtd->type == MTD_NORFLASH)
> /* If the goat may be milked */
> #define mtd_can_milk_a_goat(mtd) (bah)
> ....
> etc.
> 
> So, you have your set of capabilities. You use this header in JFFS2 and
> clean it up. Your task is solved.

And the runtime code is both larger and slower.  And the source is 
even more obfuscated.  Isn't that great?


Nicolas

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

On Wed, 2006-05-31 at 10:01 -0400, Nicolas Pitre wrote:
> Don't be ridiculous.  There is no way you can find 20 capabilities total 
> for a generic MTD interface.

I'm not sure it can't be the case.

> > 2a. In general, the capabilities thing is application-dependent. And it
> > must stay in application's area, not deep in drivers.
> 
> Not true.  The capabilities are function of the hardware not the 
> application.
OK, I agree with you. It is a function of the hardware. But, the
mtd->flags field anyway becomes application dependent, just because the
number of hardware features is very large, and you introduce only those
flags *applications* need.

Example. Let's consider the AG-AND flash. Some of them have a property
when you erase some eraseblocks, but the neighboring eraseblocks get
slowly corrupted. Is this a flash feature? - Yes. Do you have a flag for
it? - No.

So, the idea is that number of features is potentially high. And there
is a dependency on the application present in a way.

> > 3a: If I'm a user of MTD, and I want to add 20 new capability, do I have
> > to change MTD itself? It's not my business to do this.
> 
> If you want to add new capabilities, you only have to care about users 
> of those capabilities and of course users that worked without them 
> before will just continue to work if they ignore them.
I also have to check if other flashes have these capabilities, and add
it to them if yes. So I have to change drivers.

> > 4a. How wide is the mtd->flags field? 32 bits? So, you restrict me by 32
> > capabilities? And if I need more, I have to combine the existing ones?
> > Why is it better then using mtd->type?
> 
> Who said capabilities have to be only single bits?
> 
> And even if the capability field as you insist to see it has only 32 
> bits, what prevents you from extending it in the unlikely event more 
> bits might be needed?
Isn't it bad to have mtd->flags1, mtd->flags2, etc?

> > So, this is not an extensible solution.
> Artem you're more brilliant than that.
Frankly, I can't get the meaning of this :-(

> > 2. OK, what do you offer instead (the question nobody asked yet)?
> > 
> > Forget about mtd->flags. Leave alone mtd->type. Add an
> > include/linux/mtd/flash_capabilities.h header which looks like this:
> > 
> > /* If the flash allows to clear individual bits */
> > #define mtd_can_clear_bits(mtd) (mtd->type == MTD_NORFLASH)
> > /* If the goat may be milked */
> > #define mtd_can_milk_a_goat(mtd) (bah)
> > ....
> > etc.
> > 
> > So, you have your set of capabilities. You use this header in JFFS2 and
> > clean it up. Your task is solved.
> 
> And the runtime code is both larger and slower.  And the source is 
> even more obfuscated.  Isn't that great?
> 
As to "And the source is even more obfuscated" - I do not understand
this. Why? The same if (can_milk_a_goat()) in both cases. Or do you mean
after gcc -E ? Then yes.

I was telling about a general approach, you're telling about technical
details. Well, if you care about code size and speed, then create
mtd_capabilities.c, call add an init_capabilities() call which is called
on initialization, and have variables for each feature.

#define can_milk_a_goat(mtd) (mtd->priv->can_mlk_a_goat)

The Idea still stays unchanged - it's a thin layer over mtd->type.
Implement it differently.

I still don't see any drawback of this approach. The advantage is that
it'll all be put in one place, and that'll be not the driver who
initialize it.

Finally, I don't really care now much about how capabilities are
implemented, and I wrote this. Implement it your way - fine, it is not
fundamental, I'm just suggesting another approach, may I? I care more
about the MTD_GENERIC_TYPE stuff, which is senseless in my opinion. I
wrote about this, and don't see any reaction.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Nicolas Pitre]

On Wed, 31 May 2006, Artem B. Bityutskiy wrote:

> On Wed, 2006-05-31 at 10:01 -0400, Nicolas Pitre wrote:
> > Don't be ridiculous.  There is no way you can find 20 capabilities total 
> > for a generic MTD interface.
> 
> I'm not sure it can't be the case.

Last time we enumerated less than 10 parameters that would cover more 
than 90% of all cases in a clean and obvious way.  One is 
mtd->block_write_size.  For most NOR flash this is 1.  For Sibley this 
is 1024.  For ECC NOR I think it is 16.  For NAND it is another value. 
Etc.  Why should that value be determined _each_ time at runtime based 
on mtd->type using your macro proposal while it can be directly obtained 
from mtd->block_write_size?

This also means that JFFS2 can use:

	if (mtd->block_write_size > 1)
		use_wbuf();

while your proposal would require the equivalent of:

	if (mtd->type == SIBLEY || mtd->type == NAND ||
	    mtd->type == DATAFLASH || mtd->type == ECC_NOR || ... )
		use_wbuf();

And yet wbuf() still doesn't know how big the buffer should be. Don't 
you see the difference?

And if a driver for another type of flash is added to the kernel then it 
only needs to initialise its own value for mtd->block_write_size and 
JFFS2 will just work with no change.

Whereas with your own view the fact that mtd->type is set to GOAT_FLASH 
then you need to modyfy _all_ users of the MTD interface so they learn 
to also consider mtd->type == GOAT_FLASH and to hardcode its buffer size 
which might well be yet another value like 8192.

Isn't that obvious enough that the mtd->type method is suboptimal and 
obfuscates the code?  What do you not get at this point?

> > > 2a. In general, the capabilities thing is application-dependent. And it
> > > must stay in application's area, not deep in drivers.
> > 
> > Not true.  The capabilities are function of the hardware not the 
> > application.
> OK, I agree with you. It is a function of the hardware. But, the
> mtd->flags field anyway becomes application dependent, just because the
> number of hardware features is very large, and you introduce only those
> flags *applications* need.

So?  What's the problem?

> Example. Let's consider the AG-AND flash. Some of them have a property
> when you erase some eraseblocks, but the neighboring eraseblocks get
> slowly corrupted. Is this a flash feature? - Yes. Do you have a flag for
> it? - No.

Why not?

> So, the idea is that number of features is potentially high. And there
> is a dependency on the application present in a way.

If you need to support completely fubar flash then of course the 
capability model may not cover them adequately and you might have to 
bastardize the application code based on

	if (mtd->type == FUBAR_FLASH)
		do_this_fubar_hack();

But this is maybe 10% of the cases.  The other 90% will fit in the 
generic model just fine.

> > > 3a: If I'm a user of MTD, and I want to add 20 new capability, do I have
> > > to change MTD itself? It's not my business to do this.
> > 
> > If you want to add new capabilities, you only have to care about users 
> > of those capabilities and of course users that worked without them 
> > before will just continue to work if they ignore them.
> I also have to check if other flashes have these capabilities, and add
> it to them if yes. So I have to change drivers.

You don't _have_ to.  Again, if the code worked fine without knowledge 
of a given capability before then the addition of support for a given 
capability won't break those who don't advertise that capability.

But if we consider your own view, the addition of a new MTD _user_ 
require that the author of that new module knows about all flash types 
currently provided by MTD and cope with each of their specific 
characteristics which should be deduced from mtd->type.  If instead it 
only needs to cope with mtd->block_write_size to use that same example 
again then things are much simpler for both user module authors and 
flash driver authors.

> > > 4a. How wide is the mtd->flags field? 32 bits? So, you restrict me by 32
> > > capabilities? And if I need more, I have to combine the existing ones?
> > > Why is it better then using mtd->type?
> > 
> > Who said capabilities have to be only single bits?
> > 
> > And even if the capability field as you insist to see it has only 32 
> > bits, what prevents you from extending it in the unlikely event more 
> > bits might be needed?
> Isn't it bad to have mtd->flags1, mtd->flags2, etc?

Why not?  In fact it can be made into a structure bit field that the C 
language already supports rather well.

	struct MTD_blah {
		...
		int can_do_x : 1;
		int can_DO_Y : 1;
		INT CAN_DO_Z : 1;
		...
	}

ONce set those bits are always read-only so no locking issues, etc.

> > > So, this is not an extensible solution.
> > Artem you're more brilliant than that.
> Frankly, I can't get the meaning of this :-(

I'm sorry for you then.

> > > 2. OK, what do you offer instead (the question nobody asked yet)?
> > > 
> > > Forget about mtd->flags. Leave alone mtd->type. Add an
> > > include/linux/mtd/flash_capabilities.h header which looks like this:
> > > 
> > > /* If the flash allows to clear individual bits */
> > > #define mtd_can_clear_bits(mtd) (mtd->type == MTD_NORFLASH)
> > > /* If the goat may be milked */
> > > #define mtd_can_milk_a_goat(mtd) (bah)
> > > ....
> > > etc.
> > > 
> > > So, you have your set of capabilities. You use this header in JFFS2 and
> > > clean it up. Your task is solved.
> > 
> > And the runtime code is both larger and slower.  And the source is 
> > even more obfuscated.  Isn't that great?
> > 
> As to "And the source is even more obfuscated" - I do not understand
> this. Why? The same if (can_milk_a_goat()) in both cases. Or do you mean
> after gcc -E ? Then yes.

You have to dig into another header file to know what can_milk_a_goat() 
actually means in the context of MTD, especially since, according to 
you, this capability is always derived from mtd->type.

> I was telling about a general approach, you're telling about technical
> details. Well, if you care about code size and speed, then create
> mtd_capabilities.c, call add an init_capabilities() call which is called
> on initialization, and have variables for each feature.

I care about a general approach that [surprise surprise] also allows for 
obvious code from a readability point of view and produce small and fast 
runtime code.

> #define can_milk_a_goat(mtd) (mtd->priv->can_mlk_a_goat)
> 
> The Idea still stays unchanged - it's a thin layer over mtd->type.
> Implement it differently.

I'm glad you say yourself that it is an extra (IMHO unneeded) layer over 
mtd->type.

> I still don't see any drawback of this approach.

It is an extra layer over mtd->type.  That's the drawback.

> The advantage is that
> it'll all be put in one place, and that'll be not the driver who
> initialize it.

This is nonsense.  It is like saying that all architectures Linux 
supports should be merged into one directory instead of arch/arm, 
arch/alpha, etc.  After all the code in those architecture 
subdirectories is used by the same kernel scheduler.

> Finally, I don't really care now much about how capabilities are
> implemented, and I wrote this. Implement it your way - fine, it is not
> fundamental, I'm just suggesting another approach, may I?

You do so without ever acknowledging the disadvantages your approach has 
over the other one which is disconcerting.

> I care more
> about the MTD_GENERIC_TYPE stuff, which is senseless in my opinion.

And I agreed with you on that point already.  Did you miss it?

> I wrote about this, and don't see any reaction.

Do you ignore everything I wrote not addressed to you directly?


Nicolas

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

Nicolas Pitre wrote:
>>>Don't be ridiculous.  There is no way you can find 20 capabilities total 
>>>for a generic MTD interface.
>>I'm not sure it can't be the case.
> 
> Last time we enumerated less than 10 parameters that would cover more 
> than 90% of all cases in a clean and obvious way.  One is 
> mtd->block_write_size.  For most NOR flash this is 1.  For Sibley this 
> is 1024.  For ECC NOR I think it is 16.  For NAND it is another value. 
> Etc.  Why should that value be determined _each_ time at runtime based 
> on mtd->type using your macro proposal while it can be directly obtained 
> from mtd->block_write_size?
Why are you talking about mtd->block_write_size? Its completely 
irrelevant and has nothing to do with flags. I've never said we don't 
need it. I've always said we *do* need it instead of mtd->oobblock stuff 
many times and since long time ago.

> This also means that JFFS2 can use:
> 
> 	if (mtd->block_write_size > 1)
> 		use_wbuf();
Sure.

> while your proposal would require the equivalent of:
> 
> 	if (mtd->type == SIBLEY || mtd->type == NAND ||
> 	    mtd->type == DATAFLASH || mtd->type == ECC_NOR || ... )
> 		use_wbuf();
That's not my proposal.

> And yet wbuf() still doesn't know how big the buffer should be. Don't 
> you see the difference?
I see the difference and have always seen it.

> And if a driver for another type of flash is added to the kernel then it 
> only needs to initialise its own value for mtd->block_write_size and 
> JFFS2 will just work with no change.
Of course, but how does this relate to mtd->flags?

> Whereas with your own view the fact that mtd->type is set to GOAT_FLASH 
> then you need to modyfy _all_ users of the MTD interface so they learn 
> to also consider mtd->type == GOAT_FLASH and to hardcode its buffer size 
> which might well be yet another value like 8192.
No. Writesize is another question. I talked about capabilities like:

* can individual bits be cleared or not?
* do we have to erase eraseblocks before writing to them (dataflash may 
erase before writing itself)
* does this MTD device guarantee erase operation atomicity or not? I.e., 
does JFFS2 have to maintain cleanmarkers or they are not needed.
* does this flash has the "radiation" feature like AG-AND does.
* does this flash may take advantage of parallel operations or not? 
E.g., there may be a striping layer below, or this may be a HW array of 
flash chips which may operate in parallel.

etc etc etc.

The number of features and capabilities like this may be very large.

> Isn't that obvious enough that the mtd->type method is suboptimal and 
> obfuscates the code?  What do you not get at this point?
And it's obvious, that you can't foresee all the features. It is obvious 
that new applications may want new features you can't even imagine now.

>>OK, I agree with you. It is a function of the hardware. But, the
>>mtd->flags field anyway becomes application dependent, just because the
>>number of hardware features is very large, and you introduce only those
>>flags *applications* need.
> 
> So?  What's the problem?
"Problem" is a too loud word here. I just pointed that a new feature may 
require changing many drivers, and this is a drawback of Joern's 
approach. Nothing else.

>>Example. Let's consider the AG-AND flash. Some of them have a property
>>when you erase some eraseblocks, but the neighboring eraseblocks get
>>slowly corrupted. Is this a flash feature? - Yes. Do you have a flag for
>>it? - No.
> 
> Why not?
That was just an example that we cannot foresee all the capabilities. 
And "no" is because I think it is bad to change *MTD core* when a *user* 
needs a new feature flag. Just bad.

>>So, the idea is that number of features is potentially high. And there
>>is a dependency on the application present in a way.
> 
> If you need to support completely fubar flash then of course the 
> capability model may not cover them adequately and you might have to 
> bastardize the application code based on
> 
> 	if (mtd->type == FUBAR_FLASH)
> 		do_this_fubar_hack();
> 
> But this is maybe 10% of the cases.  The other 90% will fit in the 
> generic model just fine.
Well. My idea is that in this case you open the mtd_capabilities.h file, 
and add a

#define mtd_is_my_new_feature(mtd) (blah)

Or if it is completely not of other's interest, you do like you pointed.

>>I also have to check if other flashes have these capabilities, and add
>>it to them if yes. So I have to change drivers.
> 
> You don't _have_ to.  Again, if the code worked fine without knowledge 
> of a given capability before then the addition of support for a given 
> capability won't break those who don't advertise that capability.
It won't break. But you should agree that if a add a new capability, I 
have to check all flashes, not just the one I'm interested in.

> But if we consider your own view, the addition of a new MTD _user_ 
> require that the author of that new module knows about all flash types 
> currently provided by MTD and cope with each of their specific 
> characteristics which should be deduced from mtd->type.  If instead it 
> only needs to cope with mtd->block_write_size to use that same example 
> again then things are much simpler for both user module authors and 
> flash driver authors.
Writesize is another. We have a "flash model" and it is exposed by 
mtd_info. Writesize is *the part of this model*, OK? There were many 
mails where we talked about this generic flash model".

All the peculiarities are hidden. So, we are basically talking about 
*how to still let users know about some peculiarities*. For example, for 
optimization.

Joern's approach is to gave mtd->flags stuff.
My approach is to base this all on mtd->type.

>>Isn't it bad to have mtd->flags1, mtd->flags2, etc?
> 
> Why not?  In fact it can be made into a structure bit field that the C 
> language already supports rather well.
> 
> 	struct MTD_blah {
> 		...
> 		int can_do_x : 1;
> 		int can_DO_Y : 1;
> 		INT CAN_DO_Z : 1;
> 		...
> 	}
Hmm, this is good idea, but I still think we must not put stuff like 
this to mtd_info. It is out of our "common flash model". The model is 
fixed and firm. "Fluent" stuff like this should be out of it, above it.

>>>>So, this is not an extensible solution.
>>>Artem you're more brilliant than that.
>>Frankly, I can't get the meaning of this :-(
> I'm sorry for you then.
Impolite.

>>I was telling about a general approach, you're telling about technical
>>details. Well, if you care about code size and speed, then create
>>mtd_capabilities.c, call add an init_capabilities() call which is called
>>on initialization, and have variables for each feature.
> 
> I care about a general approach that [surprise surprise] also allows for 
> obvious code from a readability point of view and produce small and fast 
> runtime code.
I don't see any difference in readability. In both cases you have

if (can_milk_a_goat())
	milk();

>>The Idea still stays unchanged - it's a thin layer over mtd->type.
>>Implement it differently.
> 
> I'm glad you say yourself that it is an extra (IMHO unneeded) layer over 
> mtd->type.
Although it is a layer, it makes no additional overhead. But it makes 
MTD completely independent on features people may think of.

> It is an extra layer over mtd->type.  That's the drawback.
It's a plus: better layering, no overhead, more independence to MTD core.

>>I care more
>>about the MTD_GENERIC_TYPE stuff, which is senseless in my opinion.
> 
> And I agreed with you on that point already.  Did you miss it?
No. I didn't. I just wanted to say that I wrote "why" previously, so I 
don't write it now.

>>I wrote about this, and don't see any reaction.
> 
> Do you ignore everything I wrote not addressed to you directly?
No, that was addressed more to Joern, I meant "I don't see any reaction 
from Joern", because I wrote about that to him.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Nicolas Pitre]

On Wed, 31 May 2006, Artem B. Bityutskiy wrote:

> Nicolas Pitre wrote:
> > > The Idea still stays unchanged - it's a thin layer over mtd->type.
> > > Implement it differently.
> > 
> > I'm glad you say yourself that it is an extra (IMHO unneeded) layer over
> > mtd->type.
> Although it is a layer, it makes no additional overhead. But it makes MTD
> completely independent on features people may think of.

This is the fundamental differences between our views.

I just can't agree with the "additional layer makes no additional 
overhead" assertion. Sorry.

> > It is an extra layer over mtd->type.  That's the drawback.
> It's a plus: better layering, no overhead, more independence to MTD core.

And while this sentence sounds like marketing, I makes no sense to me 
technically.

Anyway, I give up.  Let's agree to disagree and leave it at that.
It's up to David to decide who is right and who is wrong.


Nicolas

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

Nicolas Pitre wrote:
> This is the fundamental differences between our views.
> 
> I just can't agree with the "additional layer makes no additional 
> overhead" assertion. Sorry.

Nicolas,

I bet you understand what I meant. I meant, that you may implement this 
"layer" a way it'll have no overhead comparing to the way Joern offers. 
May be layer is not the very appropriate word here. It's not like 
network layers. I just wanted to put my idea in a bit different angle.

I'd uderstood irony, but when you fetch a phrase from the context and 
out of this context it really looks weird, I don't know what is this. 
This is not an argument.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Jörn Engel]

On Thu, 1 June 2006 13:03:10 +0400, Artem B. Bityutskiy wrote:
> 
> I bet you understand what I meant. I meant, that you may implement this 
> "layer" a way it'll have no overhead comparing to the way Joern offers. 
> May be layer is not the very appropriate word here. It's not like 
> network layers. I just wanted to put my idea in a bit different angle.

I don't understand how that layer would not introduce overhead.  But
maybe you have some explanation in diff -up format.  That would
certainly make things easier to understand.

Jörn

-- 
It's just what we asked for, but not what we want!
-- anonymous

Re: [PATCH,RFC] [MTD] replace MTD_NORFLASH with MTD_GENERIC_TYPE

[Artem B. Bityutskiy]

Jörn Engel wrote:
> I don't understand how that layer would not introduce overhead.  But
> maybe you have some explanation in diff -up format.  That would
> certainly make things easier to understand.

I wrote this already. Sketck:

1. add a 'void *capabilities' field to mtd_info.

2. create mtd_capps.c like this:

#include <mtd_capps.h>

/*
  * Called from mtd_add_device() or like such, i.e., on MTD device
  * initialization.
  */
int mtd_capabilities_init(struct mtd_info *mtd)
{
	struct mtd_capabilities *caps;

	caps = mtd->capabilities = kmalloc(sizeof(struct mtd_capabilities), 
GFP_KERNEL);
	if (!mtd->capabilities)
		return -ENOMEM;

	switch (mtd->type) {
	case MTD_NORFLASH:
		caps->can_milk_goat = 0;
		caps->can_pass_iq_tests = 1;
		/* ETC */
		break;
	case MTD_NANDFLASH:
		/* ETC */
		break;
	/* ETC */
	default:
		return -EINVAL;
	}
	
	return 0;
}

/*
  * Called from mtd_del_device() or like such, i.e., on MTD device
  * de-initialization.
  */
void mtd_capabilities_free(struct mtd_info *mtd)
{
	kfree(mtd->capabilities);
}

3. create mtd_capps.h like this:

struct mtd_capabilities {
	int can_milk_goat;
	int can_pass_iq_tests;
	/* ETC */
};

/* Can this flash milk goats ? */
#define mtd_can_milk_goat(mtd) \
	((struct mtd_capabilities*)(mtd->capabilities))->can_milk_goat

-----

Well, in this case word "layer" becomes not very appropriate. It'll be a 
distinct *unit* within MTD. But the idea to have all the 
application-dependent stuff in one place, not scattered over drivers, stays.

-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.