[U-Boot,RFC,2/2] fs: add partition switch libary, implement ls and fsload commands

From: Stephen Warren <swarren@nvidia.com>

Implement "ls" and "fsload" commands that act like {fat,ext2}{ls,load},
and transparently handle either file-system. This scheme could easily be
extended to other filesystem types; I only didn't do it for zfs because
I don't have any filesystems of that type.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
There are a couple FIXMEs in here:

1) In fs/fs.c, code is ifdef on CONFIG_CMD_FAT or CONFIG_CMD_EXT2. This
means that the new commands and code can only be enabled if the "legacy"
{fat,ext2}{ls,load} are enabled. What we really want is CONFIG_FS_FAT and
CONFIG_FS_EXT2 to enable the filesystem code, and then CONFIG_CMD_FAT,
CONFIG_CMD_EXT2, CONFIG_CMD_FS to only affect the command implementations.
However, that would require making every include/config/*.h that sets the
current defines also set more. I suppose that's a fairly mechanical change
though, so easy enough to implement. Does that seem like a reasonable
approach to people?

2) In common/Makefile, I need to make this conditional upon CONFIG_CMD_FS
or similar.

Also, I wonder if the fs/* and common/* should be two separate patches or
not?

 Makefile        |    3 +-
 common/Makefile |    2 +
 common/cmd_fs.c |   86 ++++++++++++++++++++++
 fs/Makefile     |   47 ++++++++++++
 fs/fs.c         |  216 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fs.h    |   49 +++++++++++++
 6 files changed, 402 insertions(+), 1 deletions(-)
 create mode 100644 common/cmd_fs.c
 create mode 100644 fs/Makefile
 create mode 100644 fs/fs.c
 create mode 100644 include/fs.h

Hi Stephen,

On Thursday, October 11, 2012 2:05:07 AM, Stephen Warren wrote:
> Implement "ls" and "fsload" commands that act like
> {fat,ext2}{ls,load},
> and transparently handle either file-system. This scheme could easily
> be
> extended to other filesystem types; I only didn't do it for zfs
> because
> I don't have any filesystems of that type.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> There are a couple FIXMEs in here:
> 
> 1) In fs/fs.c, code is ifdef on CONFIG_CMD_FAT or CONFIG_CMD_EXT2.
> This
> means that the new commands and code can only be enabled if the
> "legacy"
> {fat,ext2}{ls,load} are enabled. What we really want is CONFIG_FS_FAT
> and
> CONFIG_FS_EXT2 to enable the filesystem code, and then
> CONFIG_CMD_FAT,
> CONFIG_CMD_EXT2, CONFIG_CMD_FS to only affect the command
> implementations.
> However, that would require making every include/config/*.h that sets
> the
> current defines also set more. I suppose that's a fairly mechanical
> change
> though, so easy enough to implement. Does that seem like a reasonable
> approach to people?

What about making CONFIG_CMD_<FS> auto-define CONFIG_FS_<FS>, just like with a
Kconfig select?

> 2) In common/Makefile, I need to make this conditional upon
> CONFIG_CMD_FS
> or similar.
> 
> Also, I wonder if the fs/* and common/* should be two separate
> patches or
> not?

For me, it's fine as a single patch.

>  Makefile        |    3 +-
>  common/Makefile |    2 +
>  common/cmd_fs.c |   86 ++++++++++++++++++++++
>  fs/Makefile     |   47 ++++++++++++
>  fs/fs.c         |  216
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fs.h    |   49 +++++++++++++
>  6 files changed, 402 insertions(+), 1 deletions(-)
>  create mode 100644 common/cmd_fs.c
>  create mode 100644 fs/Makefile
>  create mode 100644 fs/fs.c
>  create mode 100644 include/fs.h
> 
> diff --git a/Makefile b/Makefile
> index d7e2c2f..0b50eb7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -242,7 +242,8 @@ LIBS-y += drivers/net/npe/libnpe.o
>  endif
>  LIBS-$(CONFIG_OF_EMBED) += dts/libdts.o
>  LIBS-y += arch/$(ARCH)/lib/lib$(ARCH).o
> -LIBS-y += fs/cramfs/libcramfs.o \
> +LIBS-y += fs/libfs.o \
> +	fs/cramfs/libcramfs.o \
>  	fs/ext4/libext4fs.o \
>  	fs/fat/libfat.o \
>  	fs/fdos/libfdos.o \
> diff --git a/common/Makefile b/common/Makefile
> index 125b2be..4f2d944 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -103,6 +103,8 @@ COBJS-$(CONFIG_CMD_FLASH) += cmd_flash.o
>  ifdef CONFIG_FPGA
>  COBJS-$(CONFIG_CMD_FPGA) += cmd_fpga.o
>  endif
> +# FIXME: Need to make this conditional
> +COBJS-y += cmd_fs.o
>  COBJS-$(CONFIG_CMD_GPIO) += cmd_gpio.o
>  COBJS-$(CONFIG_CMD_I2C) += cmd_i2c.o
>  COBJS-$(CONFIG_CMD_IDE) += cmd_ide.o
> diff --git a/common/cmd_fs.c b/common/cmd_fs.c
> new file mode 100644
> index 0000000..948a5e0
> --- /dev/null
> +++ b/common/cmd_fs.c
> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Inspired by cmd_ext_common.c, cmd_fat.c.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <fs.h>
> +
> +int do_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
> +{
> +	unsigned long addr;
> +	unsigned long bytes;
> +	unsigned long pos;
> +	int len_read;
> +	char buf[12];
> +
> +	if (argc < 5)
> +		return -1;

What about "return CMD_RET_USAGE;"?

> +
> +	if (fs_set_blk_dev(argv[1], argv[2]))
> +		return 1;
> +
> +	addr = simple_strtoul(argv[3], NULL, 0);
> +	if (argc >= 6)
> +		bytes = simple_strtoul(argv[5], NULL, 0);
> +	else
> +		bytes = 0;
> +	if (argc >= 7)
> +		pos = simple_strtoul(argv[6], NULL, 0);
> +	else
> +		pos = 0;
> +
> +	len_read = fs_read(argv[4], addr, pos, bytes);
> +	if (len_read <= 0)
> +		return 1;
> +
> +	sprintf(buf, "0x%x", (unsigned int)len_read);
> +	setenv("filesize", buf);
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	fsload,	7,	0,	do_fsload,
> +	"load binary file from a filesystem",
> +	"<interface> [<dev[:part]>] <addr> <filename> [bytes [pos]]\n"
> +	"    - Load binary file 'filename' from partition 'part' on
> device\n"
> +	"       type 'interface' instance 'dev' to address 'addr' in
> memory.\n"
> +	"      'bytes' gives the size to load in bytes.\n"
> +	"      If 'bytes' is 0 or omitted, the file is read until the
> end.\n"
> +	"      'pos' gives the file byte position to start reading from.\n"
> +	"      If 'pos' is 0 or omitted, the file is read from the start."
> +);
> +
> +int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{

What about adding
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
?

> +	if (fs_set_blk_dev(argv[1], argv[2]))
> +		return 1;
> +
> +	if (fs_ls(argc == 4 ? argv[3] : "/"))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	ls,	4,	1,	do_ls,
> +	"list files in a directory (default /)",
> +	"<interface> [<dev[:part]>] [directory]\n"
> +	"    - List files in directory 'directory' of partition 'part'
> on\n"
> +	"      device type 'interface' instance 'dev'."
> +);
> diff --git a/fs/Makefile b/fs/Makefile
> new file mode 100644
> index 0000000..d0ab3ae
> --- /dev/null
> +++ b/fs/Makefile
> @@ -0,0 +1,47 @@
> +#
> +# (C) Copyright 2000-2006
> +# Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> +# Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> +#
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +
> +include $(TOPDIR)/config.mk
> +
> +LIB	= $(obj)libfs.o
> +
> +COBJS-y				+= fs.o
> +
> +COBJS	:= $(COBJS-y)
> +SRCS	:= $(COBJS:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(COBJS))
> +
> +all:	$(LIB)
> +
> +$(LIB):	$(obj).depend $(OBJS)
> +	$(call cmd_link_o_target, $(OBJS))
> +
> +#########################################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +#########################################################################
> diff --git a/fs/fs.c b/fs/fs.c
> new file mode 100644
> index 0000000..ea77ac9
> --- /dev/null
> +++ b/fs/fs.c
> @@ -0,0 +1,216 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <part.h>
> +#include <ext4fs.h>
> +#include <fat.h>
> +#include <fs.h>
> +
> +#define FS_TYPE_INVALID	0
> +#define FS_TYPE_FAT	1
> +#define FS_TYPE_EXT	2
> +
> +static block_dev_desc_t *fs_dev_desc;
> +static disk_partition_t fs_partition;
> +static int fs_type;

It should be initialized to FS_TYPE_INVALID to be clean.

> +
> +/* FIXME: CONFIG_CMD_FAT, CONFIG_CMD_EXT2 should be CONFIG_FS_FAT,
> CONFIG_FS_EXT2 */
> +#ifdef CONFIG_CMD_FAT
> +static int fs_probe_fat(void)
> +{
> +	return fat_set_blk_dev(fs_dev_desc, &fs_partition);
> +}
> +
> +static void fs_close_fat(void)
> +{
> +}
> +#else
> +static inline int fs_probe_fat(void)
> +{
> +	return -1;
> +}
> +
> +static inline void fs_close_fat(void)
> +{
> +}
> +#endif
> +
> +#ifdef CONFIG_CMD_EXT2
> +static int fs_probe_ext(void)
> +{
> +	ext4fs_set_blk_dev(fs_dev_desc, &fs_partition);
> +
> +	if (!ext4fs_mount(fs_partition.size)) {
> +		ext4fs_close();
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fs_close_ext(void)
> +{
> +	ext4fs_close();
> +}
> +#else
> +static inline int fs_probe_ext(void)
> +{
> +	return -1;
> +}
> +
> +static inline void fs_close_ext(void)
> +{
> +}
> +#endif
> +
> +int fs_set_blk_dev(const char *ifname, const char *dev_part_str)
> +{
> +	int part;
> +
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		fs_close_fat();
> +		break;
> +	case FS_TYPE_EXT:
> +		fs_close_ext();
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	fs_type = FS_TYPE_INVALID;

What about adding an fs_close() function that would just be the piece of code
above? It would have to be called at the end of command functions invoking
fs_set_blk_dev(). In that way, ext4 would not be left open after such a command.
That would be better if something else is done using ext4 between 2 such
commands.

> +
> +	part = get_device_and_partition(ifname, dev_part_str, &fs_dev_desc,
> +					&fs_partition, 1);
> +	if (part < 0)
> +		return -1;
> +
> +	if (!fs_probe_fat()) {
> +		fs_type = FS_TYPE_FAT;
> +		return 0;
> +	}
> +
> +	if (!fs_probe_ext()) {
> +		fs_type = FS_TYPE_EXT;
> +		return 0;
> +	}
> +
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +static inline int fs_ls_unsupported(const char *dirname)
> +{
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +#ifdef CONFIG_CMD_FAT
> +#define fs_ls_fat file_fat_ls
> +#else
> +#define fs_ls_fat fs_ls_unsupported
> +#endif
> +
> +#ifdef CONFIG_CMD_EXT2
> +#define fs_ls_ext ext4fs_ls
> +#else
> +#define fs_ls_ext fs_ls_unsupported
> +#endif
> +
> +int fs_ls(const char *dirname)
> +{
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		return fs_ls_fat(dirname);
> +	case FS_TYPE_EXT:
> +		return fs_ls_ext(dirname);
> +	default:
> +		return fs_ls_unsupported(dirname);
> +	}
> +}
> +
> +static inline int fs_read_unsupported(const char *filename, ulong
> addr, int offset, int len)
> +{
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +#ifdef CONFIG_CMD_FAT
> +static int fs_read_fat(const char *filename, ulong addr, int offset,
> int len)
> +{
> +	int len_read;
> +
> +	len_read = file_fat_read_at(filename, offset,
> +				    (unsigned char *)addr, len);
> +	if (len_read == -1) {
> +		printf("** Unable to read file %s **\n", filename);
> +		return -1;
> +	}
> +
> +	return len_read;
> +}
> +#else
> +#define fs_read_fat fs_read_unsupported
> +#endif
> +
> +#ifdef CONFIG_CMD_EXT2
> +static int fs_read_ext(const char *filename, ulong addr, int offset,
> int len)
> +{
> +	int file_len;
> +	int len_read;
> +
> +	if (offset != 0) {
> +		printf("** Cannot support non-zero offset **\n");
> +		return -1;
> +	}
> +
> +	file_len = ext4fs_open(filename);
> +	if (file_len < 0) {
> +		printf("** File not found %s **\n", filename);
> +		ext4fs_close();
> +		return -1;
> +	}
> +
> +	if (len == 0)
> +		len = file_len;
> +
> +	len_read = ext4fs_read((char *)addr, len);
> +	ext4fs_close();
> +
> +	if (len_read != len) {
> +		printf("** Unable to read file %s **\n", filename);
> +		return -1;
> +	}
> +
> +	return len_read;
> +}
> +#else
> +#define fs_read_ext fs_read_unsupported
> +#endif
> +
> +int fs_read(const char *filename, ulong addr, int offset, int len)
> +{
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		return fs_read_fat(filename, addr, offset, len);
> +	case FS_TYPE_EXT:
> +		return fs_read_ext(filename, addr, offset, len);
> +	default:
> +		return fs_read_unsupported(filename, addr, offset, len);
> +	}
> +}
> diff --git a/include/fs.h b/include/fs.h
> new file mode 100644
> index 0000000..a0fda28
> --- /dev/null
> +++ b/include/fs.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see
> <http://www.gnu.org/licenses/>.
> + */
> +#ifndef _FS_H
> +#define _FS_H
> +
> +/*
> + * Tell the fs layer which block device an partition to use for
> future
> + * commands. This also internally identifies the filesystem that is
> present
> + * within the partition.
> + *
> + * Returns 0 on success.
> + * Returns non-zero if there is an error accessing the disk or
> partition, or
> + * no known filesystem type could be recognized on it.
> + */
> +int fs_set_blk_dev(const char *ifname, const char *dev_part_str);
> +
> +/*
> + * Print the list of files on the partition previously set by
> fs_set_blk_dev(),
> + * in directory "dirname".
> + *
> + * Returns 0 on success. Returns non-zero on error.
> + */
> +int fs_ls(const char *dirname);
> +
> +/*
> + * Read file "filename" from the partition previously set by
> fs_set_blk_dev(),
> + * to address "addr", starting at byte offset "offset", and reading
> "len"
> + * bytes. "offset" may be 0 to read from the start of the file.
> "len" may be
> + * 0 to read the entire file. Note that not all filesystem types
> support
> + * either/both offset!=0 or len!=0.
> + *
> + * Returns number of bytes read on success. Returns <= 0 on error.
> + */
> +int fs_read(const char *filename, ulong addr, int offset, int len);
> +
> +#endif /* _FS_H */

Best regards,
Benoît

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/10/12 17:05, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Implement "ls" and "fsload" commands that act like 
> {fat,ext2}{ls,load}, and transparently handle either file-system. 
> This scheme could easily be extended to other filesystem types; I 
> only didn't do it for zfs because I don't have any filesystems of 
> that type.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com> --- There are a
>  couple FIXMEs in here:
> 
> 1) In fs/fs.c, code is ifdef on CONFIG_CMD_FAT or CONFIG_CMD_EXT2.
>  This means that the new commands and code can only be enabled if 
> the "legacy" {fat,ext2}{ls,load} are enabled. What we really want 
> is CONFIG_FS_FAT and CONFIG_FS_EXT2 to enable the filesystem code,
>  and then CONFIG_CMD_FAT, CONFIG_CMD_EXT2, CONFIG_CMD_FS to only 
> affect the command implementations. However, that would require 
> making every include/config/*.h that sets the current defines also
>  set more. I suppose that's a fairly mechanical change though, so 
> easy enough to implement. Does that seem like a reasonable approach
> to people?

How about a new CONFIG_CMD_GENERIC_FS for the new ls/fsload (and any
later commands like write that get added) and once most filesystems
are converted we can think about a transition plan?

> 2) In common/Makefile, I need to make this conditional upon 
> CONFIG_CMD_FS or similar.
> 
> Also, I wonder if the fs/* and common/* should be two separate 
> patches or not?

One is fine.

- -- 
Tom

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQdvgNAAoJENk4IS6UOR1Wx3gP/iRvYq0khe6ZPRbUcPyTg0r8
tkNJGfglXba52GHjU6URwSrPbN9qOHzh6fD0x26jOdj1vWlMv6nvwmyo3ttvyRLe
4VF7tvTRp3Zv461vGwm8XwAXlDXVrnsWeC4veWxo+/ptFaq2FTWYTVNo2MsqHSIN
VNnxMdgtqUIU2kgx0uJst1Fl3olDaRlQmyf88SiRE2et21FQytk/LxAc50zmNr5J
UWgVgzoUP+RnZwnZK2CWL9cAuGbEyLjQvoKK8V72dMvKzZgMFpyMEdk0+onKFYe1
Byai7INodWIhWrtQj4nGC/1WQC+kCteMvF3OTjuGY/bfDPqLYx3071kocrgYWSW4
URvvdv6hn1l+evN5BQ1erwAekwfgrfcKkavJwmuVES3ZESrEyComqWEjajqeTe/6
uIpZo58oFGojJZK1HcDmaVFyj7nxAzXloupmHiKq+xfXHbv60ZUZO6InEos/ZCjZ
bpT92wyyqTeiD70glLvLRyStKzZidqeoVTkbGM0XUCA3d3RvxXdB4zfgIqDlhhCT
EfhxKVkAZAzjsEn+U1/y5RWEEdD+Zaqi2xKwA+Ken9TJ4LFsjcOiQDPVZYklD7qu
Xmte9GdxL4tSipu0hWxrkRjO7ap09wUoEk1d0jNSrwNbJALnUUkT4qfpYXcMaAhe
okjWSANQaBZxbM0ziEoT
=AnRj
-----END PGP SIGNATURE-----

On 10/11/2012 10:47 AM, Tom Rini wrote:
> On 10/10/12 17:05, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
> 
>> Implement "ls" and "fsload" commands that act like 
>> {fat,ext2}{ls,load}, and transparently handle either file-system.
>>  This scheme could easily be extended to other filesystem types;
>> I only didn't do it for zfs because I don't have any filesystems
>> of that type.
> 
>> Signed-off-by: Stephen Warren <swarren@nvidia.com> --- There are
>> a couple FIXMEs in here:
> 
>> 1) In fs/fs.c, code is ifdef on CONFIG_CMD_FAT or
>> CONFIG_CMD_EXT2. This means that the new commands and code can
>> only be enabled if the "legacy" {fat,ext2}{ls,load} are enabled.
>> What we really want is CONFIG_FS_FAT and CONFIG_FS_EXT2 to enable
>> the filesystem code, and then CONFIG_CMD_FAT, CONFIG_CMD_EXT2,
>> CONFIG_CMD_FS to only affect the command implementations.
>> However, that would require making every include/config/*.h that
>> sets the current defines also set more. I suppose that's a fairly
>> mechanical change though, so easy enough to implement. Does that
>> seem like a reasonable approach to people?
> 
> How about a new CONFIG_CMD_GENERIC_FS for the new ls/fsload (and
> any later commands like write that get added) and once most
> filesystems are converted we can think about a transition plan?

Certainly.

The issue is that the filesystem code itself is conditionally included
based on CONFIG_CMD_FAT and CONFIG_CMD_EXT4. This would require that
in order to use the new generic commands, you also had to support the
old non-generic commands in order to get a linkable result. Perhaps
that's fine for now, but by separating the existing CONFIG_CMD_FAT
into two (CONFIG_CMD_FAT, CONFIG_FS_FAT), we could avoid that
requirement. As Benoit mentioned, perhaps we could arrange that
CONFIG_CMD_FAT selects CONFIG_FS_FAT somehow, although I haven't
looked at U-Boot's configuration system to see if that's possible yet.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/11/12 09:57, Stephen Warren wrote:
> On 10/11/2012 10:47 AM, Tom Rini wrote:
>> On 10/10/12 17:05, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>> 
>>> Implement "ls" and "fsload" commands that act like 
>>> {fat,ext2}{ls,load}, and transparently handle either 
>>> file-system. This scheme could easily be extended to other 
>>> filesystem types; I only didn't do it for zfs because I don't 
>>> have any filesystems of that type.
>> 
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> --- There 
>>> are a couple FIXMEs in here:
>> 
>>> 1) In fs/fs.c, code is ifdef on CONFIG_CMD_FAT or 
>>> CONFIG_CMD_EXT2. This means that the new commands and code can
>>>  only be enabled if the "legacy" {fat,ext2}{ls,load} are 
>>> enabled. What we really want is CONFIG_FS_FAT and 
>>> CONFIG_FS_EXT2 to enable the filesystem code, and then 
>>> CONFIG_CMD_FAT, CONFIG_CMD_EXT2, CONFIG_CMD_FS to only affect 
>>> the command implementations. However, that would require
>>> making every include/config/*.h that sets the current defines
>>> also set more. I suppose that's a fairly mechanical change
>>> though, so easy enough to implement. Does that seem like a
>>> reasonable approach to people?
>> 
>> How about a new CONFIG_CMD_GENERIC_FS for the new ls/fsload (and
>>  any later commands like write that get added) and once most 
>> filesystems are converted we can think about a transition plan?
> 
> Certainly.
> 
> The issue is that the filesystem code itself is conditionally 
> included based on CONFIG_CMD_FAT and CONFIG_CMD_EXT4. This would 
> require that in order to use the new generic commands, you also
> had to support the old non-generic commands in order to get a
> linkable result. Perhaps that's fine for now, but by separating the
> existing CONFIG_CMD_FAT into two (CONFIG_CMD_FAT, CONFIG_FS_FAT),
> we could avoid that requirement. As Benoit mentioned, perhaps we
> could arrange that CONFIG_CMD_FAT selects CONFIG_FS_FAT somehow,
> although I haven't looked at U-Boot's configuration system to see
> if that's possible yet.

Since we don't have Kconfig right now, it's not really easy to do that
kind of thing.  I think once we have more filesystems converted we can
work out doing the mechanical separate of fs/ from common/ for fs code.

- -- 
Tom

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQdvxhAAoJENk4IS6UOR1WOFIP/0VjExqx5FZ+Lau4wFtV8dI0
v5+jfwDUtG0L3h/lTv9NJkX5j9duXJVOcAkB4hVeWFAqd/fSdI0mlajPvR1rpS/J
luYFvR/XdlIoQe65CbrKhFEzHH3pwcbYXd/Zz9wx2cvxmTz7NvbM3KZsvECPpYtO
CEqNClM2e5Q0ebE/XKZ5xG8qJVWZbs48i8kkmgHQHsiplGEdf7IzNgOqiSFL5rgF
DT6oRacoAzOLBwMo5/ssIiT04GgM+3Nac1UV/4GPKiNt62k4p6OfniUN4sL6N6Yl
oq8aFR3ZWGZgHdHO+y+pp+4MLAlvbv8PxMXHhw/ZCCHgLR9sodvWQl/Pn3C9TFTC
0LdYgMvft5FBBqffI+XuPMomdKYLmmHtAhlh7lly9L5GQYw8iU25cfXP8ZeBUOX+
aY4nX/Wp2s+vtXawWUVv01r0HRZZp3r6FJNAXWvE9m2cB+7HiZQD+nJvkV7ZZk7E
6WRdRlI8NTLn6Sfh42d6AzigsYfAm2Xwp6B1/gGix075E30yhB1KGdxs/SpQW+0+
tdtFXij/jtxAyabTLNTivAXO44l6IBurtlAZqXeI1ur6gEJa0L1Ju0Mi6rFO/Vrg
aQ/KBo+1DM4QgXxYkcYPxJ+KIJKQdsmIWEmpni+sgaLBgO43Ebh+lkLKHCm5NB0l
kaIgld2V1OQQDmg/j4yW
=Tegq
-----END PGP SIGNATURE-----

On Thursday, October 11, 2012 7:05:37 PM, Tom Rini wrote:
> On 10/11/12 09:57, Stephen Warren wrote:
> > On 10/11/2012 10:47 AM, Tom Rini wrote:
> >> On 10/10/12 17:05, Stephen Warren wrote:
> >>> From: Stephen Warren <swarren@nvidia.com>
> >> 
> >>> Implement "ls" and "fsload" commands that act like
> >>> {fat,ext2}{ls,load}, and transparently handle either
> >>> file-system. This scheme could easily be extended to other
> >>> filesystem types; I only didn't do it for zfs because I don't
> >>> have any filesystems of that type.
> >> 
> >>> Signed-off-by: Stephen Warren <swarren@nvidia.com> --- There
> >>> are a couple FIXMEs in here:
> >> 
> >>> 1) In fs/fs.c, code is ifdef on CONFIG_CMD_FAT or
> >>> CONFIG_CMD_EXT2. This means that the new commands and code can
> >>>  only be enabled if the "legacy" {fat,ext2}{ls,load} are
> >>> enabled. What we really want is CONFIG_FS_FAT and
> >>> CONFIG_FS_EXT2 to enable the filesystem code, and then
> >>> CONFIG_CMD_FAT, CONFIG_CMD_EXT2, CONFIG_CMD_FS to only affect
> >>> the command implementations. However, that would require
> >>> making every include/config/*.h that sets the current defines
> >>> also set more. I suppose that's a fairly mechanical change
> >>> though, so easy enough to implement. Does that seem like a
> >>> reasonable approach to people?
> >> 
> >> How about a new CONFIG_CMD_GENERIC_FS for the new ls/fsload (and
> >>  any later commands like write that get added) and once most
> >> filesystems are converted we can think about a transition plan?
> > 
> > Certainly.
> > 
> > The issue is that the filesystem code itself is conditionally
> > included based on CONFIG_CMD_FAT and CONFIG_CMD_EXT4. This would
> > require that in order to use the new generic commands, you also
> > had to support the old non-generic commands in order to get a
> > linkable result. Perhaps that's fine for now, but by separating the
> > existing CONFIG_CMD_FAT into two (CONFIG_CMD_FAT, CONFIG_FS_FAT),
> > we could avoid that requirement. As Benoit mentioned, perhaps we
> > could arrange that CONFIG_CMD_FAT selects CONFIG_FS_FAT somehow,
> > although I haven't looked at U-Boot's configuration system to see
> > if that's possible yet.
> 
> Since we don't have Kconfig right now, it's not really easy to do
> that
> kind of thing.  I think once we have more filesystems converted we
> can
> work out doing the mechanical separate of fs/ from common/ for fs
> code.

This does not require Kconfig. include/config_fallbacks.h is already made for
automatic config fixups. The following lines could simply be added to it:

#if defined(CONFIG_CMD_FAT) && !defined(CONFIG_FS_FAT)
#define CONFIG_FS_FAT
#endif

Best regards,
Benoît

Hi

On Wednesday 10 October 2012 18:05:07 Stephen Warren wrote:
>  ...snip... 
>  Makefile        |    3 +-
>  common/Makefile |    2 +
>  common/cmd_fs.c |   86 ++++++++++++++++++++++
>  fs/Makefile     |   47 ++++++++++++
>  fs/fs.c         |  216
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/fs.h    |  
> 49 +++++++++++++
>  6 files changed, 402 insertions(+), 1 deletions(-)
>  create mode 100644 common/cmd_fs.c
>  create mode 100644 fs/Makefile
>  create mode 100644 fs/fs.c
>  create mode 100644 include/fs.h

I dont see how this is a "partition switch library", when it doesnt deal with 
partitions at all. Care to explain? Or maybe call it "filesystem-auto 
detection" or something?

Best Regards
Pavel Herrmann

On 10/13/2012 01:26 PM, Pavel Herrmann wrote:
> Hi
> 
> On Wednesday 10 October 2012 18:05:07 Stephen Warren wrote:
>>  ...snip... 
>>  Makefile        |    3 +-
>>  common/Makefile |    2 +
>>  common/cmd_fs.c |   86 ++++++++++++++++++++++
>>  fs/Makefile     |   47 ++++++++++++
>>  fs/fs.c         |  216
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/fs.h    |  
>> 49 +++++++++++++
>>  6 files changed, 402 insertions(+), 1 deletions(-)
>>  create mode 100644 common/cmd_fs.c
>>  create mode 100644 fs/Makefile
>>  create mode 100644 fs/fs.c
>>  create mode 100644 include/fs.h
> 
> I dont see how this is a "partition switch library", when it doesnt deal with 
> partitions at all. Care to explain? Or maybe call it "filesystem-auto 
> detection" or something?

Oops. I will s/partition/filesystem/ in the patch subject.