From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Brownell Date: Tue, 24 Mar 2009 21:14:35 +0000 Subject: Re: udev problem (and fix) for /dev/mtdblock* Message-Id: <200903241414.35404.david-b@pacbell.net> List-Id: References: <200903181244.23025.david-b@pacbell.net> <200903231149.15241.david-b@pacbell.net> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Kay Sievers Cc: Linux MTD , linux-hotplug@vger.kernel.org, md@linux.it 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: > >> =A0 http://git.kernel.org/?p=3Dlinux/kernel/git/kay/patches.git;a=3Dbl= ob;f=3Dmtd-parent-dev.patch;hb=3DHEAD > > > > Looks plausible ... and I may have actually seen a system where > > that "problem" matters. =A0(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*. =A0I think the issue > > might be lack of mtdpart support. >=20 > 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. =3D=3D=3D=3D=3D=3D 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=20 drivers/mtd/devices/mtd_dataflash.c | 2=20 drivers/mtd/maps/omap_nor.c | 2=20 drivers/mtd/maps/physmap.c | 15 ++-- drivers/mtd/maps/plat-ram.c | 1=20 drivers/mtd/mtd_blkdevs.c | 1=20 drivers/mtd/mtdchar.c | 46 +------------ drivers/mtd/mtdcore.c | 115 +++++++++++++++++++++++++++++++= +++ drivers/mtd/mtdpart.c | 9 ++ drivers/mtd/nand/davinci_nand.c | 2=20 drivers/mtd/nand/mxc_nand.c | 1=20 drivers/mtd/onenand/omap2.c | 2=20 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 =3D info->sector_size; } =20 + flash->mtd.dev.parent =3D &spi->dev; + dev_info(&spi->dev, "%s (%lld Kbytes)\n", info->name, (long long)flash->mtd.size >> 10); =20 --- 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 =3D dataflash_write; device->priv =3D priv; =20 + device->dev.parent =3D &spi->dev; + if (revision >=3D 'c') otp_tag =3D otp_setup(device, revision); =20 --- 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 =3D THIS_MODULE; =20 + info->mtd->dev.parent =3D &pdev->dev; + #ifdef CONFIG_MTD_PARTITIONS err =3D 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 =3D THIS_MODULE; + info->mtd[i]->dev.parent =3D &dev->dev; } =20 if (devices_found =3D 1) { @@ -171,16 +172,16 @@ static int physmap_flash_probe(struct pl #ifdef CONFIG_MTD_PARTITIONS err =3D 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 =3D err; - return 0; + else { + dev_notice(&dev->dev, "Using static partition information\n"); + info->nr_parts =3D physmap_data->nr_parts; + info->parts =3D physmap_data->parts; } =20 - 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 } =20 info->mtd->owner =3D THIS_MODULE; + info->mtd->dev.parent =3D &pdev->dev; =20 platram_setrw(info, PLATRAM_RW); =20 --- 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 =3D new; new->blkcore_priv =3D gd; gd->queue =3D tr->blkcore_priv->rq; + gd->driverfs_dev =3D new->mtd->dev.parent; =20 if (new->readonly) set_disk_ro(gd, 1); --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -19,33 +19,6 @@ =20 #include =20 -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 =3D { - .add =3D mtd_notify_add, - .remove =3D mtd_notify_remove, -}; =20 /* * Data structure to hold the pointer to the mtd device as well @@ -793,28 +766,19 @@ static const struct file_operations mtd_ =20 static int __init init_mtdchar(void) { - if (register_chrdev(MTD_CHAR_MAJOR, "mtd", &mtd_fops)) { + int status; + + status =3D 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; } =20 - mtd_class =3D 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; } =20 static void __exit cleanup_mtdchar(void) { - unregister_mtd_user(¬ifier); - class_destroy(mtd_class); unregister_chrdev(MTD_CHAR_MAJOR, "mtd"); } =20 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -22,6 +22,9 @@ =20 #include "mtdcore.h" =20 + +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); =20 static LIST_HEAD(mtd_notifiers); =20 + +#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 =3D 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 =3D dev_to_mtd(dev); + char *type; + + switch (mtd->type) { + case MTD_ABSENT: + type =3D "absent"; + break; + case MTD_RAM: + type =3D "ram"; + break; + case MTD_ROM: + type =3D "rom"; + break; + case MTD_NORFLASH: + type =3D "nor"; + break; + case MTD_NANDFLASH: + type =3D "nand"; + break; + case MTD_DATAFLASH: + type =3D "dataflash"; + break; + case MTD_UBIVOLUME: + type =3D "ubi"; + break; + default: + type =3D "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[] =3D { + &dev_attr_mtd_type.attr, + /* FIXME provide a /proc/mtd superset */ + NULL, +}; + +struct attribute_group mtd_group =3D { + .attrs =3D mtd_attrs, +}; + +struct attribute_group *mtd_groups[] =3D { + &mtd_group, + NULL, +}; + +static struct device_type mtd_devtype =3D { + .name =3D "mtd", + .groups =3D mtd_groups, + .release =3D 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. */ =20 int add_mtd_device(struct mtd_info *mtd) @@ -80,6 +160,23 @@ int add_mtd_device(struct mtd_info *mtd) mtd->name); } =20 + /* Caller should have set dev.parent to match the + * physical device. + */ + mtd->dev.type =3D &mtd_devtype; + mtd->dev.class =3D mtd_class; + mtd->dev.devt =3D MTD_DEVT(i); + dev_set_name(&mtd->dev, "mtd%d", i); + if (device_register(&mtd->dev) !=3D 0) { + mtd_table[i] =3D 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); =20 +static int __init mtd_setup(void) +{ + mtd_class =3D 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 =20 /*=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D*/ --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -343,6 +343,11 @@ static struct mtd_part *add_one_partitio slave->mtd.name =3D part->name; slave->mtd.owner =3D master->owner; =20 + /* 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 =3D master->dev.parent; + slave->mtd.read =3D part_read; slave->mtd.write =3D part_write; =20 @@ -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. */ =20 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 =3D dev_name(&pdev->dev); info->mtd.owner =3D THIS_MODULE; =20 + info->mtd.dev.parent =3D &pdev->dev; + info->chip.IO_ADDR_R =3D vaddr; info->chip.IO_ADDR_W =3D vaddr; info->chip.chip_delay =3D 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 =3D &host->mtd; mtd->priv =3D this; mtd->owner =3D THIS_MODULE; + mtd->dev.parent =3D &pdev->dev; =20 /* 50 us command delay time */ this->chip_delay =3D 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 =3D &c->onenand; c->mtd.owner =3D THIS_MODULE; =20 + c->mtd.dev.parent =3D &pdev->dev; + if (c->dma_channel >=3D 0) { struct onenand_chip *this =3D &c->onenand; =20 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -11,6 +11,7 @@ #include #include #include +#include =20 #include #include @@ -223,6 +224,7 @@ struct mtd_info { void *priv; =20 struct module *owner; + struct device dev; int usecount; =20 /* 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); }; =20 +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)