Re: [U-Boot] [PATCH V2 3/3] FAT: implement fat_set_blk_dev(), convert cmd_fat.c

Hi Stephen,

See my comments inlined below.

On Tuesday, October 9, 2012 10:41:41 PM, Stephen Warren wrote:

This makes the FAT filesystem API more consistent with other block-based filesystems. If in the future standard multi-filesystem commands such as "ls" or "load" are implemented, having FAT work the same way as other filesystems will be necessary.

Convert cmd_fat.c to the new API, so the code looks more like other files implementing the same commands for other filesystems.

Signed-off-by: Stephen Warren swarren@nvidia.com

v2: Fix inverted test of get_partition_info() result in fat_register_device().

---

common/cmd_fat.c | 8 +++--- fs/fat/fat.c | 65 ++++++++++++++++++++++++++--------------------------- include/fat.h | 1 + 3 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/common/cmd_fat.c b/common/cmd_fat.c index 5a5698b..c38302d 100644 --- a/common/cmd_fat.c +++ b/common/cmd_fat.c @@ -55,7 +55,7 @@ int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1;

dev = dev_desc->dev;

• if (fat_register_device(dev_desc,part)!=0) {

• if (fat_set_blk_dev(dev_desc, &info) != 0) { printf("\n** Unable to use %s %d:%d for fatload **\n", argv[1], dev, part); return 1;

@@ -111,7 +111,7 @@ int do_fat_ls (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1;

dev = dev_desc->dev;

• if (fat_register_device(dev_desc,part)!=0) {

• if (fat_set_blk_dev(dev_desc, &info) != 0) { printf("\n** Unable to use %s %d:%d for fatls **\n", argv[1], dev, part); return 1;

@@ -149,7 +149,7 @@ int do_fat_fsinfo (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return 1;

dev = dev_desc->dev;

• if (fat_register_device(dev_desc,part)!=0) {

• if (fat_set_blk_dev(dev_desc, &info) != 0) { printf("\n** Unable to use %s %d:%d for fatinfo **\n", argv[1], dev, part); return 1;

@@ -185,7 +185,7 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag,

dev = dev_desc->dev;

• if (fat_register_device(dev_desc, part) != 0) {

• if (fat_set_blk_dev(dev_desc, &info) != 0) { printf("\n** Unable to use %s %d:%d for fatwrite **\n", argv[1], dev, part); return 1;

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 1e0d2a3..79e66ce 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -61,42 +61,12 @@ static int disk_read(__u32 block, __u32 nr_blocks, void *buf) cur_part_info.start + block, nr_blocks, buf); }

-int fat_register_device(block_dev_desc_t * dev_desc, int part_no) +int fat_set_blk_dev(block_dev_desc_t *dev_desc, disk_partition_t *info) { ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);

• /* First close any currently found FAT filesystem */
• cur_dev = NULL;

-#if (defined(CONFIG_CMD_IDE) || \

• defined(CONFIG_CMD_SATA) || \
  

• defined(CONFIG_CMD_SCSI) || \
  

• defined(CONFIG_CMD_USB) || \
  

• defined(CONFIG_MMC) || \
  

• defined(CONFIG_SYSTEMACE) )
  

• /* Read the partition table, if present */
• if (!get_partition_info(dev_desc, part_no, &cur_part_info)) {

• cur_dev = dev_desc;
  

• }

-#endif

• /* Otherwise it might be a superfloppy (whole-disk FAT filesystem)

*/

• if (!cur_dev) {

• if (part_no != 0) {
  

• 	printf("** Partition %d not valid on device %d **\n",
  

• 			part_no, dev_desc->dev);
  

• 	return -1;
  

• }
  

• cur_dev = dev_desc;
  

• cur_part_info.part = 0;
  

• cur_part_info.start = 0;
  

• cur_part_info.size = dev_desc->lba;
  

• cur_part_info.blksz = dev_desc->blksz;
  

• memset(cur_part_info.name, 0, sizeof(cur_part_info.name));
  

• memset(cur_part_info.type, 0, sizeof(cur_part_info.type));
  

• }

• cur_dev = dev_desc;

• cur_part_info = *info;

  /* Make sure it has a valid FAT header */ if (disk_read(0, 1, buffer) != 1) {

@@ -120,6 +90,35 @@ int fat_register_device(block_dev_desc_t * dev_desc, int part_no) return -1; }

+int fat_register_device(block_dev_desc_t *dev_desc, int part_no) +{

• disk_partition_t info;
• /* First close any currently found FAT filesystem */
• cur_dev = NULL;
• /* Read the partition table, if present */
• if (get_partition_info(dev_desc, part_no, &info)) {

• if (part_no != 0) {
  

• 	printf("** Partition %d not valid on device %d **\n",
  

• 			part_no, dev_desc->dev);
  

• 	return -1;
  

• }
  

• info.part = 0;
  

• info.start = 0;
  

• info.size = dev_desc->lba;
  

• info.blksz = dev_desc->blksz;
  

• memset(info.name, 0, sizeof(info.name));
  

• memset(info.type, 0, sizeof(info.type));
  

+#ifdef CONFIG_PARTITION_UUIDS

• memset(info.uuid, 0, sizeof(info.uuid));
  

+#endif

• info.bootable = 0;
  

Sorry for pointing this out only now, but now that cmd_fat.c has been switched to fat_set_blk_dev(), the code above is handled for this file by get_device_and_partition(): --- info->start = 0; info->size = (*dev_desc)->lba; info->blksz = (*dev_desc)->blksz; info->bootable = 0; #ifdef CONFIG_PARTITION_UUIDS info->uuid[0] = 0; #endif ---

This code does not initialize the fields part, name and type, and it clears only the 1st byte of the uuid field. Is this on purpose for some reason, or is this an issue?

• }
• return fat_set_blk_dev(dev_desc, &info);

+}

/*

• Get the first occurence of a directory delimiter ('/' or '') in

a string. diff --git a/include/fat.h b/include/fat.h index cc85b06..706cd7a 100644 --- a/include/fat.h +++ b/include/fat.h @@ -212,6 +212,7 @@ long file_fat_read_at(const char *filename, unsigned long pos, void *buffer, unsigned long maxsize); long file_fat_read(const char *filename, void *buffer, unsigned long maxsize); const char *file_getfsname(int idx); +int fat_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info); int fat_register_device(block_dev_desc_t *dev_desc, int part_no);

int file_fat_write(const char *filename, void *buffer, unsigned long maxsize);

Apart from that, this new version of this series looks good.

Best regards, Benoît