diff mbox series

[v4,1/5] cramfs: direct memory access support

Message ID 20170927233224.31676-2-nicolas.pitre@linaro.org
State Superseded
Headers show
Series cramfs refresh for embedded usage | expand

Commit Message

Nicolas Pitre Sept. 27, 2017, 11:32 p.m. UTC
Small embedded systems typically execute the kernel code in place (XIP)
directly from flash to save on precious RAM usage. This adds the ability
to consume filesystem data directly from flash to the cramfs filesystem
as well. Cramfs is particularly well suited to this feature as it is
very simple and its RAM usage is already very low, and with this feature
it is possible to use it with no block device support and even lower RAM
usage.

This patch was inspired by a similar patch from Shane Nay dated 17 years
ago that used to be very popular in embedded circles but never made it
into mainline. This is a cleaned-up implementation that uses far fewer
memory address at run time when both methods are configured in. In the
context of small IoT deployments, this functionality has become relevant
and useful again.

To distinguish between both access types, the cramfs_physmem filesystem
type must be specified when using a memory accessible cramfs image, and
the physaddr argument must provide the actual filesystem image's physical
memory location.

Signed-off-by: Nicolas Pitre <nico@linaro.org>

Tested-by: Chris Brandt <chris.brandt@renesas.com>

---
 fs/cramfs/Kconfig |  29 +++++-
 fs/cramfs/inode.c | 264 +++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 241 insertions(+), 52 deletions(-)

-- 
2.9.5

Comments

Nicolas Pitre Oct. 1, 2017, 10:27 p.m. UTC | #1
On Sun, 1 Oct 2017, Christoph Hellwig wrote:

> On Wed, Sep 27, 2017 at 07:32:20PM -0400, Nicolas Pitre wrote:

> > To distinguish between both access types, the cramfs_physmem filesystem

> > type must be specified when using a memory accessible cramfs image, and

> > the physaddr argument must provide the actual filesystem image's physical

> > memory location.

> 

> Sorry, but this still is a complete no-go.  A physical address is not a

> proper interface.  You still need to have some interface for your NOR nand

> or DRAM.  - usually that would be a mtd driver, but if you have a good

> reason why that's not suitable for you (and please explain it well)

> we'll need a little OF or similar layer to bind a thin driver.


The primary use case for this is to run Linux on a small microcontroller 
with some amount of RAM and ROM on chip. And this is not theoretical -- 
I already have it running here. The ROM is some kind of flash that 
appears in the direct memory address space and requires no access layer 
what so ever given it is meant to execute code from it. The flash is 
programmed with an external programmer through some debug port. It can't 
be programmed from the microcontroller itself, not even probed, as that 
would make the running code unavailable (unless the probe code is copied 
elsewhere but what would be the point?). Persistent state is typically 
kept in NVRAM or external flash, not in _that_ flash.

The MTD subsystem provides a lot of features and flexibility, but almost 
none of it would be usable here and constitutes only a useless kernel 
size increase.

The kernel itself runs XIP from that ROM. It has to be linked for the 
exact address where it is flashed. The link address is therefore not a 
variable that can be changed at run time. It is the same for the 
filesystem image: it is related to the way things are laid out in ROM, 
and typically depends on the actual size of the kernel when ROM is 
tight.

You fundamentally need to know the address of the kernel _and_ the 
address of the fs image. Those addresses are properties of your kernel 
config. So having to specify one in Kconfig and bury the other in DT 
doesn't make sense to me as this is just an extra file to edit and 
compile, and an extra binary to write into flash, for something that 
isn't a property of the hardware. The bootloader and DT should remain 
stable as much as possible with invariant data.

If you prefer, the physical address could be specified with a Kconfig 
symbol just like the kernel link address. Personally I think it is best 
to keep it along with the other root mount args. But going all the way 
with a dynamic driver binding interface and a dummy intermediate name is 
like using a sledge hammer to kill an ant: it will work of course, but 
given the context it is prone to errors due to the added manipulations 
mentioned previously ... and a tad overkill.


> > Signed-off-by: Nicolas Pitre <nico@linaro.org>

> > Tested-by: Chris Brandt <chris.brandt@renesas.com>

> > ---

> >  fs/cramfs/Kconfig |  29 +++++-

> >  fs/cramfs/inode.c | 264 +++++++++++++++++++++++++++++++++++++++++++-----------

> >  2 files changed, 241 insertions(+), 52 deletions(-)

> > 

> > diff --git a/fs/cramfs/Kconfig b/fs/cramfs/Kconfig

> > index 11b29d491b..5b4e0b7e13 100644

> > --- a/fs/cramfs/Kconfig

> > +++ b/fs/cramfs/Kconfig

> > @@ -1,6 +1,5 @@

> >  config CRAMFS

> >  	tristate "Compressed ROM file system support (cramfs) (OBSOLETE)"

> > -	depends on BLOCK

> >  	select ZLIB_INFLATE

> >  	help

> >  	  Saying Y here includes support for CramFs (Compressed ROM File

> > @@ -20,3 +19,31 @@ config CRAMFS

> >  	  in terms of performance and features.

> >  

> >  	  If unsure, say N.

> > +

> > +config CRAMFS_BLOCKDEV

> > +	bool "Support CramFs image over a regular block device" if EXPERT

> > +	depends on CRAMFS && BLOCK

> > +	default y

> > +	help

> > +	  This option allows the CramFs driver to load data from a regular

> > +	  block device such a disk partition or a ramdisk.

> > +

> > +config CRAMFS_PHYSMEM

> > +	bool "Support CramFs image directly mapped in physical memory"

> > +	depends on CRAMFS

> > +	default y if !CRAMFS_BLOCKDEV

> > +	help

> > +	  This option allows the CramFs driver to load data directly from

> > +	  a linear adressed memory range (usually non volatile memory

> > +	  like flash) instead of going through the block device layer.

> > +	  This saves some memory since no intermediate buffering is

> > +	  necessary.

> > +

> > +	  The filesystem type for this feature is "cramfs_physmem".

> > +	  The location of the CramFs image in memory is board

> > +	  dependent. Therefore, if you say Y, you must know the proper

> > +	  physical address where to store the CramFs image and specify

> > +	  it using the physaddr=0x******** mount option (for example:

> > +	  "mount -t cramfs_physmem -o physaddr=0x100000 none /mnt").

> > +

> > +	  If unsure, say N.

> > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c

> > index 7919967488..19f464a214 100644

> > --- a/fs/cramfs/inode.c

> > +++ b/fs/cramfs/inode.c

> > @@ -24,6 +24,7 @@

> >  #include <linux/mutex.h>

> >  #include <uapi/linux/cramfs_fs.h>

> >  #include <linux/uaccess.h>

> > +#include <linux/io.h>

> >  

> >  #include "internal.h"

> >  

> > @@ -36,6 +37,8 @@ struct cramfs_sb_info {

> >  	unsigned long blocks;

> >  	unsigned long files;

> >  	unsigned long flags;

> > +	void *linear_virt_addr;

> > +	phys_addr_t linear_phys_addr;

> >  };

> >  

> >  static inline struct cramfs_sb_info *CRAMFS_SB(struct super_block *sb)

> > @@ -140,6 +143,9 @@ static struct inode *get_cramfs_inode(struct super_block *sb,

> >   * BLKS_PER_BUF*PAGE_SIZE, so that the caller doesn't need to

> >   * worry about end-of-buffer issues even when decompressing a full

> >   * page cache.

> > + *

> > + * Note: This is all optimized away at compile time when

> > + *       CONFIG_CRAMFS_BLOCKDEV=n.

> >   */

> >  #define READ_BUFFERS (2)

> >  /* NEXT_BUFFER(): Loop over [0..(READ_BUFFERS-1)]. */

> > @@ -160,10 +166,10 @@ static struct super_block *buffer_dev[READ_BUFFERS];

> >  static int next_buffer;

> >  

> >  /*

> > - * Returns a pointer to a buffer containing at least LEN bytes of

> > - * filesystem starting at byte offset OFFSET into the filesystem.

> > + * Populate our block cache and return a pointer from it.

> >   */

> > -static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned int len)

> > +static void *cramfs_blkdev_read(struct super_block *sb, unsigned int offset,

> > +				unsigned int len)

> >  {

> >  	struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;

> >  	struct page *pages[BLKS_PER_BUF];

> > @@ -239,7 +245,39 @@ static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned i

> >  	return read_buffers[buffer] + offset;

> >  }

> >  

> > -static void cramfs_kill_sb(struct super_block *sb)

> > +/*

> > + * Return a pointer to the linearly addressed cramfs image in memory.

> > + */

> > +static void *cramfs_direct_read(struct super_block *sb, unsigned int offset,

> > +				unsigned int len)

> > +{

> > +	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);

> > +

> > +	if (!len)

> > +		return NULL;

> > +	if (len > sbi->size || offset > sbi->size - len)

> > +	       return page_address(ZERO_PAGE(0));

> > +	return sbi->linear_virt_addr + offset;

> > +}

> > +

> > +/*

> > + * Returns a pointer to a buffer containing at least LEN bytes of

> > + * filesystem starting at byte offset OFFSET into the filesystem.

> > + */

> > +static void *cramfs_read(struct super_block *sb, unsigned int offset,

> > +			 unsigned int len)

> > +{

> > +	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);

> > +

> > +	if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM) && sbi->linear_virt_addr)

> > +		return cramfs_direct_read(sb, offset, len);

> > +	else if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV))

> > +		return cramfs_blkdev_read(sb, offset, len);

> > +	else

> > +		return NULL;

> > +}

> > +

> > +static void cramfs_blkdev_kill_sb(struct super_block *sb)

> >  {

> >  	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);

> >  

> > @@ -247,6 +285,16 @@ static void cramfs_kill_sb(struct super_block *sb)

> >  	kfree(sbi);

> >  }

> >  

> > +static void cramfs_physmem_kill_sb(struct super_block *sb)

> > +{

> > +	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);

> > +

> > +	if (sbi->linear_virt_addr)

> > +		memunmap(sbi->linear_virt_addr);

> > +	kill_anon_super(sb);

> > +	kfree(sbi);

> > +}

> > +

> >  static int cramfs_remount(struct super_block *sb, int *flags, char *data)

> >  {

> >  	sync_filesystem(sb);

> > @@ -254,34 +302,24 @@ static int cramfs_remount(struct super_block *sb, int *flags, char *data)

> >  	return 0;

> >  }

> >  

> > -static int cramfs_fill_super(struct super_block *sb, void *data, int silent)

> > +static int cramfs_read_super(struct super_block *sb,

> > +			     struct cramfs_super *super, int silent)

> >  {

> > -	int i;

> > -	struct cramfs_super super;

> > +	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);

> >  	unsigned long root_offset;

> > -	struct cramfs_sb_info *sbi;

> > -	struct inode *root;

> > -

> > -	sb->s_flags |= MS_RDONLY;

> > -

> > -	sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);

> > -	if (!sbi)

> > -		return -ENOMEM;

> > -	sb->s_fs_info = sbi;

> >  

> > -	/* Invalidate the read buffers on mount: think disk change.. */

> > -	mutex_lock(&read_mutex);

> > -	for (i = 0; i < READ_BUFFERS; i++)

> > -		buffer_blocknr[i] = -1;

> > +	/* We don't know the real size yet */

> > +	sbi->size = PAGE_SIZE;

> >  

> >  	/* Read the first block and get the superblock from it */

> > -	memcpy(&super, cramfs_read(sb, 0, sizeof(super)), sizeof(super));

> > +	mutex_lock(&read_mutex);

> > +	memcpy(super, cramfs_read(sb, 0, sizeof(*super)), sizeof(*super));

> >  	mutex_unlock(&read_mutex);

> >  

> >  	/* Do sanity checks on the superblock */

> > -	if (super.magic != CRAMFS_MAGIC) {

> > +	if (super->magic != CRAMFS_MAGIC) {

> >  		/* check for wrong endianness */

> > -		if (super.magic == CRAMFS_MAGIC_WEND) {

> > +		if (super->magic == CRAMFS_MAGIC_WEND) {

> >  			if (!silent)

> >  				pr_err("wrong endianness\n");

> >  			return -EINVAL;

> > @@ -289,10 +327,10 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)

> >  

> >  		/* check at 512 byte offset */

> >  		mutex_lock(&read_mutex);

> > -		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));

> > +		memcpy(super, cramfs_read(sb, 512, sizeof(*super)), sizeof(*super));

> >  		mutex_unlock(&read_mutex);

> > -		if (super.magic != CRAMFS_MAGIC) {

> > -			if (super.magic == CRAMFS_MAGIC_WEND && !silent)

> > +		if (super->magic != CRAMFS_MAGIC) {

> > +			if (super->magic == CRAMFS_MAGIC_WEND && !silent)

> >  				pr_err("wrong endianness\n");

> >  			else if (!silent)

> >  				pr_err("wrong magic\n");

> > @@ -301,34 +339,34 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)

> >  	}

> >  

> >  	/* get feature flags first */

> > -	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {

> > +	if (super->flags & ~CRAMFS_SUPPORTED_FLAGS) {

> >  		pr_err("unsupported filesystem features\n");

> >  		return -EINVAL;

> >  	}

> >  

> >  	/* Check that the root inode is in a sane state */

> > -	if (!S_ISDIR(super.root.mode)) {

> > +	if (!S_ISDIR(super->root.mode)) {

> >  		pr_err("root is not a directory\n");

> >  		return -EINVAL;

> >  	}

> >  	/* correct strange, hard-coded permissions of mkcramfs */

> > -	super.root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);

> > +	super->root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);

> >  

> > -	root_offset = super.root.offset << 2;

> > -	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {

> > -		sbi->size = super.size;

> > -		sbi->blocks = super.fsid.blocks;

> > -		sbi->files = super.fsid.files;

> > +	root_offset = super->root.offset << 2;

> > +	if (super->flags & CRAMFS_FLAG_FSID_VERSION_2) {

> > +		sbi->size = super->size;

> > +		sbi->blocks = super->fsid.blocks;

> > +		sbi->files = super->fsid.files;

> >  	} else {

> >  		sbi->size = 1<<28;

> >  		sbi->blocks = 0;

> >  		sbi->files = 0;

> >  	}

> > -	sbi->magic = super.magic;

> > -	sbi->flags = super.flags;

> > +	sbi->magic = super->magic;

> > +	sbi->flags = super->flags;

> >  	if (root_offset == 0)

> >  		pr_info("empty filesystem");

> > -	else if (!(super.flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&

> > +	else if (!(super->flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&

> >  		 ((root_offset != sizeof(struct cramfs_super)) &&

> >  		  (root_offset != 512 + sizeof(struct cramfs_super))))

> >  	{

> > @@ -336,9 +374,18 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)

> >  		return -EINVAL;

> >  	}

> >  

> > +	return 0;

> > +}

> > +

> > +static int cramfs_finalize_super(struct super_block *sb,

> > +				 struct cramfs_inode *cramfs_root)

> > +{

> > +	struct inode *root;

> > +

> >  	/* Set it all up.. */

> > +	sb->s_flags |= MS_RDONLY;

> >  	sb->s_op = &cramfs_ops;

> > -	root = get_cramfs_inode(sb, &super.root, 0);

> > +	root = get_cramfs_inode(sb, cramfs_root, 0);

> >  	if (IS_ERR(root))

> >  		return PTR_ERR(root);

> >  	sb->s_root = d_make_root(root);

> > @@ -347,6 +394,92 @@ static int cramfs_fill_super(struct super_block *sb, void *data, int silent)

> >  	return 0;

> >  }

> >  

> > +static int cramfs_blkdev_fill_super(struct super_block *sb, void *data, int silent)

> > +{

> > +	struct cramfs_sb_info *sbi;

> > +	struct cramfs_super super;

> > +	int i, err;

> > +

> > +	sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);

> > +	if (!sbi)

> > +		return -ENOMEM;

> > +	sb->s_fs_info = sbi;

> > +

> > +	/* Invalidate the read buffers on mount: think disk change.. */

> > +	for (i = 0; i < READ_BUFFERS; i++)

> > +		buffer_blocknr[i] = -1;

> > +

> > +	err = cramfs_read_super(sb, &super, silent);

> > +	if (err)

> > +		return err;

> > +	return cramfs_finalize_super(sb, &super.root);

> > +}

> > +

> > +static int cramfs_physmem_fill_super(struct super_block *sb, void *data, int silent)

> > +{

> > +	struct cramfs_sb_info *sbi;

> > +	struct cramfs_super super;

> > +	char *p;

> > +	int err;

> > +

> > +	sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);

> > +	if (!sbi)

> > +		return -ENOMEM;

> > +	sb->s_fs_info = sbi;

> > +

> > +	/*

> > +	 * The physical location of the cramfs image is specified as

> > +	 * a mount parameter.  This parameter is mandatory for obvious

> > +	 * reasons.  Some validation is made on the phys address but this

> > +	 * is not exhaustive and we count on the fact that someone using

> > +	 * this feature is supposed to know what he/she's doing.

> > +	 */

> > +	if (!data || !(p = strstr((char *)data, "physaddr="))) {

> > +		pr_err("unknown physical address for linear cramfs image\n");

> > +		return -EINVAL;

> > +	}

> > +	sbi->linear_phys_addr = memparse(p + 9, NULL);

> > +	if (!sbi->linear_phys_addr) {

> > +		pr_err("bad value for cramfs image physical address\n");

> > +		return -EINVAL;

> > +	}

> > +	if (sbi->linear_phys_addr & (PAGE_SIZE-1)) {

> > +		pr_err("physical address %pap for linear cramfs isn't aligned to a page boundary\n",

> > +			&sbi->linear_phys_addr);

> > +		return -EINVAL;

> > +	}

> > +

> > +	/*

> > +	 * Map only one page for now.  Will remap it when fs size is known.

> > +	 * Although we'll only read from it, we want the CPU cache to

> > +	 * kick in for the higher throughput it provides, hence MEMREMAP_WB.

> > +	 */

> > +	pr_info("checking physical address %pap for linear cramfs image\n", &sbi->linear_phys_addr);

> > +	sbi->linear_virt_addr = memremap(sbi->linear_phys_addr, PAGE_SIZE,

> > +					 MEMREMAP_WB);

> > +	if (!sbi->linear_virt_addr) {

> > +		pr_err("ioremap of the linear cramfs image failed\n");

> > +		return -ENOMEM;

> > +	}

> > +

> > +	err = cramfs_read_super(sb, &super, silent);

> > +	if (err)

> > +		return err;

> > +

> > +	/* Remap the whole filesystem now */

> > +	pr_info("linear cramfs image appears to be %lu KB in size\n",

> > +		sbi->size/1024);

> > +	memunmap(sbi->linear_virt_addr);

> > +	sbi->linear_virt_addr = memremap(sbi->linear_phys_addr, sbi->size,

> > +					 MEMREMAP_WB);

> > +	if (!sbi->linear_virt_addr) {

> > +		pr_err("ioremap of the linear cramfs image failed\n");

> > +		return -ENOMEM;

> > +	}

> > +

> > +	return cramfs_finalize_super(sb, &super.root);

> > +}

> > +

> >  static int cramfs_statfs(struct dentry *dentry, struct kstatfs *buf)

> >  {

> >  	struct super_block *sb = dentry->d_sb;

> > @@ -573,38 +706,67 @@ static const struct super_operations cramfs_ops = {

> >  	.statfs		= cramfs_statfs,

> >  };

> >  

> > -static struct dentry *cramfs_mount(struct file_system_type *fs_type,

> > -	int flags, const char *dev_name, void *data)

> > +static struct dentry *cramfs_blkdev_mount(struct file_system_type *fs_type,

> > +				int flags, const char *dev_name, void *data)

> > +{

> > +	return mount_bdev(fs_type, flags, dev_name, data, cramfs_blkdev_fill_super);

> > +}

> > +

> > +static struct dentry *cramfs_physmem_mount(struct file_system_type *fs_type,

> > +				int flags, const char *dev_name, void *data)

> >  {

> > -	return mount_bdev(fs_type, flags, dev_name, data, cramfs_fill_super);

> > +	return mount_nodev(fs_type, flags, data, cramfs_physmem_fill_super);

> >  }

> >  

> >  static struct file_system_type cramfs_fs_type = {

> >  	.owner		= THIS_MODULE,

> >  	.name		= "cramfs",

> > -	.mount		= cramfs_mount,

> > -	.kill_sb	= cramfs_kill_sb,

> > +	.mount		= cramfs_blkdev_mount,

> > +	.kill_sb	= cramfs_blkdev_kill_sb,

> >  	.fs_flags	= FS_REQUIRES_DEV,

> >  };

> > +

> > +static struct file_system_type cramfs_physmem_fs_type = {

> > +	.owner		= THIS_MODULE,

> > +	.name		= "cramfs_physmem",

> > +	.mount		= cramfs_physmem_mount,

> > +	.kill_sb	= cramfs_physmem_kill_sb,

> > +};

> > +

> > +#ifdef CONFIG_CRAMFS_BLOCKDEV

> >  MODULE_ALIAS_FS("cramfs");

> > +#endif

> > +#ifdef CONFIG_CRAMFS_PHYSMEM

> > +MODULE_ALIAS_FS("cramfs_physmem");

> > +#endif

> >  

> >  static int __init init_cramfs_fs(void)

> >  {

> >  	int rv;

> >  

> > -	rv = cramfs_uncompress_init();

> > -	if (rv < 0)

> > -		return rv;

> > -	rv = register_filesystem(&cramfs_fs_type);

> > -	if (rv < 0)

> > -		cramfs_uncompress_exit();

> > -	return rv;

> > +	if ((rv = cramfs_uncompress_init()) < 0)

> > +		goto err0;

> > +	if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV) &&

> > +	    (rv = register_filesystem(&cramfs_fs_type)) < 0)

> > +		goto err1;

> > +	if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM) &&

> > +	    (rv = register_filesystem(&cramfs_physmem_fs_type)) < 0)

> > +		goto err2;

> > +	return 0;

> > +

> > +err2:	if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV))

> > +		unregister_filesystem(&cramfs_fs_type);

> > +err1:	cramfs_uncompress_exit();

> > +err0:	return rv;

> >  }

> >  

> >  static void __exit exit_cramfs_fs(void)

> >  {

> >  	cramfs_uncompress_exit();

> > -	unregister_filesystem(&cramfs_fs_type);

> > +	if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV))

> > +		unregister_filesystem(&cramfs_fs_type);

> > +	if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM))

> > +		unregister_filesystem(&cramfs_physmem_fs_type);

> >  }

> >  

> >  module_init(init_cramfs_fs)

> > -- 

> > 2.9.5

> > 

> ---end quoted text---

>
Rob Herring (Arm) Oct. 3, 2017, 2:43 p.m. UTC | #2
On Sun, Oct 1, 2017 at 3:29 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Sep 27, 2017 at 07:32:20PM -0400, Nicolas Pitre wrote:

>> To distinguish between both access types, the cramfs_physmem filesystem

>> type must be specified when using a memory accessible cramfs image, and

>> the physaddr argument must provide the actual filesystem image's physical

>> memory location.

>

> Sorry, but this still is a complete no-go.  A physical address is not a

> proper interface.  You still need to have some interface for your NOR nand

> or DRAM.  - usually that would be a mtd driver, but if you have a good

> reason why that's not suitable for you (and please explain it well)

> we'll need a little OF or similar layer to bind a thin driver.


I don't disagree that we may need DT binding here, but DT bindings are
h/w description and not a mechanism bind Linux kernel drivers. It can
be a subtle distinction, but it is an important one.

I can see the case where we have no driver. For RAM we don't have a
driver, yet pretty much all hardware has a DRAM controller which we
just rely on the firmware to setup. I could also envision that we have
hardware we do need to configure in the kernel. Perhaps the boot
settings are not optimal or we want/need to manage the clocks. That
seems somewhat unlikely if the kernel is also XIP from the same flash
as it is in Nico's case.

We do often describe the flash layout in DT when partitions are not
discoverable. I don't know if that would be needed here. Would the ROM
here ever be updateable from within Linux? If we're talking about a
single address to pass the kernel, DT seems like an overkill and
kernel cmdline is perfectly valid IMO.

Rob
Chris Brandt Oct. 3, 2017, 2:58 p.m. UTC | #3
On Tuesday, October 03, 2017 1, Rob Herring wrote:
> On Sun, Oct 1, 2017 at 3:29 AM, Christoph Hellwig <hch@infradead.org>

> wrote:

> > On Wed, Sep 27, 2017 at 07:32:20PM -0400, Nicolas Pitre wrote:

> >> To distinguish between both access types, the cramfs_physmem filesystem

> >> type must be specified when using a memory accessible cramfs image, and

> >> the physaddr argument must provide the actual filesystem image's

> physical

> >> memory location.

> >

> > Sorry, but this still is a complete no-go.  A physical address is not a

> > proper interface.  You still need to have some interface for your NOR

> nand

> > or DRAM.  - usually that would be a mtd driver, but if you have a good

> > reason why that's not suitable for you (and please explain it well)

> > we'll need a little OF or similar layer to bind a thin driver.

> 

> I don't disagree that we may need DT binding here, but DT bindings are

> h/w description and not a mechanism bind Linux kernel drivers. It can

> be a subtle distinction, but it is an important one.

> 

> I can see the case where we have no driver. For RAM we don't have a

> driver, yet pretty much all hardware has a DRAM controller which we

> just rely on the firmware to setup. I could also envision that we have

> hardware we do need to configure in the kernel. Perhaps the boot

> settings are not optimal or we want/need to manage the clocks. That

> seems somewhat unlikely if the kernel is also XIP from the same flash

> as it is in Nico's case.

> 

> We do often describe the flash layout in DT when partitions are not

> discoverable. I don't know if that would be needed here. Would the ROM

> here ever be updateable from within Linux? If we're talking about a

> single address to pass the kernel, DT seems like an overkill and

> kernel cmdline is perfectly valid IMO.



As someone that's been using an XIP File system for a while now (AXFS, 
obviously not xip-cramfs), there is a way (in my system at least) to 
write to the same Flash that the kernel and file system are currently XIP 
executing (think jumping to RAM, doing a small flash operation, then 
jumping back to Flash).

The use case is if you've logically partitioned your flash such that you
keep your application in a separate file XIP filesystem image, you 
remotely download an updated version to some unused portion of Flash, then 
simply unmount what you have been using and mount the new image since you
can pass in the physical address of where you wrote your new image to.

So in that case, I guess you can do some type of DT overlay or 
something, but at the moment, just having the physical address as a parameter in 
mount command makes it pretty darn easy.

Chris
'Christoph Hellwig' Oct. 3, 2017, 2:59 p.m. UTC | #4
On Sun, Oct 01, 2017 at 06:27:11PM -0400, Nicolas Pitre wrote:
> If you prefer, the physical address could be specified with a Kconfig 

> symbol just like the kernel link address. Personally I think it is best 

> to keep it along with the other root mount args. But going all the way 

> with a dynamic driver binding interface and a dummy intermediate name is 

> like using a sledge hammer to kill an ant: it will work of course, but 

> given the context it is prone to errors due to the added manipulations 

> mentioned previously ... and a tad overkill.


As soon as a kernel enables CRAMFS_PHYSMEM this mount option is
available, so you don't just need to think of your use case.

The normal way for doings this would be to specify it in the device
tree.
diff mbox series

Patch

diff --git a/fs/cramfs/Kconfig b/fs/cramfs/Kconfig
index 11b29d491b..5b4e0b7e13 100644
--- a/fs/cramfs/Kconfig
+++ b/fs/cramfs/Kconfig
@@ -1,6 +1,5 @@ 
 config CRAMFS
 	tristate "Compressed ROM file system support (cramfs) (OBSOLETE)"
-	depends on BLOCK
 	select ZLIB_INFLATE
 	help
 	  Saying Y here includes support for CramFs (Compressed ROM File
@@ -20,3 +19,31 @@  config CRAMFS
 	  in terms of performance and features.
 
 	  If unsure, say N.
+
+config CRAMFS_BLOCKDEV
+	bool "Support CramFs image over a regular block device" if EXPERT
+	depends on CRAMFS && BLOCK
+	default y
+	help
+	  This option allows the CramFs driver to load data from a regular
+	  block device such a disk partition or a ramdisk.
+
+config CRAMFS_PHYSMEM
+	bool "Support CramFs image directly mapped in physical memory"
+	depends on CRAMFS
+	default y if !CRAMFS_BLOCKDEV
+	help
+	  This option allows the CramFs driver to load data directly from
+	  a linear adressed memory range (usually non volatile memory
+	  like flash) instead of going through the block device layer.
+	  This saves some memory since no intermediate buffering is
+	  necessary.
+
+	  The filesystem type for this feature is "cramfs_physmem".
+	  The location of the CramFs image in memory is board
+	  dependent. Therefore, if you say Y, you must know the proper
+	  physical address where to store the CramFs image and specify
+	  it using the physaddr=0x******** mount option (for example:
+	  "mount -t cramfs_physmem -o physaddr=0x100000 none /mnt").
+
+	  If unsure, say N.
diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 7919967488..19f464a214 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -24,6 +24,7 @@ 
 #include <linux/mutex.h>
 #include <uapi/linux/cramfs_fs.h>
 #include <linux/uaccess.h>
+#include <linux/io.h>
 
 #include "internal.h"
 
@@ -36,6 +37,8 @@  struct cramfs_sb_info {
 	unsigned long blocks;
 	unsigned long files;
 	unsigned long flags;
+	void *linear_virt_addr;
+	phys_addr_t linear_phys_addr;
 };
 
 static inline struct cramfs_sb_info *CRAMFS_SB(struct super_block *sb)
@@ -140,6 +143,9 @@  static struct inode *get_cramfs_inode(struct super_block *sb,
  * BLKS_PER_BUF*PAGE_SIZE, so that the caller doesn't need to
  * worry about end-of-buffer issues even when decompressing a full
  * page cache.
+ *
+ * Note: This is all optimized away at compile time when
+ *       CONFIG_CRAMFS_BLOCKDEV=n.
  */
 #define READ_BUFFERS (2)
 /* NEXT_BUFFER(): Loop over [0..(READ_BUFFERS-1)]. */
@@ -160,10 +166,10 @@  static struct super_block *buffer_dev[READ_BUFFERS];
 static int next_buffer;
 
 /*
- * Returns a pointer to a buffer containing at least LEN bytes of
- * filesystem starting at byte offset OFFSET into the filesystem.
+ * Populate our block cache and return a pointer from it.
  */
-static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned int len)
+static void *cramfs_blkdev_read(struct super_block *sb, unsigned int offset,
+				unsigned int len)
 {
 	struct address_space *mapping = sb->s_bdev->bd_inode->i_mapping;
 	struct page *pages[BLKS_PER_BUF];
@@ -239,7 +245,39 @@  static void *cramfs_read(struct super_block *sb, unsigned int offset, unsigned i
 	return read_buffers[buffer] + offset;
 }
 
-static void cramfs_kill_sb(struct super_block *sb)
+/*
+ * Return a pointer to the linearly addressed cramfs image in memory.
+ */
+static void *cramfs_direct_read(struct super_block *sb, unsigned int offset,
+				unsigned int len)
+{
+	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
+
+	if (!len)
+		return NULL;
+	if (len > sbi->size || offset > sbi->size - len)
+	       return page_address(ZERO_PAGE(0));
+	return sbi->linear_virt_addr + offset;
+}
+
+/*
+ * Returns a pointer to a buffer containing at least LEN bytes of
+ * filesystem starting at byte offset OFFSET into the filesystem.
+ */
+static void *cramfs_read(struct super_block *sb, unsigned int offset,
+			 unsigned int len)
+{
+	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
+
+	if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM) && sbi->linear_virt_addr)
+		return cramfs_direct_read(sb, offset, len);
+	else if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV))
+		return cramfs_blkdev_read(sb, offset, len);
+	else
+		return NULL;
+}
+
+static void cramfs_blkdev_kill_sb(struct super_block *sb)
 {
 	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
 
@@ -247,6 +285,16 @@  static void cramfs_kill_sb(struct super_block *sb)
 	kfree(sbi);
 }
 
+static void cramfs_physmem_kill_sb(struct super_block *sb)
+{
+	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
+
+	if (sbi->linear_virt_addr)
+		memunmap(sbi->linear_virt_addr);
+	kill_anon_super(sb);
+	kfree(sbi);
+}
+
 static int cramfs_remount(struct super_block *sb, int *flags, char *data)
 {
 	sync_filesystem(sb);
@@ -254,34 +302,24 @@  static int cramfs_remount(struct super_block *sb, int *flags, char *data)
 	return 0;
 }
 
-static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
+static int cramfs_read_super(struct super_block *sb,
+			     struct cramfs_super *super, int silent)
 {
-	int i;
-	struct cramfs_super super;
+	struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
 	unsigned long root_offset;
-	struct cramfs_sb_info *sbi;
-	struct inode *root;
-
-	sb->s_flags |= MS_RDONLY;
-
-	sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);
-	if (!sbi)
-		return -ENOMEM;
-	sb->s_fs_info = sbi;
 
-	/* Invalidate the read buffers on mount: think disk change.. */
-	mutex_lock(&read_mutex);
-	for (i = 0; i < READ_BUFFERS; i++)
-		buffer_blocknr[i] = -1;
+	/* We don't know the real size yet */
+	sbi->size = PAGE_SIZE;
 
 	/* Read the first block and get the superblock from it */
-	memcpy(&super, cramfs_read(sb, 0, sizeof(super)), sizeof(super));
+	mutex_lock(&read_mutex);
+	memcpy(super, cramfs_read(sb, 0, sizeof(*super)), sizeof(*super));
 	mutex_unlock(&read_mutex);
 
 	/* Do sanity checks on the superblock */
-	if (super.magic != CRAMFS_MAGIC) {
+	if (super->magic != CRAMFS_MAGIC) {
 		/* check for wrong endianness */
-		if (super.magic == CRAMFS_MAGIC_WEND) {
+		if (super->magic == CRAMFS_MAGIC_WEND) {
 			if (!silent)
 				pr_err("wrong endianness\n");
 			return -EINVAL;
@@ -289,10 +327,10 @@  static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 
 		/* check at 512 byte offset */
 		mutex_lock(&read_mutex);
-		memcpy(&super, cramfs_read(sb, 512, sizeof(super)), sizeof(super));
+		memcpy(super, cramfs_read(sb, 512, sizeof(*super)), sizeof(*super));
 		mutex_unlock(&read_mutex);
-		if (super.magic != CRAMFS_MAGIC) {
-			if (super.magic == CRAMFS_MAGIC_WEND && !silent)
+		if (super->magic != CRAMFS_MAGIC) {
+			if (super->magic == CRAMFS_MAGIC_WEND && !silent)
 				pr_err("wrong endianness\n");
 			else if (!silent)
 				pr_err("wrong magic\n");
@@ -301,34 +339,34 @@  static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	/* get feature flags first */
-	if (super.flags & ~CRAMFS_SUPPORTED_FLAGS) {
+	if (super->flags & ~CRAMFS_SUPPORTED_FLAGS) {
 		pr_err("unsupported filesystem features\n");
 		return -EINVAL;
 	}
 
 	/* Check that the root inode is in a sane state */
-	if (!S_ISDIR(super.root.mode)) {
+	if (!S_ISDIR(super->root.mode)) {
 		pr_err("root is not a directory\n");
 		return -EINVAL;
 	}
 	/* correct strange, hard-coded permissions of mkcramfs */
-	super.root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
+	super->root.mode |= (S_IRUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
 
-	root_offset = super.root.offset << 2;
-	if (super.flags & CRAMFS_FLAG_FSID_VERSION_2) {
-		sbi->size = super.size;
-		sbi->blocks = super.fsid.blocks;
-		sbi->files = super.fsid.files;
+	root_offset = super->root.offset << 2;
+	if (super->flags & CRAMFS_FLAG_FSID_VERSION_2) {
+		sbi->size = super->size;
+		sbi->blocks = super->fsid.blocks;
+		sbi->files = super->fsid.files;
 	} else {
 		sbi->size = 1<<28;
 		sbi->blocks = 0;
 		sbi->files = 0;
 	}
-	sbi->magic = super.magic;
-	sbi->flags = super.flags;
+	sbi->magic = super->magic;
+	sbi->flags = super->flags;
 	if (root_offset == 0)
 		pr_info("empty filesystem");
-	else if (!(super.flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
+	else if (!(super->flags & CRAMFS_FLAG_SHIFTED_ROOT_OFFSET) &&
 		 ((root_offset != sizeof(struct cramfs_super)) &&
 		  (root_offset != 512 + sizeof(struct cramfs_super))))
 	{
@@ -336,9 +374,18 @@  static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 		return -EINVAL;
 	}
 
+	return 0;
+}
+
+static int cramfs_finalize_super(struct super_block *sb,
+				 struct cramfs_inode *cramfs_root)
+{
+	struct inode *root;
+
 	/* Set it all up.. */
+	sb->s_flags |= MS_RDONLY;
 	sb->s_op = &cramfs_ops;
-	root = get_cramfs_inode(sb, &super.root, 0);
+	root = get_cramfs_inode(sb, cramfs_root, 0);
 	if (IS_ERR(root))
 		return PTR_ERR(root);
 	sb->s_root = d_make_root(root);
@@ -347,6 +394,92 @@  static int cramfs_fill_super(struct super_block *sb, void *data, int silent)
 	return 0;
 }
 
+static int cramfs_blkdev_fill_super(struct super_block *sb, void *data, int silent)
+{
+	struct cramfs_sb_info *sbi;
+	struct cramfs_super super;
+	int i, err;
+
+	sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);
+	if (!sbi)
+		return -ENOMEM;
+	sb->s_fs_info = sbi;
+
+	/* Invalidate the read buffers on mount: think disk change.. */
+	for (i = 0; i < READ_BUFFERS; i++)
+		buffer_blocknr[i] = -1;
+
+	err = cramfs_read_super(sb, &super, silent);
+	if (err)
+		return err;
+	return cramfs_finalize_super(sb, &super.root);
+}
+
+static int cramfs_physmem_fill_super(struct super_block *sb, void *data, int silent)
+{
+	struct cramfs_sb_info *sbi;
+	struct cramfs_super super;
+	char *p;
+	int err;
+
+	sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);
+	if (!sbi)
+		return -ENOMEM;
+	sb->s_fs_info = sbi;
+
+	/*
+	 * The physical location of the cramfs image is specified as
+	 * a mount parameter.  This parameter is mandatory for obvious
+	 * reasons.  Some validation is made on the phys address but this
+	 * is not exhaustive and we count on the fact that someone using
+	 * this feature is supposed to know what he/she's doing.
+	 */
+	if (!data || !(p = strstr((char *)data, "physaddr="))) {
+		pr_err("unknown physical address for linear cramfs image\n");
+		return -EINVAL;
+	}
+	sbi->linear_phys_addr = memparse(p + 9, NULL);
+	if (!sbi->linear_phys_addr) {
+		pr_err("bad value for cramfs image physical address\n");
+		return -EINVAL;
+	}
+	if (sbi->linear_phys_addr & (PAGE_SIZE-1)) {
+		pr_err("physical address %pap for linear cramfs isn't aligned to a page boundary\n",
+			&sbi->linear_phys_addr);
+		return -EINVAL;
+	}
+
+	/*
+	 * Map only one page for now.  Will remap it when fs size is known.
+	 * Although we'll only read from it, we want the CPU cache to
+	 * kick in for the higher throughput it provides, hence MEMREMAP_WB.
+	 */
+	pr_info("checking physical address %pap for linear cramfs image\n", &sbi->linear_phys_addr);
+	sbi->linear_virt_addr = memremap(sbi->linear_phys_addr, PAGE_SIZE,
+					 MEMREMAP_WB);
+	if (!sbi->linear_virt_addr) {
+		pr_err("ioremap of the linear cramfs image failed\n");
+		return -ENOMEM;
+	}
+
+	err = cramfs_read_super(sb, &super, silent);
+	if (err)
+		return err;
+
+	/* Remap the whole filesystem now */
+	pr_info("linear cramfs image appears to be %lu KB in size\n",
+		sbi->size/1024);
+	memunmap(sbi->linear_virt_addr);
+	sbi->linear_virt_addr = memremap(sbi->linear_phys_addr, sbi->size,
+					 MEMREMAP_WB);
+	if (!sbi->linear_virt_addr) {
+		pr_err("ioremap of the linear cramfs image failed\n");
+		return -ENOMEM;
+	}
+
+	return cramfs_finalize_super(sb, &super.root);
+}
+
 static int cramfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	struct super_block *sb = dentry->d_sb;
@@ -573,38 +706,67 @@  static const struct super_operations cramfs_ops = {
 	.statfs		= cramfs_statfs,
 };
 
-static struct dentry *cramfs_mount(struct file_system_type *fs_type,
-	int flags, const char *dev_name, void *data)
+static struct dentry *cramfs_blkdev_mount(struct file_system_type *fs_type,
+				int flags, const char *dev_name, void *data)
+{
+	return mount_bdev(fs_type, flags, dev_name, data, cramfs_blkdev_fill_super);
+}
+
+static struct dentry *cramfs_physmem_mount(struct file_system_type *fs_type,
+				int flags, const char *dev_name, void *data)
 {
-	return mount_bdev(fs_type, flags, dev_name, data, cramfs_fill_super);
+	return mount_nodev(fs_type, flags, data, cramfs_physmem_fill_super);
 }
 
 static struct file_system_type cramfs_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "cramfs",
-	.mount		= cramfs_mount,
-	.kill_sb	= cramfs_kill_sb,
+	.mount		= cramfs_blkdev_mount,
+	.kill_sb	= cramfs_blkdev_kill_sb,
 	.fs_flags	= FS_REQUIRES_DEV,
 };
+
+static struct file_system_type cramfs_physmem_fs_type = {
+	.owner		= THIS_MODULE,
+	.name		= "cramfs_physmem",
+	.mount		= cramfs_physmem_mount,
+	.kill_sb	= cramfs_physmem_kill_sb,
+};
+
+#ifdef CONFIG_CRAMFS_BLOCKDEV
 MODULE_ALIAS_FS("cramfs");
+#endif
+#ifdef CONFIG_CRAMFS_PHYSMEM
+MODULE_ALIAS_FS("cramfs_physmem");
+#endif
 
 static int __init init_cramfs_fs(void)
 {
 	int rv;
 
-	rv = cramfs_uncompress_init();
-	if (rv < 0)
-		return rv;
-	rv = register_filesystem(&cramfs_fs_type);
-	if (rv < 0)
-		cramfs_uncompress_exit();
-	return rv;
+	if ((rv = cramfs_uncompress_init()) < 0)
+		goto err0;
+	if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV) &&
+	    (rv = register_filesystem(&cramfs_fs_type)) < 0)
+		goto err1;
+	if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM) &&
+	    (rv = register_filesystem(&cramfs_physmem_fs_type)) < 0)
+		goto err2;
+	return 0;
+
+err2:	if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV))
+		unregister_filesystem(&cramfs_fs_type);
+err1:	cramfs_uncompress_exit();
+err0:	return rv;
 }
 
 static void __exit exit_cramfs_fs(void)
 {
 	cramfs_uncompress_exit();
-	unregister_filesystem(&cramfs_fs_type);
+	if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV))
+		unregister_filesystem(&cramfs_fs_type);
+	if (IS_ENABLED(CONFIG_CRAMFS_PHYSMEM))
+		unregister_filesystem(&cramfs_physmem_fs_type);
 }
 
 module_init(init_cramfs_fs)