Re: udev problem (and fix) for /dev/mtdblock*

Re: udev problem (and fix) for /dev/mtdblock*

[Kay Sievers]

On Wed, Mar 18, 2009 at 20:44, David Brownell <david-b@pacbell.net> wrote:

> I have a machine running Debian and it's been getting nasty
> boot messages like:
>
>  ...
>  Synthesizing the initial hotplug events...done.
>  Waiting for /dev to be fully populated...uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 0
>  Buffer I/O error on device mtdblock0, logical block 0
>  uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 8
>  Buffer I/O error on device mtdblock0, logical block 1
>  end_request: I/O error, dev mtdblock0, sector 16
>  Buffer I/O error on device mtdblock0, logical block 2
>  end_request: I/O error, dev mtdblock0, sector 24
>  Buffer I/O error on device mtdblock0, logical block 3
>  uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 0
>  Buffer I/O error on device mtdblock0, logical block 0
>  done.
>  Setting parameters of disc: (none).
>  ...
>
> Where /dev/mtdblock0 is something of a special NAND partition,
> used by the mask ROM on the ARM to get a second stage loader,
> which is then loaded from /dev/mtdblock1 etc.  The messages
> about "uncorrectable error" are from the kernel, since that
> first partition uses a different ECC scheme than the rest of
> the flash.
>
> What's happening is that it's trying to read partition data
> from the MTD devices ... which is inappropriate, they don't
> use internal partition data.  And the mtdblock0 read fails,
> because of the ECC differences, producing messages ... ones
> that should never have appeared, since it shouldn't have
> been trying to read partition data.
>
> The boot is easily cleaned up by a patch to a udev rules
> file:  /etc/udev/rules.d/60-persistent-storage.rules is
> already skipping a bunch of devices, it just needs to add
> the "mtd*" devices to what should be skipped:
>
>  KERNEL="ram*|loop*|fd*|mtd*|nbd*|gnbd*|dm-*|md*", GOTO="persistent_storage_end"
>
> It'd be good to see this bug fixed before lenny goes final...

Udev, by default, investigates all block devices which are created by
the kernel. People use label/uuid and other metadata based stuff, so
we should look at them. We can not know in advance which device will
not respond properly. I have requests where people ask for more
persistent link support for mtd devices, while disabling them would
really not make them happier. :)

I don't see how userspace could work around this, and guess the kernel
should stop printing errors for stuff it has announced to userspace,
but is not usable when we look at it.

Would it help, if we make it easier to disable these rules for
specific devices from a custom udev rule?

Thanks,
Kay

Re: udev problem (and fix) for /dev/mtdblock*

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

On Thu, 2009-03-19 at 02:45 +0100, Kay Sievers wrote:

> > The boot is easily cleaned up by a patch to a udev rules
> > file:  /etc/udev/rules.d/60-persistent-storage.rules is
> > already skipping a bunch of devices, it just needs to add
> > the "mtd*" devices to what should be skipped:
> >
> >  KERNEL=="ram*|loop*|fd*|mtd*|nbd*|gnbd*|dm-*|md*", GOTO="persistent_storage_end"
> >
> > It'd be good to see this bug fixed before lenny goes final...
> 
> Udev, by default, investigates all block devices which are created by
> the kernel. People use label/uuid and other metadata based stuff, so
> we should look at them. We can not know in advance which device will
> not respond properly. I have requests where people ask for more
> persistent link support for mtd devices, while disabling them would
> really not make them happier. :)
> 
Indeed, for the Ubuntu ARM port we *rely* on having UUIDs for the MTD
partitions.  I would be unhappy if mtd devices did not have them.

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: udev problem (and fix) for /dev/mtdblock*

[David Brownell]

On Wednesday 18 March 2009, Kay Sievers wrote:
> On Wed, Mar 18, 2009 at 20:44, David Brownell <david-b@pacbell.net> wrote:
> 
> > I have a machine running Debian and it's been getting nasty
> > boot messages like:
> >
> >  ...
> >  Synthesizing the initial hotplug events...done.
> >  Waiting for /dev to be fully populated...uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 0
> >  Buffer I/O error on device mtdblock0, logical block 0
> >  uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 8
> >  Buffer I/O error on device mtdblock0, logical block 1
> >  end_request: I/O error, dev mtdblock0, sector 16
> >  Buffer I/O error on device mtdblock0, logical block 2
> >  end_request: I/O error, dev mtdblock0, sector 24
> >  Buffer I/O error on device mtdblock0, logical block 3
> >  uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 0
> >  Buffer I/O error on device mtdblock0, logical block 0
> >  done.
> >  Setting parameters of disc: (none).
> >  ...
> >
> > Where /dev/mtdblock0 is something of a special NAND partition,
> > used by the mask ROM on the ARM to get a second stage loader,
> > which is then loaded from /dev/mtdblock1 etc.  The messages
> > about "uncorrectable error" are from the kernel, since that
> > first partition uses a different ECC scheme than the rest of
> > the flash.
> >
> > What's happening is that it's trying to read partition data
> > from the MTD devices ... which is inappropriate, they don't
> > use internal partition data.  And the mtdblock0 read fails,
> > because of the ECC differences, producing messages ... ones
> > that should never have appeared, since it shouldn't have
> > been trying to read partition data.
> >
> > The boot is easily cleaned up by a patch to a udev rules
> > file:  /etc/udev/rules.d/60-persistent-storage.rules is
> > already skipping a bunch of devices, it just needs to add
> > the "mtd*" devices to what should be skipped:
> >
> >  KERNEL="ram*|loop*|fd*|mtd*|nbd*|gnbd*|dm-*|md*", GOTO="persistent_storage_end"
> >
> > It'd be good to see this bug fixed before lenny goes final...
> 
> Udev, by default, investigates all block devices which are created by
> the kernel.

Except for the seven exceptions listed above...

I'm just pointing out an eighth one that should be
included in that list, as a bugfix:  "mtd*".
(Or maybe "mtdblock*".)


> People use label/uuid and other metadata based stuff, so 
> we should look at them.

Yes, people are stupid too.  Such things are basically
meaningless for MTDs.  They don't have labels as those
tools understand them, and partition data is normally
part of board-specific kernel configuration data (or
even kernel command line data passed from bootloaders).

And there's MTD-specific metadata, like the OTP bits
provided on most modern chips and which MTD technology
is used by that device.  Not that it's used like disk
labels are ... but if "metadata" is going to be your
argument, rather than "sort out which disk goes where",
you've come up with yet another reason to treat MTD
hardware differently than udev does today.


See also:

  http://www.linux-mtd.infradead.org/faq/general.html#L_mtd_what
  http://www.linux-mtd.infradead.org/faq/general.html#L_mtd_vs_hdd

Plus the "can I use ext2 on an MTD device" section.

Don't be deceived by the need to have a block device to
mount JFFS2.  Anyone trying to use one of the /dev/mtdblock
devices as a general block device is risking system damage.
Example, with an MLC NAND operation, don't just randomly
update an erase block 1000 times, you will probably kill it
forever.  And if it's already a bad block ... well, it doesn't
handle that well.


> I don't see how userspace could work around this,

Well, the obvious fix is the one I described:  add the
/dev/mtdblock devices to that existing exception list
in udev.

A less obvious one would be to teach every tool that
udev could possibly point towards MTDs to first check
if the device is an MTD, and refuse to use it.  Since
that set of tools is unbounded, this approach clearly
loses.

Another would be to use MTD-specific tools for MTDs.
Of course, those tools aren't used to do anything like
what that persistent-rules file is doing -- and in fact
those tools use the chardev interfaces to MTDs, not the
blockdev ones -- so that's equivalent to the original
fix:  do nothing for any mtdblock device nodes you find.


> and guess the kernel 
> should stop printing errors for stuff it has announced to userspace,
> but is not usable when we look at it.

It's perfectly usable.  Not by the tools udev tries to
use with it, but by "mount" ... /dev/mtdblock* nodes
exist only to support "mount".  (And maybe just for
JFFS2; UBIFS doesn't need them, and I've not looked
at other options.)

Agreed it would be nice to see /dev/mtdblock* vanish,
but that wouldn't be a near-term change.  JFFS would
first need to vanish; I don't know about YAFFS2 (which
may never make mainline given UBIFS).


> Would it help, if we make it easier to disable these rules for
> specific devices from a custom udev rule?

What's needed is to recognize that MTDs are not
block devices in any meaningful sense.  This is
not a "custom" thing -- it's a "standard" thing.

- Dave

 
> Thanks,
> Kay
> 
> 

Re: udev problem (and fix) for /dev/mtdblock*

[Kay Sievers]

On Fri, Mar 20, 2009 at 19:46, David Brownell <david-b@pacbell.net> wrote:
> On Wednesday 18 March 2009, Kay Sievers wrote:
>> On Wed, Mar 18, 2009 at 20:44, David Brownell <david-b@pacbell.net> wrote:

>> >  KERNEL="ram*|loop*|fd*|mtd*|nbd*|gnbd*|dm-*|md*", GOTO="persistent_storage_end"
>> >
>> > It'd be good to see this bug fixed before lenny goes final...
>>
>> Udev, by default, investigates all block devices which are created by
>> the kernel.
>
> Except for the seven exceptions listed above...

Loop is no longer there today, ram can also be removed. The others are
remote network devices, which are difficult to handle, and can not be
identified at creation event time. Floppies do not detect any media
changes, and are excluded for that reason. Dm and md have their own
rules.

>> People use label/uuid and other metadata based stuff, so
>> we should look at them.
>
> Yes, people are stupid too. Such things are basically
> meaningless for MTDs.

Maybe, but seems they are used, because mtd currently does not offer
any other attributes to identify a device.

> They don't have labels as those
> tools understand them, and partition data is normally
> part of board-specific kernel configuration data (or
> even kernel command line data passed from bootloaders).
>
> And there's MTD-specific metadata, like the OTP bits
> provided on most modern chips and which MTD technology
> is used by that device.

If we can provide any other useful data to identify devices, it would
be nice, if someone could make that work. Can such attributes somehow
be added to sysfs?

MTD currently creates all devices as "virtual", it does not even use
the proper parent devices in the kernel, which would at least allow a
very simple identification, based on the underlying platform device
properties.

I keep an untested and unfinished patch around, I did the last time
someone tried to solve the problem of reordered mtd devices with udev
rules:
  http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=mtd-parent-dev.patch;hb=HEAD

If we can offer people the information which will be available with
this rather simple change, people can probably switch over to that,
and we can replace the current one.

Thanks,
Kay

Re: udev problem (and fix) for /dev/mtdblock*

[David Brownell]

On Sunday 22 March 2009, Kay Sievers wrote:
> On Fri, Mar 20, 2009 at 19:46, David Brownell <david-b@pacbell.net> wrote:
> > ... etc, dating from last fall ...
> 
> >> >  KERNEL="ram*|loop*|fd*|mtd*|nbd*|gnbd*|dm-*|md*", GOTO="persistent_storage_end"
> >> >
> >> > It'd be good to see this bug fixed before lenny goes final...
> >>
> >> Udev, by default, investigates all block devices which are created by
> >> the kernel.
> >
> > Except for the seven exceptions listed above...
> 
> Loop is no longer there today, ram can also be removed. The others are
> remote network devices, which are difficult to handle, and can not be
> identified at creation event time. Floppies do not detect any media
> changes, and are excluded for that reason. Dm and md have their own
> rules.

All beside the point.  That's not "all" block devices;
and "mtdblock" should be in that exception list.  The
problem is that MTDs don't support the model implemented
in the rest of that file 


> >> People use label/uuid and other metadata based stuff, so
> >> we should look at them.
> >
> > Yes, people are stupid too. Such things are basically
> > meaningless for MTDs.
> 
> Maybe, but seems they are used, because mtd currently does not offer
> any other attributes to identify a device.
> 
> > They don't have labels as those
> > tools understand them, and partition data is normally
> > part of board-specific kernel configuration data (or
> > even kernel command line data passed from bootloaders).

If "they are used", it's by a decided minority of MTDs.
Th


> > And there's MTD-specific metadata, like the OTP bits
> > provided on most modern chips and which MTD technology
> > is used by that device.
> 
> If we can provide any other useful data to identify devices, it would
> be nice, if someone could make that work. Can such attributes somehow
> be added to sysfs?

In some cases, maybe ... thing is that OTP (One Time Programmable)
bits vary widely in terms of structure, size, and even presence.

OTP data in lot of the NAND chips I have here exceeds the sysfs 4KB
binary file limit; 20KB commonly, 128KB for OneNand.  That's enough
that some systems would not want to model it as metadata.  And the
"manufacturer" data (basically, chip-specific IDs) are not always
documented such that GPL'd code could access them.


> MTD currently creates all devices as "virtual", it does not even use
> the proper parent devices in the kernel, which would at least allow a
> very simple identification, based on the underlying platform device
> properties.

That would be an entirely separate problem though.  ISTR
having a similar "WTF is this doing" reaction last time I
had to look at MTD core driver model stuff.


> I keep an untested and unfinished patch around, I did the last time
> someone tried to solve the problem of reordered mtd devices with udev
> rules:
>   http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=mtd-parent-dev.patch;hb=HEAD

Looks plausible ... and I may have actually seen a system where
that "problem" matters.  (Normally all flash drivers are linked
statically, and the system root uses one of them.)

That obviously needs to be split into "core" (mtd block and char
dev support) and driver bits; the core bits could go in at any
time once they're ready.

FWIW I did a quick test of this on a physmap flash ... didn't
work, /sys/devices/virtual/mtd still held everything that was
not in /sys/devices/virtual/block/mtdblock*.  I think the issue
might be lack of mtdpart support.


> If we can offer people the information which will be available with
> this rather simple change, people can probably switch over to that,
> and we can replace the current one.

Best to get rid of the currently-broken udev bits first.  There's
no particular reason MTD couldn't add that other feature though.

- Dave

Re: udev problem (and fix) for /dev/mtdblock*

[Kay Sievers]

On Mon, Mar 23, 2009 at 19:49, David Brownell <david-b@pacbell.net> wrote:
> On Sunday 22 March 2009, Kay Sievers wrote:
>> On Fri, Mar 20, 2009 at 19:46, David Brownell <david-b@pacbell.net> wrote:

>> > And there's MTD-specific metadata, like the OTP bits
>> > provided on most modern chips and which MTD technology
>> > is used by that device.
>>
>> If we can provide any other useful data to identify devices, it would
>> be nice, if someone could make that work. Can such attributes somehow
>> be added to sysfs?
>
> In some cases, maybe ... thing is that OTP (One Time Programmable)
> bits vary widely in terms of structure, size, and even presence.
>
> OTP data in lot of the NAND chips I have here exceeds the sysfs 4KB
> binary file limit; 20KB commonly, 128KB for OneNand.

There is no such limit for binary sysfs files. We have many large file
there, which map such sizes.

>> MTD currently creates all devices as "virtual", it does not even use
>> the proper parent devices in the kernel, which would at least allow a
>> very simple identification, based on the underlying platform device
>> properties.
>
> That would be an entirely separate problem though.  ISTR
> having a similar "WTF is this doing" reaction last time I
> had to look at MTD core driver model stuff.

Yeah, it definitely needs fixing. It's not only the driver core, also
all mtd devices share a single block request queue for all devices,
which is pretty weird.

>> I keep an untested and unfinished patch around, I did the last time
>> someone tried to solve the problem of reordered mtd devices with udev
>> rules:
>>   http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=mtd-parent-dev.patch;hb=HEAD
>
> Looks plausible ... and I may have actually seen a system where
> that "problem" matters.  (Normally all flash drivers are linked
> statically, and the system root uses one of them.)
>
> That obviously needs to be split into "core" (mtd block and char
> dev support) and driver bits; the core bits could go in at any
> time once they're ready.
>
> FWIW I did a quick test of this on a physmap flash ... didn't
> work, /sys/devices/virtual/mtd still held everything that was
> not in /sys/devices/virtual/block/mtdblock*.  I think the issue
> might be lack of mtdpart support.

Would be great, if you possibly could find out how to get something
like this working, so the mtd devices which are backed by real
hardware would no longer show up as virtual, and people have at least
a chance to identify them by the properties of the platform devices.

>> If we can offer people the information which will be available with
>> this rather simple change, people can probably switch over to that,
>> and we can replace the current one.
>
> Best to get rid of the currently-broken udev bits first.

Done.

Thanks,
Kay

Re: udev problem (and fix) for /dev/mtdblock*

[David Brownell]

On Monday 23 March 2009, Kay Sievers wrote:
> >> I keep an untested and unfinished patch around, I did the last time
> >> someone tried to solve the problem of reordered mtd devices with udev
> >> rules:
> >>   http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=blob;f=mtd-parent-dev.patch;hb=HEAD
> >
> > Looks plausible ... and I may have actually seen a system where
> > that "problem" matters.  (Normally all flash drivers are linked
> > statically, and the system root uses one of them.)
> >
> > That obviously needs to be split into "core" (mtd block and char
> > dev support) and driver bits; the core bits could go in at any
> > time once they're ready.
> >
> > FWIW I did a quick test of this on a physmap flash ... didn't
> > work, /sys/devices/virtual/mtd still held everything that was
> > not in /sys/devices/virtual/block/mtdblock*.  I think the issue
> > might be lack of mtdpart support.
> 
> Would be great, if you possibly could find out how to get something
> like this working, so the mtd devices which are backed by real
> hardware would no longer show up as virtual, and people have at least
> a chance to identify them by the properties of the platform devices.

See the appended.  I've not split out "core" bits yet, or
provided all attributes I expect would eventually appear.

====== CUT HERE
This patch updates driver model support in the MTD framework:

 - Each mtd_info now has a device node; MTD drivers should set
   its parent field to point to the physical device, before
   setting up partitions.

 - Those device nodes always map to /sys/class/mtdX device nodes,
   which no longer depend on MTD_CHARDEV.

 - Those mtdX sysfs nodes have a "starter set" of attributes;
   it's not yet sufficient to replace /proc/mtd

 - Enabling MTD_CHARDEV provides /sys/class/mtdXro/ nodes and the
   /sys/class/mtd*/dev attributes (for udev, mdev, etc).

So the sysfs structure is pretty much what you'd expect, except
that readonly chardev nodes are a bit quirky.

Based on a partial patch from Kay Sievers.

Sanity tested with NOR (physmap, omap_nor, m25p80), DataFlash,
NAND (davinci, omap), OneNand (omap); with and without mtdblock.
When the MTD driver has been converted, /sys/devices/virtual/mtd/*
nodes are gone.

NOT YET SIGNED-OFF ... needs splitting into core vs drivers,
and probably a few more attributes.

---
 drivers/mtd/devices/m25p80.c        |    2 
 drivers/mtd/devices/mtd_dataflash.c |    2 
 drivers/mtd/maps/omap_nor.c         |    2 
 drivers/mtd/maps/physmap.c          |   15 ++--
 drivers/mtd/maps/plat-ram.c         |    1 
 drivers/mtd/mtd_blkdevs.c           |    1 
 drivers/mtd/mtdchar.c               |   46 +------------
 drivers/mtd/mtdcore.c               |  115 ++++++++++++++++++++++++++++++++++
 drivers/mtd/mtdpart.c               |    9 ++
 drivers/mtd/nand/davinci_nand.c     |    2 
 drivers/mtd/nand/mxc_nand.c         |    1 
 drivers/mtd/onenand/omap2.c         |    2 
 include/linux/mtd/mtd.h             |    7 ++
 13 files changed, 156 insertions(+), 49 deletions(-)

--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -678,6 +678,8 @@ static int __devinit m25p_probe(struct s
 		flash->mtd.erasesize = info->sector_size;
 	}
 
+	flash->mtd.dev.parent = &spi->dev;
+
 	dev_info(&spi->dev, "%s (%lld Kbytes)\n", info->name,
 			(long long)flash->mtd.size >> 10);
 
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -670,6 +670,8 @@ add_dataflash_otp(struct spi_device *spi
 	device->write = dataflash_write;
 	device->priv = priv;
 
+	device->dev.parent = &spi->dev;
+
 	if (revision >= 'c')
 		otp_tag = otp_setup(device, revision);
 
--- a/drivers/mtd/maps/omap_nor.c
+++ b/drivers/mtd/maps/omap_nor.c
@@ -115,6 +115,8 @@ static int __init omapflash_probe(struct
 	}
 	info->mtd->owner = THIS_MODULE;
 
+	info->mtd->dev.parent = &pdev->dev;
+
 #ifdef CONFIG_MTD_PARTITIONS
 	err = parse_mtd_partitions(info->mtd, part_probes, &info->parts, 0);
 	if (err > 0)
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -147,6 +147,7 @@ static int physmap_flash_probe(struct pl
 			devices_found++;
 		}
 		info->mtd[i]->owner = THIS_MODULE;
+		info->mtd[i]->dev.parent = &dev->dev;
 	}
 
 	if (devices_found = 1) {
@@ -171,16 +172,16 @@ static int physmap_flash_probe(struct pl
 #ifdef CONFIG_MTD_PARTITIONS
 	err = parse_mtd_partitions(info->cmtd, part_probe_types,
 				&info->parts, 0);
-	if (err > 0) {
-		add_mtd_partitions(info->cmtd, info->parts, err);
+	if (err > 0)
 		info->nr_parts = err;
-		return 0;
+	else {
+		dev_notice(&dev->dev, "Using static partition information\n");
+		info->nr_parts = physmap_data->nr_parts;
+		info->parts = physmap_data->parts;
 	}
 
-	if (physmap_data->nr_parts) {
-		printk(KERN_NOTICE "Using physmap partition information\n");
-		add_mtd_partitions(info->cmtd, physmap_data->parts,
-				   physmap_data->nr_parts);
+	if (info->nr_parts > 0) {
+		add_mtd_partitions(info->cmtd, info->parts, info->nr_parts);
 		return 0;
 	}
 #endif
--- a/drivers/mtd/maps/plat-ram.c
+++ b/drivers/mtd/maps/plat-ram.c
@@ -224,6 +224,7 @@ static int platram_probe(struct platform
 	}
 
 	info->mtd->owner = THIS_MODULE;
+	info->mtd->dev.parent = &pdev->dev;
 
 	platram_setrw(info, PLATRAM_RW);
 
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -286,6 +286,7 @@ int add_mtd_blktrans_dev(struct mtd_blkt
 	gd->private_data = new;
 	new->blkcore_priv = gd;
 	gd->queue = tr->blkcore_priv->rq;
+	gd->driverfs_dev = new->mtd->dev.parent;
 
 	if (new->readonly)
 		set_disk_ro(gd, 1);
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -19,33 +19,6 @@
 
 #include <asm/uaccess.h>
 
-static struct class *mtd_class;
-
-static void mtd_notify_add(struct mtd_info* mtd)
-{
-	if (!mtd)
-		return;
-
-	device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2),
-		      NULL, "mtd%d", mtd->index);
-
-	device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2+1),
-		      NULL, "mtd%dro", mtd->index);
-}
-
-static void mtd_notify_remove(struct mtd_info* mtd)
-{
-	if (!mtd)
-		return;
-
-	device_destroy(mtd_class, MKDEV(MTD_CHAR_MAJOR, mtd->index*2));
-	device_destroy(mtd_class, MKDEV(MTD_CHAR_MAJOR, mtd->index*2+1));
-}
-
-static struct mtd_notifier notifier = {
-	.add	= mtd_notify_add,
-	.remove	= mtd_notify_remove,
-};
 
 /*
  * Data structure to hold the pointer to the mtd device as well
@@ -793,28 +766,19 @@ static const struct file_operations mtd_
 
 static int __init init_mtdchar(void)
 {
-	if (register_chrdev(MTD_CHAR_MAJOR, "mtd", &mtd_fops)) {
+	int status;
+
+	status = register_chrdev(MTD_CHAR_MAJOR, "mtd", &mtd_fops);
+	if (status < 0) {
 		printk(KERN_NOTICE "Can't allocate major number %d for Memory Technology Devices.\n",
 		       MTD_CHAR_MAJOR);
-		return -EAGAIN;
 	}
 
-	mtd_class = class_create(THIS_MODULE, "mtd");
-
-	if (IS_ERR(mtd_class)) {
-		printk(KERN_ERR "Error creating mtd class.\n");
-		unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
-		return PTR_ERR(mtd_class);
-	}
-
-	register_mtd_user(&notifier);
-	return 0;
+	return status;
 }
 
 static void __exit cleanup_mtdchar(void)
 {
-	unregister_mtd_user(&notifier);
-	class_destroy(mtd_class);
 	unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
 }
 
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -22,6 +22,9 @@
 
 #include "mtdcore.h"
 
+
+static struct class *mtd_class;
+
 /* These are exported solely for the purpose of mtd_blkdevs.c. You
    should not use them for _anything_ else */
 DEFINE_MUTEX(mtd_table_mutex);
@@ -32,6 +35,82 @@ EXPORT_SYMBOL_GPL(mtd_table);
 
 static LIST_HEAD(mtd_notifiers);
 
+
+#if defined(CONFIG_MTD_CHAR) || defined(CONFIG_MTD_CHAR_MODULE)
+#define MTD_DEVT(index) MKDEV(MTD_CHAR_MAJOR, (index)*2)
+#else
+#define MTD_DEVT(index) 0
+#endif
+
+/* REVISIT once MTD uses the driver model better, whoever allocates
+ * the mtd_info will probably want to use the release() hook...
+ */
+static void mtd_release(struct device *dev)
+{
+	struct mtd_info *mtd = dev_to_mtd(dev);
+
+	/* remove /dev/mtdXro node if needed */
+	if (MTD_DEVT(mtd->index))
+		device_destroy(mtd_class, MTD_DEVT(mtd->index) + 1);
+}
+
+static ssize_t mtd_type_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_to_mtd(dev);
+	char *type;
+
+	switch (mtd->type) {
+	case MTD_ABSENT:
+		type = "absent";
+		break;
+	case MTD_RAM:
+		type = "ram";
+		break;
+	case MTD_ROM:
+		type = "rom";
+		break;
+	case MTD_NORFLASH:
+		type = "nor";
+		break;
+	case MTD_NANDFLASH:
+		type = "nand";
+		break;
+	case MTD_DATAFLASH:
+		type = "dataflash";
+		break;
+	case MTD_UBIVOLUME:
+		type = "ubi";
+		break;
+	default:
+		type = "unknown";
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", type);
+}
+static DEVICE_ATTR(mtd_type, S_IRUGO, mtd_type_show, NULL);
+
+static struct attribute *mtd_attrs[] = {
+	&dev_attr_mtd_type.attr,
+	/* FIXME provide a /proc/mtd superset */
+	NULL,
+};
+
+struct attribute_group mtd_group = {
+	.attrs		= mtd_attrs,
+};
+
+struct attribute_group *mtd_groups[] = {
+	&mtd_group,
+	NULL,
+};
+
+static struct device_type mtd_devtype = {
+	.name		= "mtd",
+	.groups		= mtd_groups,
+	.release	= mtd_release,
+};
+
 /**
  *	add_mtd_device - register an MTD device
  *	@mtd: pointer to new MTD device info structure
@@ -40,6 +119,7 @@ static LIST_HEAD(mtd_notifiers);
  *	notify each currently active MTD 'user' of its arrival. Returns
  *	zero on success or 1 on failure, which currently will only happen
  *	if the number of present devices exceeds MAX_MTD_DEVICES (i.e. 16)
+ *	or there's a sysfs error.
  */
 
 int add_mtd_device(struct mtd_info *mtd)
@@ -80,6 +160,23 @@ int add_mtd_device(struct mtd_info *mtd)
 					       mtd->name);
 			}
 
+			/* Caller should have set dev.parent to match the
+			 * physical device.
+			 */
+			mtd->dev.type = &mtd_devtype;
+			mtd->dev.class = mtd_class;
+			mtd->dev.devt = MTD_DEVT(i);
+			dev_set_name(&mtd->dev, "mtd%d", i);
+			if (device_register(&mtd->dev) != 0) {
+				mtd_table[i] = NULL;
+				break;
+			}
+
+			if (MTD_DEVT(i))
+				device_create(mtd_class, mtd->dev.parent,
+						MTD_DEVT(i) + 1,
+						NULL, "mtd%dro", i);
+
 			DEBUG(0, "mtd: Giving out device %d to %s\n",i, mtd->name);
 			/* No need to get a refcount on the module containing
 			   the notifier, since we hold the mtd_table_mutex */
@@ -343,6 +440,24 @@ EXPORT_SYMBOL_GPL(register_mtd_user);
 EXPORT_SYMBOL_GPL(unregister_mtd_user);
 EXPORT_SYMBOL_GPL(default_mtd_writev);
 
+static int __init mtd_setup(void)
+{
+	mtd_class = class_create(THIS_MODULE, "mtd");
+
+	if (IS_ERR(mtd_class)) {
+		pr_err("Error creating mtd class.\n");
+		return PTR_ERR(mtd_class);
+	}
+	return 0;
+}
+core_initcall(mtd_setup);
+
+static void __exit mtd_teardown(void)
+{
+	class_destroy(mtd_class);
+}
+__exitcall(mtd_teardown);
+
 #ifdef CONFIG_PROC_FS
 
 /*==================================*/
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -343,6 +343,11 @@ static struct mtd_part *add_one_partitio
 	slave->mtd.name = part->name;
 	slave->mtd.owner = master->owner;
 
+	/* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
+	 * to have the same data be in two different partitions.
+	 */
+	slave->mtd.dev.parent = master->dev.parent;
+
 	slave->mtd.read = part_read;
 	slave->mtd.write = part_write;
 
@@ -493,7 +498,9 @@ out_register:
  * This function, given a master MTD object and a partition table, creates
  * and registers slave MTD objects which are bound to the master according to
  * the partition definitions.
- * (Q: should we register the master MTD object as well?)
+ *
+ * We don't register the master, or expect the caller to have done so,
+ * for reasons of data integrity.
  */
 
 int add_mtd_partitions(struct mtd_info *master,
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -615,6 +615,8 @@ static int __init nand_davinci_probe(str
 	info->mtd.name		= dev_name(&pdev->dev);
 	info->mtd.owner		= THIS_MODULE;
 
+	info->mtd.dev.parent	= &pdev->dev;
+
 	info->chip.IO_ADDR_R	= vaddr;
 	info->chip.IO_ADDR_W	= vaddr;
 	info->chip.chip_delay	= 0;
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -866,6 +866,7 @@ static int __init mxcnd_probe(struct pla
 	mtd = &host->mtd;
 	mtd->priv = this;
 	mtd->owner = THIS_MODULE;
+	mtd->dev.parent = &pdev->dev;
 
 	/* 50 us command delay time */
 	this->chip_delay = 5;
--- a/drivers/mtd/onenand/omap2.c
+++ b/drivers/mtd/onenand/omap2.c
@@ -672,6 +672,8 @@ static int __devinit omap2_onenand_probe
 	c->mtd.priv = &c->onenand;
 	c->mtd.owner = THIS_MODULE;
 
+	c->mtd.dev.parent = &pdev->dev;
+
 	if (c->dma_channel >= 0) {
 		struct onenand_chip *this = &c->onenand;
 
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/uio.h>
 #include <linux/notifier.h>
+#include <linux/device.h>
 
 #include <linux/mtd/compatmac.h>
 #include <mtd/mtd-abi.h>
@@ -223,6 +224,7 @@ struct mtd_info {
 	void *priv;
 
 	struct module *owner;
+	struct device dev;
 	int usecount;
 
 	/* If the driver is something smart, like UBI, it may need to maintain
@@ -233,6 +235,11 @@ struct mtd_info {
 	void (*put_device) (struct mtd_info *mtd);
 };
 
+static inline struct mtd_info *dev_to_mtd(struct device *dev)
+{
+	return dev ? container_of(dev, struct mtd_info, dev) : NULL;
+}
+
 static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd)
 {
 	if (mtd->erasesize_shift)

Re: udev problem (and fix) for /dev/mtdblock*

[David Brownell]

On Monday 23 March 2009, Kay Sievers wrote:
> > OTP data in lot of the NAND chips I have here exceeds the sysfs 4KB
> > binary file limit; 20KB commonly, 128KB for OneNand.
> 
> There is no such limit for binary sysfs files. We have many large file
> there, which map such sizes.

Odd, just the other day I created one with actual size of 8KB.
But "ls -l" showed 4K.  That's clearly a limit... and "cp" was
unable to read the whole thing, it stopped at 4K.  "dd" could
read everything though:  dd bs=1k count=8 did it.

Something seems odd with binary sysfs files in general.  I've
observed something zapping size down to zero after userspace
did some oddball reads ... of the "past EOF" variety, ISTR.

Re: udev problem (and fix) for /dev/mtdblock*

[Kay Sievers]

On Mon, Mar 23, 2009 at 21:39, David Brownell <david-b@pacbell.net> wrote:
> On Monday 23 March 2009, Kay Sievers wrote:
>> > OTP data in lot of the NAND chips I have here exceeds the sysfs 4KB
>> > binary file limit; 20KB commonly, 128KB for OneNand.
>>
>> There is no such limit for binary sysfs files. We have many large file
>> there, which map such sizes.
>
> Odd, just the other day I created one with actual size of 8KB.
> But "ls -l" showed 4K.

There are larger files:
$ ls -l /sys/devices/pci .../usb1/1-1/descriptors
-r--r--r-- 1 root root 65553 2009-03-23 22:12 ... /usb1/1-1/descriptors

> That's clearly a limit... and "cp" was
> unable to read the whole thing, it stopped at 4K.  "dd" could
> read everything though:  dd bs=1k count=8 did it.

Some larger files seem to work here:
$ ls -l /sys/devices/pci0000:00/0000:00:02.0/rom
-r-------- 1 root root 131072 2009-03-23 22:13 /sys/devices/pci ...

$ cat /sys/devices/pci0000:00/0000:00:02.0/rom | wc -c
65536

$ dd if=/sys/devices/.../rom of=/dev/null bs=1
65536+0 records in
65536+0 records out
65536 bytes (66 kB) copied, 0.103753 s, 632 kB/s

> Something seems odd with binary sysfs files in general.  I've
> observed something zapping size down to zero after userspace
> did some oddball reads ... of the "past EOF" variety, ISTR.

Hmm, never seen such a thing, but the logic is implemented per-file,
so it might be something that should be fixed in the driver that
created the file.

Kay

Re: udev problem (and fix) for /dev/mtdblock*

[David Brownell]

On Monday 23 March 2009, Kay Sievers wrote:
> On Mon, Mar 23, 2009 at 21:39, David Brownell <david-b@pacbell.net> wrote:
> > On Monday 23 March 2009, Kay Sievers wrote:
> >> > OTP data in lot of the NAND chips I have here exceeds the sysfs 4KB
> >> > binary file limit; 20KB commonly, 128KB for OneNand.
> >>
> >> There is no such limit for binary sysfs files. We have many large file
> >> there, which map such sizes.
> >
> > Odd, just the other day I created one with actual size of 8KB.
> > But "ls -l" showed 4K.
> 
> There are larger files:

Could be.  But still ... I set the size of the attribute to 8K,
but "ls" reported only 4K.


> > Something seems odd with binary sysfs files in general.  I've
> > observed something zapping size down to zero after userspace
> > did some oddball reads ... of the "past EOF" variety, ISTR.
> 
> Hmm, never seen such a thing, but the logic is implemented per-file,
> so it might be something that should be fixed in the driver that
> created the file.

All those drivers do is respond to read/write calls, and set
the size statically.


I've seen both those failures in multiple drivers, FWIW.

- Dave

Fwd: udev problem (and fix) for /dev/mtdblock*

[David Brownell]

Marco suggested this goes "the upstream list" but didn't
forward it or say what that list is... udev still goes
to the hotplug list?


----------  Forwarded Message  ----------

Subject: udev problem (and fix) for lenny/armel
Date: Thursday 20 November 2008
From: David Brownell <david-b@pacbell.net>
To: md@linux.it

Hi,

I have a machine running Debian and it's been getting nasty
boot messages like:

  ...
  Synthesizing the initial hotplug events...done.
  Waiting for /dev to be fully populated...uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 0
  Buffer I/O error on device mtdblock0, logical block 0
  uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 8
  Buffer I/O error on device mtdblock0, logical block 1
  end_request: I/O error, dev mtdblock0, sector 16
  Buffer I/O error on device mtdblock0, logical block 2
  end_request: I/O error, dev mtdblock0, sector 24 
  Buffer I/O error on device mtdblock0, logical block 3
  uncorrectable error : <3>end_request: I/O error, dev mtdblock0, sector 0
  Buffer I/O error on device mtdblock0, logical block 0
  done.
  Setting parameters of disc: (none).
  ...

Where /dev/mtdblock0 is something of a special NAND partition,
used by the mask ROM on the ARM to get a second stage loader,
which is then loaded from /dev/mtdblock1 etc.  The messages
about "uncorrectable error" are from the kernel, since that
first partition uses a different ECC scheme than the rest of
the flash.

What's happening is that it's trying to read partition data
from the MTD devices ... which is inappropriate, they don't
use internal partition data.  And the mtdblock0 read fails,
because of the ECC differences, producing messages ... ones
that should never have appeared, since it shouldn't have
been trying to read partition data.

The boot is easily cleaned up by a patch to a udev rules
file:  /etc/udev/rules.d/60-persistent-storage.rules is
already skipping a bunch of devices, it just needs to add
the "mtd*" devices to what should be skipped:

  KERNEL="ram*|loop*|fd*|mtd*|nbd*|gnbd*|dm-*|md*", GOTO="persistent_storage_end"

It'd be good to see this bug fixed before lenny goes final...

- Dave

-------------------------------------------------------