From: David Brownell <[email protected]>
Update driver model support in the MTD framework, so it fits
better into the current udev-based hotplug framework:
- Each mtd_info now has a device node. MTD drivers should set
the dev.parent field to point to the physical device, before
setting up partitions or otherwise declaring MTDs.
- 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).
- Include a MODULE_ALIAS_CHARDEV_MAJOR macro. It'll work with
udev creating the /dev/mtd* nodes, not just a static rootfs.
So the sysfs structure is pretty much what you'd expect, except
that readonly chardev nodes are a bit quirky.
Signed-off-by: David Brownell <[email protected]>
---
Sanity tested with NOR (physmap, omap_nor, m25p80), DataFlash,
NAND (davinci, omap), OneNand (omap); with and without mtdblock.
With converted MTD drivers, /sys/devices/virtual/mtd/* are gone.
Didn't test modular configs.
drivers/mtd/mtd_blkdevs.c | 1
drivers/mtd/mtdchar.c | 47 ++---------------
drivers/mtd/mtdcore.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
drivers/mtd/mtdpart.c | 9 +++
include/linux/mtd/mtd.h | 7 ++
5 files changed, 137 insertions(+), 42 deletions(-)
--- 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,34 +766,26 @@ 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(¬ifier);
- return 0;
+ return status;
}
static void __exit cleanup_mtdchar(void)
{
- unregister_mtd_user(¬ifier);
- class_destroy(mtd_class);
unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
}
module_init(init_mtdchar);
module_exit(cleanup_mtdchar);
+MODULE_ALIAS_CHARDEV_MAJOR(MTD_CHAR_MAJOR);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("David Woodhouse <[email protected]>");
--- 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/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)
On Thursday 26 March 2009, David Brownell wrote:
> From: David Brownell <[email protected]>
>
> Update driver model support in the MTD framework, so it fits
> better into the current udev-based hotplug framework:
Hmm, no comments? I had understood there was interest over on
the MTD side of things in exposing more information through
sysfs, to help avoid the need to add Even More Ioctls as part
of support for things like NAND chips with 4KB pages, or which
handle more than 4GBytes ...
>
> - Each mtd_info now has a device node. MTD drivers should set
> the dev.parent field to point to the physical device, before
> setting up partitions or otherwise declaring MTDs.
>
> - 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).
>
> - Include a MODULE_ALIAS_CHARDEV_MAJOR macro. It'll work with
> udev creating the /dev/mtd* nodes, not just a static rootfs.
>
> So the sysfs structure is pretty much what you'd expect, except
> that readonly chardev nodes are a bit quirky.
>
> Signed-off-by: David Brownell <[email protected]>
> ---
> Sanity tested with NOR (physmap, omap_nor, m25p80), DataFlash,
> NAND (davinci, omap), OneNand (omap); with and without mtdblock.
> With converted MTD drivers, /sys/devices/virtual/mtd/* are gone.
> Didn't test modular configs.
>
> drivers/mtd/mtd_blkdevs.c | 1
> drivers/mtd/mtdchar.c | 47 ++---------------
> drivers/mtd/mtdcore.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/mtd/mtdpart.c | 9 +++
> include/linux/mtd/mtd.h | 7 ++
> 5 files changed, 137 insertions(+), 42 deletions(-)
>
> --- 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,34 +766,26 @@ 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(¬ifier);
> - return 0;
> + return status;
> }
>
> static void __exit cleanup_mtdchar(void)
> {
> - unregister_mtd_user(¬ifier);
> - class_destroy(mtd_class);
> unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
> }
>
> module_init(init_mtdchar);
> module_exit(cleanup_mtdchar);
>
> +MODULE_ALIAS_CHARDEV_MAJOR(MTD_CHAR_MAJOR);
>
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("David Woodhouse <[email protected]>");
> --- 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/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)
>
On Tue, Mar 31, 2009 at 23:18, David Brownell <[email protected]> wrote:
> On Thursday 26 March 2009, David Brownell wrote:
>> From: David Brownell <[email protected]>
>>
>> Update driver model support in the MTD framework, so it fits
>> better into the current udev-based hotplug framework:
>
> Hmm, no comments? I had understood there was interest over on
> the MTD side of things in exposing more information through
> sysfs, to help avoid the need to add Even More Ioctls as part
> of support for things like NAND chips with 4KB pages, or which
> handle more than 4GBytes ...
Please have a look at this. We got asked repeatedly to provide better
hotplug/udev integration, and the patches, and having the parent
device properly assigned, would solve some of the problems people run
into currently.
Thanks,
Kay
On Tue, Mar 31, 2009 at 2:18 PM, David Brownell <[email protected]> wrote:
> Hmm, no comments? I had understood there was interest over on
> the MTD side of things in exposing more information through
> sysfs, to help avoid the need to add Even More Ioctls as part
> of support for things like NAND chips with 4KB pages, or which
> handle more than 4GBytes ...
Based on this post:
http://lists.infradead.org/pipermail/linux-mtd/2009-March/025060.html
I am inferring that the following attributes are wanted in the MTD
sysfs interface:
1) mtd_info_user fields - type, flags, size, erasesize, writesize,
oobsize. So far we have "type," but it should be easy to add the
rest.
2) region_info_user fields? Not really sure how this would work.
Maybe a separate subdirectory for each region?
On Tuesday 31 March 2009, Kevin Cernekee wrote:
> On Tue, Mar 31, 2009 at 2:18 PM, David Brownell <[email protected]> wrote:
> > Hmm, no comments? I had understood there was interest over on
> > the MTD side of things in exposing more information through
> > sysfs, to help avoid the need to add Even More Ioctls as part
> > of support for things like NAND chips with 4KB pages, or which
> > handle more than 4GBytes ...
>
> Based on this post:
> http://lists.infradead.org/pipermail/linux-mtd/2009-March/025060.html
>
> I am inferring that the following attributes are wanted in the MTD
> sysfs interface:
>
> 1) mtd_info_user fields - type, flags, size, erasesize, writesize,
> oobsize. So far we have "type," but it should be easy to add the
> rest.
Yes, easy to add those. As noted in the $SUBJECT patch, the set
consisting of just "type" is a "starter set". :)
For procfs elimination, a name/label would be needed too; that can
be an input to, or output from, cmdlinepart.
My question about the $SUBJECT patch is whether there's any reason
not to use that as the start of such driver model enhancements.
I'm thinking the answer to that seems to be "no", and that someone
more involved in *using* those sysfs attributes would be a better
choice for growing that set of attributes.
> 2) region_info_user fields? Not really sure how this would work.
> Maybe a separate subdirectory for each region?
I'm not sure I've ever had reason to care about a "region" (whatever
that is!) with MTD hardware.
I suspect that a lot of interesting questions could come up in
the context of enhancing mtd-utils to work with sysfs and bigger
NAND chips. Some might relate to "regions".
- Dave
On 3/31/09, David Brownell <[email protected]> wrote:
>> 2) region_info_user fields? Not really sure how this would work.
>> Maybe a separate subdirectory for each region?
>
> I'm not sure I've ever had reason to care about a "region" (whatever
> that is!) with MTD hardware.
Erase Block Regions. From the CFI spec:
"x specifies the number of regions within the device containing one or
more contiguous Erase Blocks of the same size. For example, a 128KB
device (1Mb) having blocking of 16KB, 8KB, four 2KB, two 16KB, and one
64KB is considered to have 5 Erase Block Regions."
This is fairly common on parallel NOR devices. Probably less so on
huge NAND devices.
> I suspect that a lot of interesting questions could come up in
> the context of enhancing mtd-utils to work with sysfs and bigger
> NAND chips. Some might relate to "regions".
Right, this sysfs requirement raises a number of issues that need to
be fully thought out in order to make sure the new interface is a
suitable replacement for the "INFO" ioctls. For instance:
1) If each region is a subdirectory, are user programs supposed to use
readdir() to count them? Is ioctl(MEMGETREGIONCOUNT) still the
preferred method? Or do we make a new "regioncount" sysfs attribute?
(A somewhat related question is whether MEMGETREGIONCOUNT only exists
because it was impossible to expand the MEMGETINFO struct. After all,
it's just copying another field from the mtd_info struct.)
2) How are user programs expected to access MTD sysfs? Do we
introduce a new libsysfs dependency, or roll our own implementation?
Are there any past examples of ioctls being phased out in favor of
sysfs, particularly in subsystems that are popular on embedded
platforms?
3) What should the mtd-utils changes look like? Do we define
backward-compatibility wrapper functions that try to work the same way
the ioctls used to? New libraries and layers of abstraction? Or
something in the middle?
On Tuesday 31 March 2009, Kevin Cernekee wrote:
> On 3/31/09, David Brownell <[email protected]> wrote:
> >> 2) region_info_user fields? Not really sure how this would work.
> >> Maybe a separate subdirectory for each region?
> >
> > I'm not sure I've ever had reason to care about a "region" (whatever
> > that is!) with MTD hardware.
>
> Erase Block Regions. From the CFI spec:
>
> ...
>
> This is fairly common on parallel NOR devices. Probably less so on
> huge NAND devices.
Oh, that. Right. Few new boards I've seen in the past few
years have NOR; it's mostly NAND nowadays. That gets mixed
up with bootblocks too ... as in, store u-boot parameters in
a 4K erase block (surrounded by u-boot code) instead of some
dedicated 128K block that's almost entirely unused.
> > I suspect that a lot of interesting questions could come up in
> > the context of enhancing mtd-utils to work with sysfs and bigger
> > NAND chips. Some might relate to "regions".
>
> Right, this sysfs requirement raises a number of issues that need to
> be fully thought out in order to make sure the new interface is a
> suitable replacement for the "INFO" ioctls.
Hmm, it's the same as the *current* sysfs model for chardevs, except
that (a) it's there even if chardevs aren't, (b) it supports proper
parent devices, and (c) it adds attributes. So in that sense maybe
that's not the best question to ask.
Maybe you should ask a slightly different question: what's the right
interface to build using sysfs? Certainly let the answers be illuminated
by what current tools can do.
I suspect answering that revised question will lead to a desire for
more driver model update, exposing concerpts beyond just raw MTDs.
> For instance:
>
> 1) If each region is a subdirectory, are user programs supposed to use
> readdir() to count them? Is ioctl(MEMGETREGIONCOUNT) still the
> preferred method? Or do we make a new "regioncount" sysfs attribute?
Model-wise, it might make sense to export chips (potentially
concatentated) with their regions, as distinct from partitions.
That notion doesn't show up all that cleanly in the framework,
thoug it might be good to add it.
> (A somewhat related question is whether MEMGETREGIONCOUNT only exists
> because it was impossible to expand the MEMGETINFO struct. After all,
> it's just copying another field from the mtd_info struct.)
There are folk who are rabidly in favor of the "one value
per attribute" model, but I've never seen that as compelling.
A "regions" attribute, with versioned header (field labels?)
and one line per region, would be a natural model for any
interface that didn't get waylaid by religious fervor.
But ... not my call.
> 2) How are user programs expected to access MTD sysfs? Do we
> introduce a new libsysfs dependency, or roll our own implementation?
> Are there any past examples of ioctls being phased out in favor of
> sysfs, particularly in subsystems that are popular on embedded
> platforms?
I thought the idea was not to use libsysfs...
> 3) What should the mtd-utils changes look like? Do we define
> backward-compatibility wrapper functions that try to work the same way
> the ioctls used to? New libraries and layers of abstraction? Or
> something in the middle?
Up to mtd-utils maintainers. I'd expect some period of backward
compatibility would be required. The "carrot" might be that new
support for 4+ GB chips/partitions might depend on sysfs, while
smaller chips can be supported without it (using existing tools).
Hi David, Kay,
On Mittwoch, 1. April 2009, Kay Sievers wrote:
> On Tue, Mar 31, 2009 at 23:18, David Brownell <[email protected]> wrote:
> > On Thursday 26 March 2009, David Brownell wrote:
> >> From: David Brownell <[email protected]>
> >>
> >> Update driver model support in the MTD framework, so it fits
> >> better into the current udev-based hotplug framework:
> >
> > Hmm, no comments? I had understood there was interest over on
> > the MTD side of things in exposing more information through
> > sysfs, to help avoid the need to add Even More Ioctls as part
> > of support for things like NAND chips with 4KB pages, or which
> > handle more than 4GBytes ...
>
> Please have a look at this. We got asked repeatedly to provide better
> hotplug/udev integration, and the patches, and having the parent
> device properly assigned, would solve some of the problems people run
> into currently.
Without patch:
--------------
$ udevadm info -a -p /sys/block/mtdblock0
[...]
looking at device '/devices/virtual/block/mtdblock0':
KERNEL=="mtdblock0"
SUBSYSTEM=="block"
DRIVER==""
ATTR{range}=="1"
ATTR{removable}=="0"
ATTR{size}=="256"
ATTR{capability}=="10"
ATTR{stat}==" 0 0 0 0 0 0 0 0 0 0 0"
And nearly the same data for the other flash device. No chance to detect if
this one is the NOR or the NAND type...
With the patch:
---------------
$ udevadm info -a -p /sys/block/mtdblock0
[...]
looking at parent device '/devices/platform/physmap-flash.0':
KERNELS=="physmap-flash.0"
SUBSYSTEMS=="platform"
DRIVERS=="physmap-flash"
ATTRS{modalias}=="platform:physmap-flash"
The second flash device is of NAND type and 'udevadm' shows:
$ udevadm info -a -p /sys/block/mtdblock4
[...]
looking at parent device '/devices/platform/mxc_nand.0':
KERNELS=="mxc_nand.0"
SUBSYSTEMS=="platform"
DRIVERS=="mxc_nand"
ATTRS{modalias}=="platform:mxc_nand"
\o/
I will try now to define some udev rules to match for my different flash
memories.
Thank you,
Juergen
--
Pengutronix e.K. | Juergen Beisert |
Linux Solutions for Science and Industry | Phone: +49-8766-939 228 |
Vertretung Sued/Muenchen, Germany | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de/ |
Here is a follow-up patch to apply on top of David Brownell's original
MTD driver model patch at:
http://lists.infradead.org/pipermail/linux-mtd/2009-March/025005.html
Changes:
1) Add more sysfs attributes: mtd_flags, mtd_size, mtd_erasesize,
mtd_writesize, mtd_oobsize, mtd_numeraseregions, mtd_name .
2) Move core_initcall() code into init_mtd(). The original approach
does not work if CONFIG_MTD=m .
3) Add device_unregister() in del_mtd_device() so that devices get
removed from sysfs as each driver is unloaded.
Signed-off-by: Kevin Cernekee <[email protected]>
---
drivers/mtd/mtdcore.c | 107 ++++++++++++++++++++++++++++++++++++++++---------
1 files changed, 88 insertions(+), 19 deletions(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index a88f8bc..19897ba 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -91,9 +91,87 @@ static ssize_t mtd_type_show(struct device *dev,
}
static DEVICE_ATTR(mtd_type, S_IRUGO, mtd_type_show, NULL);
+static ssize_t mtd_flags_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "0x%lx\n", (unsigned long)mtd->flags);
+
+}
+static DEVICE_ATTR(mtd_flags, S_IRUGO, mtd_flags_show, NULL);
+
+static ssize_t mtd_size_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%llu\n",
+ (unsigned long long)mtd->size);
+
+}
+static DEVICE_ATTR(mtd_size, S_IRUGO, mtd_size_show, NULL);
+
+static ssize_t mtd_erasesize_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%lu\n", (unsigned long)mtd->erasesize);
+
+}
+static DEVICE_ATTR(mtd_erasesize, S_IRUGO, mtd_erasesize_show, NULL);
+
+static ssize_t mtd_writesize_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%lu\n", (unsigned long)mtd->writesize);
+
+}
+static DEVICE_ATTR(mtd_writesize, S_IRUGO, mtd_writesize_show, NULL);
+
+static ssize_t mtd_oobsize_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%lu\n", (unsigned long)mtd->oobsize);
+
+}
+static DEVICE_ATTR(mtd_oobsize, S_IRUGO, mtd_oobsize_show, NULL);
+
+static ssize_t mtd_numeraseregions_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%u\n", mtd->numeraseregions);
+
+}
+static DEVICE_ATTR(mtd_numeraseregions, S_IRUGO, mtd_numeraseregions_show,
+ NULL);
+
+static ssize_t mtd_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", mtd->name);
+
+}
+static DEVICE_ATTR(mtd_name, S_IRUGO, mtd_name_show, NULL);
+
static struct attribute *mtd_attrs[] = {
&dev_attr_mtd_type.attr,
- /* FIXME provide a /proc/mtd superset */
+ &dev_attr_mtd_flags.attr,
+ &dev_attr_mtd_size.attr,
+ &dev_attr_mtd_erasesize.attr,
+ &dev_attr_mtd_writesize.attr,
+ &dev_attr_mtd_oobsize.attr,
+ &dev_attr_mtd_numeraseregions.attr,
+ &dev_attr_mtd_name.attr,
NULL,
};
@@ -236,6 +314,8 @@ int del_mtd_device (struct mtd_info *mtd)
} else {
struct mtd_notifier *not;
+ device_unregister(&mtd->dev);
+
/* No need to get a refcount on the module containing
the notifier, since we hold the mtd_table_mutex */
list_for_each_entry(not, &mtd_notifiers, list)
@@ -455,24 +535,6 @@ 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
/*====================================================================*/
@@ -528,6 +590,12 @@ done:
static int __init init_mtd(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);
+ }
if ((proc_mtd = create_proc_entry( "mtd", 0, NULL )))
proc_mtd->read_proc = mtd_read_proc;
return 0;
@@ -537,6 +605,7 @@ static void __exit cleanup_mtd(void)
{
if (proc_mtd)
remove_proc_entry( "mtd", NULL);
+ class_destroy(mtd_class);
}
module_init(init_mtd);
--
1.5.6.3
On Thu, 2009-04-02 at 16:41 -0700, Kevin Cernekee wrote:
> Here is a follow-up patch to apply on top of David Brownell's original
> MTD driver model patch at:
>
> http://lists.infradead.org/pipermail/linux-mtd/2009-March/025005.html
>
> Changes:
>
> 1) Add more sysfs attributes: mtd_flags, mtd_size, mtd_erasesize,
> mtd_writesize, mtd_oobsize, mtd_numeraseregions, mtd_name .
Please, do not all the "mtd_" prefix to the file names. This is
redundant information because we already know this is about
mtd from the path.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
On Fri, 2009-04-03 at 10:03 +0300, Artem Bityutskiy wrote:
> On Thu, 2009-04-02 at 16:41 -0700, Kevin Cernekee wrote:
> > Here is a follow-up patch to apply on top of David Brownell's original
> > MTD driver model patch at:
> >
> > http://lists.infradead.org/pipermail/linux-mtd/2009-March/025005.html
> >
> > Changes:
> >
> > 1) Add more sysfs attributes: mtd_flags, mtd_size, mtd_erasesize,
> > mtd_writesize, mtd_oobsize, mtd_numeraseregions, mtd_name .
>
> Please, do not all the "mtd_" prefix to the file names. This is
> redundant information because we already know this is about
> mtd from the path.
Sorry, s/all/add/.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
On Thu, 2009-03-26 at 00:42 -0700, David Brownell wrote:
>
> @@ -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;
Can you elaborate on that? I think we _do_ want to arrange partitions as
sub-devices of the master, don't we? And I'd rather not change the way
they appear at a later date; I'd prefer them to be that way from the
beginning.
> 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.
> */
Again, can you elaborate?
A lot of devices do just that. Where you have a partition table of some
kind that's actually stored on the flash, that might be the only way to
access it.
I really don't like the way our partitioning works at the moment.
--
dwmw2
On Friday 03 April 2009, David Woodhouse wrote:
> On Thu, 2009-03-26 at 00:42 -0700, David Brownell wrote:
> >
> > @@ -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;
>
> Can you elaborate on that? I think we _do_ want to arrange partitions as
> sub-devices of the master, don't we?
They're part of a tree, and are subdevices of the physical flash
node, so those partitions get nodes like:
.../physical_flashdev
/block/mtdblock*
/mtd/mtd*
/mtd/mtd*ro
If that were "slave->mtd.dev.parent = &master->dev" instead
(you could try it!), not only would most MTD drivers need to
change to register that master->dev ("mtd0" here), but the
tree would look something like (from memory):
.../physical_flashdev
/block/mtdblock*
/mtd/mtd0
/mtd/mtd*
/mtd/mtd*ro
/mtd/mtd0ro
which is at the very least ugly, confusing, counter-intuitive,
and internally inconsistent (why do all the block nodes show
up where you'd expect, but not all the MTD/chardev ones).
> And I'd rather not change the way
> they appear at a later date; I'd prefer them to be that way from the
> beginning.
Agreed. The focus of this patch was getting a model that
would evolve later ... attributes and tool support being
the most apparent issues.
> > 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.
> > */
>
> Again, can you elaborate?
Same point as above ... presuming an A to that long-standing Q.
> A lot of devices do just that. Where you have a partition table of some
> kind that's actually stored on the flash, that might be the only way to
> access it.
I happen never to have come across such a flash layout;
that's presumably what "RedBoot" does (eCOS)?
As I'm sure you're aware, MTDs don't need to be registered
to be used. There's no innate need to "do just that" just
to support RedBoot-style internal partition tables.
As a rule I've seen partitioning be provided by Linux, or
cmdlinepart. Systems using u-boot or proprietary loaders
just "know" where they, their data, and the kernel are to
be found; Linux tables either declare them as read-only
partitions or, less often, only list writable parts of
the flash chip.
One way to model RedBoot tables that might be to have a
partition table node "device_type" that's distinct from
the MTD node type, even if it's still coupled to the
same generic "mtd_info". I'm not sure what to make of
that technique, but it might be useful elsewhere in MTD.
Another way to model RedBoot tables is ... just hide them.
Don't expose an MTD with that data (or "just that data").
Can Linux contine to operate if some tool modifies that
partition data, or must it reboot?
> I really don't like the way our partitioning works at the moment.
Arguably fixing that would be a prerequisite to making
stable driver model support ... which is part of the
reason this was a "patch/RFC". :)
I will confess to trying to avoid opening the can of
worms too far, and suspecting that parts of the MTD
stack I've never needed would expose more issues.
- Dave
Based on: http://lists.infradead.org/pipermail/linux-mtd/2009-March/025005.html
My only change from the previous posting
(http://lists.infradead.org/pipermail/linux-mtd/2009-April/025121.html)
was to remove the "mtd_" prefix on the device attributes.
David's 2/2 patch
(http://lists.infradead.org/pipermail/linux-mtd/2009-March/025011.html)
may still be used as-is.
Signed-off-by: Kevin Cernekee <[email protected]>
---
drivers/mtd/mtd_blkdevs.c | 1 +
drivers/mtd/mtdchar.c | 47 ++----------
drivers/mtd/mtdcore.c | 184 +++++++++++++++++++++++++++++++++++++++++++++
drivers/mtd/mtdpart.c | 9 ++-
include/linux/mtd/mtd.h | 7 ++
5 files changed, 206 insertions(+), 42 deletions(-)
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 4109e0b..a49a9c8 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -286,6 +286,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
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);
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index f478f1f..763d3f0 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -20,33 +20,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
@@ -854,34 +827,26 @@ static const struct file_operations mtd_fops = {
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(¬ifier);
- return 0;
+ return status;
}
static void __exit cleanup_mtdchar(void)
{
- unregister_mtd_user(¬ifier);
- class_destroy(mtd_class);
unregister_chrdev(MTD_CHAR_MAJOR, "mtd");
}
module_init(init_mtdchar);
module_exit(cleanup_mtdchar);
+MODULE_ALIAS_CHARDEV_MAJOR(MTD_CHAR_MAJOR);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("David Woodhouse <[email protected]>");
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 65a7f37..89c1e5d 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -23,6 +23,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);
@@ -33,6 +36,160 @@ 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(type, S_IRUGO, mtd_type_show, NULL);
+
+static ssize_t mtd_flags_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "0x%lx\n", (unsigned long)mtd->flags);
+
+}
+static DEVICE_ATTR(flags, S_IRUGO, mtd_flags_show, NULL);
+
+static ssize_t mtd_size_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%llu\n",
+ (unsigned long long)mtd->size);
+
+}
+static DEVICE_ATTR(size, S_IRUGO, mtd_size_show, NULL);
+
+static ssize_t mtd_erasesize_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%lu\n", (unsigned long)mtd->erasesize);
+
+}
+static DEVICE_ATTR(erasesize, S_IRUGO, mtd_erasesize_show, NULL);
+
+static ssize_t mtd_writesize_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%lu\n", (unsigned long)mtd->writesize);
+
+}
+static DEVICE_ATTR(writesize, S_IRUGO, mtd_writesize_show, NULL);
+
+static ssize_t mtd_oobsize_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%lu\n", (unsigned long)mtd->oobsize);
+
+}
+static DEVICE_ATTR(oobsize, S_IRUGO, mtd_oobsize_show, NULL);
+
+static ssize_t mtd_numeraseregions_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%u\n", mtd->numeraseregions);
+
+}
+static DEVICE_ATTR(numeraseregions, S_IRUGO, mtd_numeraseregions_show,
+ NULL);
+
+static ssize_t mtd_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct mtd_info *mtd = dev_to_mtd(dev);
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", mtd->name);
+
+}
+static DEVICE_ATTR(name, S_IRUGO, mtd_name_show, NULL);
+
+static struct attribute *mtd_attrs[] = {
+ &dev_attr_type.attr,
+ &dev_attr_flags.attr,
+ &dev_attr_size.attr,
+ &dev_attr_erasesize.attr,
+ &dev_attr_writesize.attr,
+ &dev_attr_oobsize.attr,
+ &dev_attr_numeraseregions.attr,
+ &dev_attr_name.attr,
+ 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
@@ -41,6 +198,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)
@@ -95,6 +253,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 */
@@ -139,6 +314,8 @@ int del_mtd_device (struct mtd_info *mtd)
} else {
struct mtd_notifier *not;
+ device_unregister(&mtd->dev);
+
/* No need to get a refcount on the module containing
the notifier, since we hold the mtd_table_mutex */
list_for_each_entry(not, &mtd_notifiers, list)
@@ -413,6 +590,12 @@ done:
static int __init init_mtd(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);
+ }
if ((proc_mtd = create_proc_entry( "mtd", 0, NULL )))
proc_mtd->read_proc = mtd_read_proc;
return 0;
@@ -422,6 +605,7 @@ static void __exit cleanup_mtd(void)
{
if (proc_mtd)
remove_proc_entry( "mtd", NULL);
+ class_destroy(mtd_class);
}
module_init(init_mtd);
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 06d5b9d..02ce38f 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -356,6 +356,11 @@ static struct mtd_part *add_one_partition(struct
mtd_info *master,
slave->mtd.owner = master->owner;
slave->mtd.backing_dev_info = master->backing_dev_info;
+ /* 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;
@@ -508,7 +513,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,
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 0c079fd..5675b63 100644
--- 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>
@@ -237,6 +238,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
@@ -247,6 +249,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)
--
1.5.6.3
On Fri, 2009-04-03 at 13:00 -0700, Kevin Cernekee wrote:
> Based on: http://lists.infradead.org/pipermail/linux-mtd/2009-March/025005.html
>
> My only change from the previous posting
> (http://lists.infradead.org/pipermail/linux-mtd/2009-April/025121.html)
> was to remove the "mtd_" prefix on the device attributes.
>
> David's 2/2 patch
> (http://lists.infradead.org/pipermail/linux-mtd/2009-March/025011.html)
> may still be used as-is.
>
> Signed-off-by: Kevin Cernekee <[email protected]>
Thanks, this looks like a very good start in the direction we need to
go. I've applied David's patches as well as the changes from this one,
and hooked it up for the CAFÉ NAND controller too.
Since the callers are passing a struct mtd_info * into nand_scan(), it
doesn't seem necessary to pass the device in too; they can just set it
for themselves. Passing it in to the NOR chip probe routines might make
sense though.
I'm not worried about a flag day for _internal_ stuff -- for 2.6.31 I
think I'm going to add a WARN_ON(&mtd->dev.parent) into the core code.
I really do think I want this to avoid the need for 64-bit ioctls
(except maybe MEMERASE64).
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
On 4/4/09, David Woodhouse <[email protected]> wrote:
> I really do think I want this to avoid the need for 64-bit ioctls
> (except maybe MEMERASE64).
So, after adding the sysfs patches, we have two remaining sets of MTD
operations that are limited to 32-bit offsets:
The first set is required to support >4GiB NAND devices: MEMERASE,
MEMREADOOB, and MEMWRITEOOB.
The second set might not make sense at all for most NAND devices:
MEMLOCK, MEMUNLOCK, and MEMGETREGIONINFO. Maybe it would be nice to
extend them for consistency's sake, but it is possible that they will
never be needed on any >4GiB flash chip.
How would you like to handle these? Any thoughts on what the
interfaces (sysfs or otherwise) might look like?
Thanks.
On Saturday 04 April 2009, David Woodhouse wrote:
> think I'm going to add a WARN_ON(&mtd->dev.parent) into the core code.
I hope you mean WARN_ON(!mtd->dev.parent) ... :)
On Sat, 2009-04-04 at 09:20 -0700, David Brownell wrote:
> On Saturday 04 April 2009, David Woodhouse wrote:
> > think I'm going to add a WARN_ON(&mtd->dev.parent) into the core code.
>
> I hope you mean WARN_ON(!mtd->dev.parent) ... :)
If I didn't, I'd probably change my mind fairly quickly... :)
--
dwmw2
On Friday 03 April 2009, Kevin Cernekee wrote:
> @@ -413,6 +590,12 @@ done:
>
> ?static int __init init_mtd(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);
> +???????}
The reason I had the class creation code in its own initcall
is to ensure that it's there even when procfs isn't. This
init_mtd() stuff is only there if procfs is configured.
So I don't much like this part of Kevin's change ... unless
MTD becomes dependent on procfs. If there's an issue with
CONFIG_MTD=m then there's a better fix than this.
> ????????if ((proc_mtd = create_proc_entry( "mtd", 0, NULL )))
> ????????????????proc_mtd->read_proc = mtd_read_proc;
> ????????return 0;
> @@ -422,6 +605,7 @@ static void __exit cleanup_mtd(void)
> ?{
> ? ? ? ? ?if (proc_mtd)
> ????????????????remove_proc_entry( "mtd", NULL);
> +???????class_destroy(mtd_class);
> ?}
>
> ?module_init(init_mtd);
On Thu, Apr 02, 2009 at 04:41:47PM -0700, Kevin Cernekee wrote:
> Here is a follow-up patch to apply on top of David Brownell's original
> MTD driver model patch at:
>
> http://lists.infradead.org/pipermail/linux-mtd/2009-March/025005.html
>
> Changes:
>
> 1) Add more sysfs attributes: mtd_flags, mtd_size, mtd_erasesize,
> mtd_writesize, mtd_oobsize, mtd_numeraseregions, mtd_name .
When adding new sysfs attributes, please add the proper
Documentation/ABI/ information as well.
thanks,
greg k-h