Re: [PATCH 3/3] init: auto-create ubiblock device for non-UBIFS rootfs on UBI

[Daniel Golle <daniel@makrotopia.org>]

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())
>