Message ID | 20160827194437.GA17566@makrotopia.org |
---|---|
State | Changes Requested |
Delegated to: | John Crispin |
Headers | show |
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()) >
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())
Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- init/do_mounts.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 7 deletions(-)