Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support
On 03/27/2012 08:13 AM, manjunatha wrote: Dear Rob Herring/ Wolfgang, As discussed in earlier mail chain, our current ext4 implementation is capable enough to list(ls) and read(load) ext2 partitons as well. Also the current ext4 implementation is optimized to 7-8 times greater read throughput than ext2load. Cool. IIRC, there was just recently some discussion on the list about ext2 performance problems. I propose that we can remove the existing ext2 code and let it be replaced with ext4. That's exactly what I proposed. Although, I think your patch should be done as modifications to cmd_ext2.c rather than deleting it and adding a new cmd_ext4.c. All users using ext2ls and ext2load command can replace them with ext4ls and ext4load respectively. I think you need to keep the command names to not break existing users' scripts. So either we just stick with ext2ls/ext2load or define additional ext4* commands which call the same functions as the ext2 version. I don't really care, but would lean toward the former. Rob The ext4 code is based on ext2 implementation and we have added the names of respective owners(ext2 code authors) in the file headers. Please suggest what could be the best approach. Thanks Regards, Manjunatha C achar -- From: Rob Herring robherri...@gmail.com Sent: Monday, March 26, 2012 6:40 PM To: uma.shan...@samsung.com Cc: u-boot@lists.denx.de; MANJUNATHAC ACHAR a.manjuna...@samsung.com; kim.phill...@freescale.com; IQBAL SHAREEF iqbal@samsung.com; HAKGOO LEE goodguy@samsung.com; w...@denx.de Subject: Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support On 03/26/2012 02:24 AM, UMA SHANKAR wrote: Dear Rob Herring, Thanks for evaluating and testing our code. Currently, Our ext4 implementation is capable of listing and reading (ls and load) ext2 partitions as well. But, we wanted the ext4 code to be separate from ext2 implementation. Why? Your comment: --- Using ext2 commands on an ext4 partition will hang now. ext2load command will only read ext2 partitions, and will definitely not work on ext4 (even without our implementation). I'm not saying it should work, but it shouldn't hang. IIRC without your patch, it would return with an error. So you have changed something in the core ext2 code that makes it hang. But it's not something I think you need to worry about as the ext4 version of code should just replace the ext2 version and the issue will go away. Rob So, I propose, we can let both these implementations be there in uboot under different CONFIG Options (as is the case currently). Thanks Regards, Uma Shankar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support
Dear Rob Herring/ Wolfgang, As discussed in earlier mail chain, our current ext4 implementation is capable enough to list(ls) and read(load) ext2 partitons as well. Also the current ext4 implementation is optimized to 7-8 times greater read throughput than ext2load. I propose that we can remove the existing ext2 code and let it be replaced with ext4. All users using ext2ls and ext2load command can replace them with ext4ls and ext4load respectively. The ext4 code is based on ext2 implementation and we have added the names of respective owners(ext2 code authors) in the file headers. Please suggest what could be the best approach. Thanks Regards, Manjunatha C achar -- From: Rob Herring robherri...@gmail.com Sent: Monday, March 26, 2012 6:40 PM To: uma.shan...@samsung.com Cc: u-boot@lists.denx.de; MANJUNATHAC ACHAR a.manjuna...@samsung.com; kim.phill...@freescale.com; IQBAL SHAREEF iqbal@samsung.com; HAKGOO LEE goodguy@samsung.com; w...@denx.de Subject: Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support On 03/26/2012 02:24 AM, UMA SHANKAR wrote: Dear Rob Herring, Thanks for evaluating and testing our code. Currently, Our ext4 implementation is capable of listing and reading (ls and load) ext2 partitions as well. But, we wanted the ext4 code to be separate from ext2 implementation. Why? Your comment: --- Using ext2 commands on an ext4 partition will hang now. ext2load command will only read ext2 partitions, and will definitely not work on ext4 (even without our implementation). I'm not saying it should work, but it shouldn't hang. IIRC without your patch, it would return with an error. So you have changed something in the core ext2 code that makes it hang. But it's not something I think you need to worry about as the ext4 version of code should just replace the ext2 version and the issue will go away. Rob So, I propose, we can let both these implementations be there in uboot under different CONFIG Options (as is the case currently). Thanks Regards, Uma Shankar ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support
On 01/09/2012 11:54 AM, uma.shan...@samsung.com wrote:
From: uma.shankar uma.shan...@samsung.com
Signed-off-by: Uma Shankar uma.shan...@samsung.com
Signed-off-by: Manjunatha C Achar a.manjuna...@samsung.com
Signed-off-by: Iqbal Shareef iqbal@samsung.com
Signed-off-by: Hakgoo Lee goodguy@samsung.com
---
Changes for v3:
- Copyright has been updated in respective files
- ext4fs has been made independant of ext2fs.c
- Fixed API namespace
- Removed endianness conversion API, used uboot defined API
for the same
- Fixed coding style issues
- Moved README.ext4 file into doc folder
Changes for v2:
- Code cleanup, changed comment style
- camel case removed, resolved code alignment issues
- memory allocation logic changed, removed busybox logic
- Modified ext4 load to remove grub dependency (GPLv3)
- Introduced new Config for ext4 write
Changes for v1:
- Removed checkpatch warnings and errors
- Moved common API's of ext2 and ext4 to one generic header file
snip
+static int do_ext4_load(cmd_tbl_t *cmdtp, int flag, int argc,
+ char *const argv[])
+{
+ char *filename = NULL;
+ char *ep;
+ int dev;
+ unsigned long part = 1;
+ ulong addr = 0;
+ ulong part_length;
+ int filelen;
+ disk_partition_t info;
+ struct ext_filesystem *fs;
+ char buf[12];
+ unsigned long count;
+ const char *addr_str;
+
+ count = 0;
+ addr = simple_strtoul(argv[3], NULL, 16);
+ filename = getenv(bootfile);
+ switch (argc) {
+ case 3:
+ addr_str = getenv(loadaddr);
+ if (addr_str != NULL)
+ addr = simple_strtoul(addr_str, NULL, 16);
+ else
+ addr = CONFIG_SYS_LOAD_ADDR;
+
+ break;
+ case 4:
+ break;
+ case 5:
+ filename = argv[4];
+ break;
+ case 6:
+ filename = argv[4];
+ count = simple_strtoul(argv[5], NULL, 16);
+ break;
+
+ default:
+ return cmd_usage(cmdtp);
+ }
+
+ if (!filename) {
+ puts(** No boot file defined **\n);
+ return 1;
+ }
+
+ dev = (int)simple_strtoul(argv[2], ep, 16);
+ ext4_dev_desc = get_dev(argv[1], dev);
+ if (ext4_dev_desc == NULL) {
+ printf(** Block device %s %d not supported\n, argv[1], dev);
+ return 1;
+ }
+ if (init_fs(ext4_dev_desc))
+ return 1;
+
+ fs = get_fs();
These functions pollute the name space and could be combined.
Or can't these 2 calls be rolled into...
+ if (*ep) {
+ if (*ep != ':') {
+ puts(** Invalid boot device, use `dev[:part]' **\n);
+ return 1;
+ }
+ part = simple_strtoul(++ep, NULL, 16);
+ }
+
+ if (part != 0) {
+ if (get_partition_info(fs-dev_desc, part, info)) {
+ printf(** Bad partition %lu **\n, part);
+ return 1;
+ }
+
+ if (strncmp((char *)info.type, BOOT_PART_TYPE,
+ strlen(BOOT_PART_TYPE)) != 0) {
+ printf(** Invalid partition type \%s\
+ (expect \ BOOT_PART_TYPE \)\n, info.type);
+ return 1;
+ }
+ printf(Loading file \%s\
+from %s device %d:%lu %s\n,
+filename, argv[1], dev, part, info.name);
+ } else {
+ printf(Loading file \%s\ from %s device %d\n,
+filename, argv[1], dev);
+ }
+
+ part_length = ext4fs_set_blk_dev(fs-dev_desc, part);
...this function?
+ if (part_length == 0) {
+ printf(**Bad partition - %s %d:%lu **\n, argv[1], dev, part);
+ ext4fs_close();
You are leaking memory allocated in init_fs here.
+ return 1;
+ }
+
+ if (!ext4fs_mount(part_length)) {
+ printf(** Bad ext2 partition or disk - %s %d:%lu **\n,
+argv[1], dev, part);
+ ext4fs_close();
and here...
+ return 1;
+ }
+
+ filelen = ext4fs_open(filename);
+ if (filelen 0) {
+ printf(** File not found %s\n, filename);
+ ext4fs_close();
and here...
+ return 1;
+ }
+ if ((count filelen) (count != 0))
+ filelen = count;
+
+ if (ext4fs_read((char *)addr, filelen) != filelen) {
+ printf(** Unable to read \%s\ from %s %d:%lu **\n,
+filename, argv[1], dev, part);
+ ext4fs_close();
and here...
Same comments apply in do_ext4_ls.
Rob
___
U-Boot mailing
Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support
Hi Manjunatha Rob On Wed, Mar 28, 2012 at 12:46 AM, Rob Herring robherri...@gmail.com wrote: On 03/27/2012 08:13 AM, manjunatha wrote: Dear Rob Herring/ Wolfgang, As discussed in earlier mail chain, our current ext4 implementation is capable enough to list(ls) and read(load) ext2 partitons as well. Also the current ext4 implementation is optimized to 7-8 times greater read throughput than ext2load. Cool. IIRC, there was just recently some discussion on the list about ext2 performance problems. I am very keen to see this ext4 support hit maturity as I too am seeing sub-par performance reading from an ext2 filesystem on an SD card. (I'm a bit sad that I simply have not had the time to test drive it myself) I propose that we can remove the existing ext2 code and let it be replaced with ext4. That's exactly what I proposed. Although, I think your patch should be done as modifications to cmd_ext2.c rather than deleting it and adding a new cmd_ext4.c That depends on what the delta between the ext2 and ext4 code is. If the delta is large (i.e. the patches are not intuitive to read) then I would suggest the following sequence: 1. Introduce the ext4 code with dedicated ext4ls, ext4load and ext4write functions as 100% seperate code (i.e. do not modify existing ext2 code) 2. Delete the ext2 code and modify ext2ls and ext2load to use the new ext4 functions (also add ext2write assuming these patches support writing to ext2) All users using ext2ls and ext2load command can replace them with ext4ls and ext4load respectively. I think you need to keep the command names to not break existing users' scripts. So either we just stick with ext2ls/ext2load or define additional ext4* commands which call the same functions as the ext2 version. I don't really care, but would lean toward the former. As above - I don't see using ext2* to access an ext4 filesystem as very intuitive (or ext4* to access an ext2 filesystem) - The user does not need to know what happens under the hood, but the command interface should always make sense. Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support
Dear UMA SHANKAR, please do not top post / full quote. In message 27542660.71361332746650945.JavaMail.weblogic@epml04 you wrote: Currently, Our ext4 implementation is capable of listing and reading (ls and load) ext2 partitions as well. But, we wanted the ext4 code to be separate from ext2 implementation. This does not make much sense to me. So, I propose, we can let both these implementations be there in uboot under different CONFIG Options (as is the case currently). What would be the rationale here? I think more and more people are going to use ext4. It makes no sense to have different commands and different options to support both ext2 (and ext3, to some degree) and ext4 (and ext3?) file systems. And especially, it makes no sense to dusplicate large amounts of code. Please use common code whererver possible. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de I knew then (in 1970) that a 4-kbyte minicomputer would cost as much as a house. So I reasoned that after college, I'd have to live cheaply in an apartment and put all my money into owning a computer. - Apple co-founder Steve Wozniak, EE Times, June 6, 1988, pg 45 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support
On 03/26/2012 02:24 AM, UMA SHANKAR wrote: Dear Rob Herring, Thanks for evaluating and testing our code. Currently, Our ext4 implementation is capable of listing and reading (ls and load) ext2 partitions as well. But, we wanted the ext4 code to be separate from ext2 implementation. Why? Your comment: --- Using ext2 commands on an ext4 partition will hang now. ext2load command will only read ext2 partitions, and will definitely not work on ext4 (even without our implementation). I'm not saying it should work, but it shouldn't hang. IIRC without your patch, it would return with an error. So you have changed something in the core ext2 code that makes it hang. But it's not something I think you need to worry about as the ext4 version of code should just replace the ext2 version and the issue will go away. Rob So, I propose, we can let both these implementations be there in uboot under different CONFIG Options (as is the case currently). Thanks Regards, Uma Shankar --- Original Message --- Sender : Rob Herringrobherri...@gmail.com Date : Mar 22, 2012 21:19 (GMT+05:30) Title : Re: [U-Boot] [PATCH V4 1/2] ext4fs ls load support On 01/09/2012 11:54 AM, uma.shan...@samsung.com wrote: From: uma.shankar Signed-off-by: Uma Shankar Signed-off-by: Manjunatha C Achar Signed-off-by: Iqbal Shareef Signed-off-by: Hakgoo Lee --- Changes for v3: - Copyright has been updated in respective files - ext4fs has been made independant of ext2fs.c - Fixed API namespace - Removed endianness conversion API, used uboot defined API for the same - Fixed coding style issues - Moved README.ext4 file into doc folder Changes for v2: - Code cleanup, changed comment style - camel case removed, resolved code alignment issues - memory allocation logic changed, removed busybox logic - Modified ext4 load to remove grub dependency (GPLv3) - Introduced new Config for ext4 write Changes for v1: - Removed checkpatch warnings and errors - Moved common API's of ext2 and ext4 to one generic header file Makefile |2 +- common/Makefile |1 + common/cmd_ext4.c | 266 +++ I don't think we need yet another version of filesystem commands that just duplicates the same code. I tested this patch and can read ext2 partitions with the ext4ls/load commands. Using ext2 commands on an ext4 partition will hang now. You should merge your ext4 command changes into the existing ext2 commands so that commands work on either ext2/3 or ext4 partitions. Really, I would like to see all the block based device and file commands collapsed down into just fsls and fsload commands and then detect (or specify if necessary) the fs type. But that's a bigger problem. Rob fs/Makefile |1 + fs/ext2/dev.c |1 + fs/ext2/ext2fs.c | 181 ++- fs/ext4/Makefile | 51 +++ fs/ext4/dev.c | 145 fs/ext4/ext4_common.c | 875 + fs/ext4/ext4_common.h | 63 fs/ext4/ext4fs.c | 228 + include/ext4fs.h | 132 include/ext_common.h | 188 +++ 13 files changed, 1977 insertions(+), 157 deletions(-) create mode 100644 common/cmd_ext4.c create mode 100644 fs/ext4/Makefile create mode 100644 fs/ext4/dev.c create mode 100644 fs/ext4/ext4_common.c create mode 100644 fs/ext4/ext4_common.h create mode 100644 fs/ext4/ext4fs.c create mode 100644 include/ext4fs.h create mode 100644 include/ext_common.h diff --git a/Makefile b/Makefile index 7e9491d..e77f6be 100644 --- a/Makefile +++ b/Makefile @@ -235,7 +235,7 @@ LIBS += dts/libdts.o endif LIBS += arch/$(ARCH)/lib/lib$(ARCH).o LIBS += fs/cramfs/libcramfs.o fs/fat/libfat.o fs/fdos/libfdos.o fs/jffs2/libjffs2.o \ - fs/reiserfs/libreiserfs.o fs/ext2/libext2fs.o fs/yaffs2/libyaffs2.o \ + fs/reiserfs/libreiserfs.o fs/ext2/libext2fs.o fs/ext4/libext4fs.o fs/yaffs2/libyaffs2.o \ fs/ubifs/libubifs.o LIBS += net/libnet.o LIBS += disk/libdisk.o diff --git a/common/Makefile b/common/Makefile index 2d9ae8c..f5243f6 100644 --- a/common/Makefile +++ b/common/Makefile @@ -86,6 +86,7 @@ COBJS-$(CONFIG_ENV_IS_IN_EEPROM) += cmd_eeprom.o COBJS-$(CONFIG_CMD_EEPROM) += cmd_eeprom.o COBJS-$(CONFIG_CMD_ELF) += cmd_elf.o COBJS-$(CONFIG_SYS_HUSH_PARSER) += cmd_exit.o +COBJS-$(CONFIG_CMD_EXT4) += cmd_ext4.o COBJS-$(CONFIG_CMD_EXT2) += cmd_ext2.o COBJS-$(CONFIG_CMD_FAT) += cmd_fat.o COBJS-$(CONFIG_CMD_FDC)$(CONFIG_CMD_FDOS) += cmd_fdc.o diff --git a/common/cmd_ext4.c b/common/cmd_ext4.c new file mode 100644 index 000..2c53d2c --- /dev/null +++ b/common/cmd_ext4.c @@ -0,0 +1,266 @@ +/* + * (C) Copyright 2011 - 2012 Samsung Electronics + * EXT4 filesystem implementation in Uboot by + * Uma Shankar + * Manjunatha C Achar + * + * Ext4fs support + * made from existing cmd_ext2.c file of Uboot + * + * (C) Copyright 2004 + * esd gmbh + * Reinhard Arlt + * + * made from
