[U-Boot] [PATCH V2 1/3] part: add partition number to disk_partition_t

[U-Boot] [PATCH V2 1/3] part: add partition number to disk_partition_t

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The FAT filesystem code knows which partition ID it is operating on.
Currently, this is passed to fat_register_device() as a parameter.
In order to convert FAT to the more standardized fat_set_blk_dev(), the
information needs to come from somewhere else, and the partition
definition structure is the logical place.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: No change.
---
 disk/part.c    |    1 +
 include/part.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/disk/part.c b/disk/part.c
index a0c77dd..2bc7acb 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -391,6 +391,7 @@ int get_partition_info(block_dev_desc_t *dev_desc, int part
 	defined(CONFIG_MMC) || \
 	defined(CONFIG_SYSTEMACE)
 
+	info->part = part;
 #ifdef CONFIG_PARTITION_UUIDS
 	/* The common case is no UUID support */
 	info->uuid[0] = 0;
diff --git a/include/part.h b/include/part.h
index 27ea283..ebdebd8 100644
--- a/include/part.h
+++ b/include/part.h
@@ -88,6 +88,7 @@ typedef struct block_dev_desc {
 #define DEV_TYPE_OPDISK		0x07	/* optical disk */
 
 typedef struct disk_partition {
+	int	part;		/* Partition number			*/
 	ulong	start;		/* # of first block in partition	*/
 	ulong	size;		/* number of blocks in partition	*/
 	ulong	blksz;		/* block size in bytes			*/
-- 
1.7.0.4

[U-Boot] [PATCH V2 2/3] FAT: make use of disk_partition_t.part

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This removes the standalone cur_part_nr variable, opening the way to
replacing fat_register_device() with fat_set_blk_dev().

Note that when get_partition_info() fails and we use the entire disk,
the correct partition number is 0 (whole disk) not 1 (first partition),
so that change is also made here.

An alternative to this (and the previous) patch might be to simply
remove the partition number from the printf(). I don't know how attached
people are to it.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: No change.
---
 fs/fat/fat.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 80156c8..1e0d2a3 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -46,7 +46,6 @@ static void downcase(char *str)
 }
 
 static block_dev_desc_t *cur_dev;
-static unsigned int cur_part_nr;
 static disk_partition_t cur_part_info;
 
 #define DOS_BOOT_MAGIC_OFFSET	0x1fe
@@ -79,7 +78,6 @@ int fat_register_device(block_dev_desc_t * dev_desc, int part_no)
 	/* Read the partition table, if present */
 	if (!get_partition_info(dev_desc, part_no, &cur_part_info)) {
 		cur_dev = dev_desc;
-		cur_part_nr = part_no;
 	}
 #endif
 
@@ -92,7 +90,7 @@ int fat_register_device(block_dev_desc_t * dev_desc, int part_no)
 		}
 
 		cur_dev = dev_desc;
-		cur_part_nr = 1;
+		cur_part_info.part = 0;
 		cur_part_info.start = 0;
 		cur_part_info.size = dev_desc->lba;
 		cur_part_info.blksz = dev_desc->blksz;
@@ -1235,7 +1233,7 @@ int file_fat_detectfs(void)
 	vol_label[11] = '\0';
 	volinfo.fs_type[5] = '\0';
 
-	printf("Partition %d: Filesystem: %s \"%s\"\n", cur_part_nr,
+	printf("Partition %d: Filesystem: %s \"%s\"\n", cur_part_info.part,
 		volinfo.fs_type, vol_label);
 
 	return 0;
-- 
1.7.0.4

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

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

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;
+	}
+
+	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);
-- 
1.7.0.4

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

[Benoît Thébaudeau]

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

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

[Stephen Warren]

On 10/09/2012 05:06 PM, Beno?t Th?baudeau wrote:
> 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.

>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c

>> +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?

get_device_and_partition() should set indeed set info->part in the
whole-disk case; I'll fix that.

I think get_device_and_partition() never cleared name or type in this
case; I guess that's a pre-existing inconsistency between the two pieces
of code? I'll have a look and add a cleanup patch to make the code
consistent.

The uuid field is a string, so clearing just the first byte is fine; I
ended up using memset in the new code in this patch since the existing
fat_register_device() implementation used memset() on name and type;
something I imagine is not strictly necessary. Since the function is
being re-written anyway, I'll take a look to make sure that clearing
only the first byte is fine, and remove all the memsets and just zero
out the first byte.