* [PATCH 3/3] init: auto-create ubiblock device for non-UBIFS rootfs on UBI @ 2016-08-27 19:44 Daniel Golle 2016-08-28 16:54 ` Boris Brezillon 0 siblings, 1 reply; 3+ messages in thread From: Daniel Golle @ 2016-08-27 19:44 UTC (permalink / raw) To: linux-mtd Cc: Richard Weinberger, Ralph Sennhauser, Zoltan HERPAI, Hauke Mehrtens, lede-dev, openwrt-devel Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- init/do_mounts.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 7 deletions(-) diff --git a/init/do_mounts.c b/init/do_mounts.c index dea5de9..485df12 100644 --- a/init/do_mounts.c +++ b/init/do_mounts.c @@ -28,6 +28,7 @@ #include <linux/slab.h> #include <linux/ramfs.h> #include <linux/shmem_fs.h> +#include <linux/mtd/ubi.h> #include <linux/nfs_fs.h> #include <linux/nfs_fs_sb.h> @@ -179,6 +180,47 @@ done: } #endif +#if defined(CONFIG_MTD_UBI_BLOCK) && !defined(CONFIG_MTD_UBI_MODULE) +#define UBIFS_NODE_MAGIC 0x06101831 +static inline int ubi_vol_is_ubifs(struct ubi_volume_desc *desc) +{ + int ret; + uint32_t magic_of, magic; + ret = ubi_read(desc, 0, (char *)&magic_of, 0, 4); + if (ret) + return 0; + magic = le32_to_cpu(magic_of); + return magic == UBIFS_NODE_MAGIC; +} + +static void ubiblock_create_rootdev(char *name) +{ + int ret, is_ubifs; + struct ubi_volume_desc *desc; + struct ubi_volume_info vi; + dev_t bdev; + + desc = ubi_open_volume_str(name, UBI_READONLY); + if (IS_ERR(desc)) + return; + + ubi_get_volume_info(desc, &vi); + + is_ubifs = ubi_vol_is_ubifs(desc); + ubi_close_volume(desc); + + if (is_ubifs) + return; + + ret = ubiblock_create_dev(&vi, &bdev); + if (!ret) { + pr_notice("ubiblock%u_%u: '%s' set to be root filesystem\n", + vi.ubi_num, vi.vol_id, vi.name); + ROOT_DEV = bdev; + } +} +#endif + /* * Convert a name into device number. We accept the following variants: * @@ -569,14 +611,20 @@ void __init prepare_namespace(void) if (saved_root_name[0]) { root_device_name = saved_root_name; - if (!strncmp(root_device_name, "mtd", 3) || - !strncmp(root_device_name, "ubi", 3)) { - mount_block_root(root_device_name, root_mountflags); - goto out; +#if defined(CONFIG_MTD_UBI_BLOCK) && !defined(CONFIG_MTD_UBI_MODULE) + if (!strncmp(root_device_name, "ubi", 3)) + ubiblock_create_rootdev(root_device_name); +#endif + if (ROOT_DEV == 0) { + if (!strncmp(root_device_name, "mtd", 3) || + !strncmp(root_device_name, "ubi", 3)) { + mount_block_root(root_device_name, root_mountflags); + goto out; + } + ROOT_DEV = name_to_dev_t(root_device_name); + if (strncmp(root_device_name, "/dev/", 5) == 0) + root_device_name += 5; } - ROOT_DEV = name_to_dev_t(root_device_name); - if (strncmp(root_device_name, "/dev/", 5) == 0) - root_device_name += 5; } if (initrd_load()) -- 2.9.3 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 3/3] init: auto-create ubiblock device for non-UBIFS rootfs on UBI 2016-08-27 19:44 [PATCH 3/3] init: auto-create ubiblock device for non-UBIFS rootfs on UBI Daniel Golle @ 2016-08-28 16:54 ` Boris Brezillon 2016-08-28 21:22 ` Daniel Golle 0 siblings, 1 reply; 3+ messages in thread From: Boris Brezillon @ 2016-08-28 16:54 UTC (permalink / raw) To: Daniel Golle Cc: linux-mtd, Richard Weinberger, lede-dev, Zoltan HERPAI, Hauke Mehrtens, Ralph Sennhauser, openwrt-devel On Sat, 27 Aug 2016 21:44:46 +0200 Daniel Golle <daniel@makrotopia.org> wrote: > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > init/do_mounts.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 55 insertions(+), 7 deletions(-) > > diff --git a/init/do_mounts.c b/init/do_mounts.c > index dea5de9..485df12 100644 > --- a/init/do_mounts.c > +++ b/init/do_mounts.c > @@ -28,6 +28,7 @@ > #include <linux/slab.h> > #include <linux/ramfs.h> > #include <linux/shmem_fs.h> > +#include <linux/mtd/ubi.h> Really??? You include UBI stuff in generic kernel code? Come on. Linux is defining clear interfaces to be implemented by drivers/FS for a good reason: the core code should be implementation agnostic, and you're just breaking this rule. > > #include <linux/nfs_fs.h> > #include <linux/nfs_fs_sb.h> > @@ -179,6 +180,47 @@ done: > } > #endif > > +#if defined(CONFIG_MTD_UBI_BLOCK) && !defined(CONFIG_MTD_UBI_MODULE) > +#define UBIFS_NODE_MAGIC 0x06101831 > +static inline int ubi_vol_is_ubifs(struct ubi_volume_desc *desc) > +{ > + int ret; > + uint32_t magic_of, magic; > + ret = ubi_read(desc, 0, (char *)&magic_of, 0, 4); > + if (ret) > + return 0; > + magic = le32_to_cpu(magic_of); > + return magic == UBIFS_NODE_MAGIC; > +} This is even worst. Now your parsing data within a specific volume to determine if the volume is likely to contain a UBIFS FS. And all that is done in core kernel code. > + > +static void ubiblock_create_rootdev(char *name) > +{ > + int ret, is_ubifs; > + struct ubi_volume_desc *desc; > + struct ubi_volume_info vi; > + dev_t bdev; > + > + desc = ubi_open_volume_str(name, UBI_READONLY); > + if (IS_ERR(desc)) > + return; > + > + ubi_get_volume_info(desc, &vi); > + > + is_ubifs = ubi_vol_is_ubifs(desc); > + ubi_close_volume(desc); > + > + if (is_ubifs) > + return; > + > + ret = ubiblock_create_dev(&vi, &bdev); > + if (!ret) { > + pr_notice("ubiblock%u_%u: '%s' set to be root filesystem\n", > + vi.ubi_num, vi.vol_id, vi.name); > + ROOT_DEV = bdev; > + } > +} And it continues here. Now you're automatically creating a ubiblock device based on the UBIFS detection, and again, this is in core kernel code. > +#endif > + > /* > * Convert a name into device number. We accept the following variants: > * > @@ -569,14 +611,20 @@ void __init prepare_namespace(void) > > if (saved_root_name[0]) { > root_device_name = saved_root_name; > - if (!strncmp(root_device_name, "mtd", 3) || > - !strncmp(root_device_name, "ubi", 3)) { > - mount_block_root(root_device_name, root_mountflags); > - goto out; > +#if defined(CONFIG_MTD_UBI_BLOCK) && !defined(CONFIG_MTD_UBI_MODULE) > + if (!strncmp(root_device_name, "ubi", 3)) > + ubiblock_create_rootdev(root_device_name); > +#endif > + if (ROOT_DEV == 0) { > + if (!strncmp(root_device_name, "mtd", 3) || > + !strncmp(root_device_name, "ubi", 3)) { > + mount_block_root(root_device_name, root_mountflags); > + goto out; > + } > + ROOT_DEV = name_to_dev_t(root_device_name); > + if (strncmp(root_device_name, "/dev/", 5) == 0) > + root_device_name += 5; > } > - ROOT_DEV = name_to_dev_t(root_device_name); > - if (strncmp(root_device_name, "/dev/", 5) == 0) > - root_device_name += 5; And the last piece: you're making use of all the hacks you've introduced earlier to create your blockdevice and pass it to the 'mount blockdev FS' logic. I hope you understand why this patch is not acceptable. > } > > if (initrd_load()) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 3/3] init: auto-create ubiblock device for non-UBIFS rootfs on UBI 2016-08-28 16:54 ` Boris Brezillon @ 2016-08-28 21:22 ` Daniel Golle 0 siblings, 0 replies; 3+ messages in thread From: Daniel Golle @ 2016-08-28 21:22 UTC (permalink / raw) To: Boris Brezillon Cc: linux-mtd, Richard Weinberger, lede-dev, Zoltan HERPAI, Hauke Mehrtens, Ralph Sennhauser, openwrt-devel Hi Boris, thanks for the review! This is more helpful and more of the type of feedback I was hoping for. On Sun, Aug 28, 2016 at 06:54:15PM +0200, Boris Brezillon wrote: > On Sat, 27 Aug 2016 21:44:46 +0200 > Daniel Golle <daniel@makrotopia.org> wrote: > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > init/do_mounts.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 55 insertions(+), 7 deletions(-) > > > > diff --git a/init/do_mounts.c b/init/do_mounts.c > > index dea5de9..485df12 100644 > > --- a/init/do_mounts.c > > +++ b/init/do_mounts.c > > @@ -28,6 +28,7 @@ > > #include <linux/slab.h> > > #include <linux/ramfs.h> > > #include <linux/shmem_fs.h> > > +#include <linux/mtd/ubi.h> > > Really??? You include UBI stuff in generic kernel code? Come on. Linux > is defining clear interfaces to be implemented by drivers/FS for a good > reason: the core code should be implementation agnostic, and you're > just breaking this rule. The whole thing could be moved to a new file (similar as done for other things for specific subsystems, like do_mounts_md.c). > > > > > #include <linux/nfs_fs.h> > > #include <linux/nfs_fs_sb.h> > > @@ -179,6 +180,47 @@ done: > > } > > #endif > > > > +#if defined(CONFIG_MTD_UBI_BLOCK) && !defined(CONFIG_MTD_UBI_MODULE) > > +#define UBIFS_NODE_MAGIC 0x06101831 > > +static inline int ubi_vol_is_ubifs(struct ubi_volume_desc *desc) > > +{ > > + int ret; > > + uint32_t magic_of, magic; > > + ret = ubi_read(desc, 0, (char *)&magic_of, 0, 4); > > + if (ret) > > + return 0; > > + magic = le32_to_cpu(magic_of); > > + return magic == UBIFS_NODE_MAGIC; > > +} > > This is even worst. Now your parsing data within a specific volume to > determine if the volume is likely to contain a UBIFS FS. And all that > is done in core kernel code. I was unsure, however, maybe ubiblock should generally refuse to create a ubiblock device if an UBIFS signature is found...? In that case, this function and the logic using it below could be moved to driver/mtd/ubi/block.c > > > + > > +static void ubiblock_create_rootdev(char *name) > > +{ > > + int ret, is_ubifs; > > + struct ubi_volume_desc *desc; > > + struct ubi_volume_info vi; > > + dev_t bdev; > > + > > + desc = ubi_open_volume_str(name, UBI_READONLY); > > + if (IS_ERR(desc)) > > + return; > > + > > + ubi_get_volume_info(desc, &vi); > > + > > + is_ubifs = ubi_vol_is_ubifs(desc); > > + ubi_close_volume(desc); > > + > > + if (is_ubifs) > > + return; > > + > > + ret = ubiblock_create_dev(&vi, &bdev); > > + if (!ret) { > > + pr_notice("ubiblock%u_%u: '%s' set to be root filesystem\n", > > + vi.ubi_num, vi.vol_id, vi.name); > > + ROOT_DEV = bdev; > > + } > > +} > > And it continues here. Now you're automatically creating a ubiblock > device based on the UBIFS detection, and again, this is in core kernel > code. This, again, could go into a file of it's own like init/do_mounts_ubiblock.c which is just how it's done for ramdisk and mdraid stuff. > > > +#endif > > + > > /* > > * Convert a name into device number. We accept the following variants: > > * > > @@ -569,14 +611,20 @@ void __init prepare_namespace(void) > > > > if (saved_root_name[0]) { > > root_device_name = saved_root_name; > > - if (!strncmp(root_device_name, "mtd", 3) || > > - !strncmp(root_device_name, "ubi", 3)) { > > - mount_block_root(root_device_name, root_mountflags); > > - goto out; > > +#if defined(CONFIG_MTD_UBI_BLOCK) && !defined(CONFIG_MTD_UBI_MODULE) > > + if (!strncmp(root_device_name, "ubi", 3)) > > + ubiblock_create_rootdev(root_device_name); > > +#endif > > + if (ROOT_DEV == 0) { > > + if (!strncmp(root_device_name, "mtd", 3) || > > + !strncmp(root_device_name, "ubi", 3)) { > > + mount_block_root(root_device_name, root_mountflags); > > + goto out; > > + } > > + ROOT_DEV = name_to_dev_t(root_device_name); > > + if (strncmp(root_device_name, "/dev/", 5) == 0) > > + root_device_name += 5; > > } > > - ROOT_DEV = name_to_dev_t(root_device_name); > > - if (strncmp(root_device_name, "/dev/", 5) == 0) > > - root_device_name += 5; > > And the last piece: you're making use of all the hacks you've > introduced earlier to create your blockdevice and pass it to the 'mount > blockdev FS' logic. This is what is done for all sorts of block devices in that function... What's wrong with that approach? > > I hope you understand why this patch is not acceptable. I surely do, it was sent in the intention to start a discussion and collect comments, not with the intention to have it merged at this stage. I thus really appreciate your detailed review, though I'm aware that you are opposed to the whole idea of automagically creating the ubiblock device and thereby allowing to unify the rootfs= cmdline syntax to be agnostic to the filesystem-type used. Well, thank you anyway. Cheers Daniel > > > } > > > > if (initrd_load()) > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-08-28 21:22 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-27 19:44 [PATCH 3/3] init: auto-create ubiblock device for non-UBIFS rootfs on UBI Daniel Golle 2016-08-28 16:54 ` Boris Brezillon 2016-08-28 21:22 ` Daniel Golle
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox