Re: [PATCH v1 2/2] fs: fat: calculate FAT type based on cluster count

[Sean Anderson <seanga2@gmail.com>]

On 11/8/23 07:12, christian.taedcke-oss@weidmueller.com wrote:
> From: Christian Taedcke <christian.taedcke@weidmueller.com>
> 
> This fixes an issue where the FAT type (FAT12, FAT16) is not
> correctly detected, e.g. when the BPB field BS_FilSysType contains the
> valid value "FAT     ".
> 
> According to the FAT spec the field BS_FilSysType has only
> informational character and does not determine the FAT type.
> 
> The logic of this code is based on the linux kernel implementation
> from the file fs/fat/inode.c function fat_fill_super().
> 
> For details about FAT see http://elm-chan.org/docs/fat_e.html
> 
> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>

Have you run bloat-o-meter on this? FAT is used in SPL a lot.

Would it be possible to write a test for this, especially for a FAT12 filesystem with
4084 data sectors and a FAT16 filesystem with 4085?

> ---
> 
>   fs/fat/fat.c             | 58 +++++++++++++++++++++++-----------------
>   include/fat.h            |  6 -----
>   test/image/spl_load_fs.c |  2 +-
>   3 files changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 8a0f4e4e6c..9179257ff2 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -27,6 +27,9 @@
>   
>   #define FATU8ARRAY2CPU16(x) (((x)[1] << 8) + (x)[0])
>   
> +/* maximum number of clusters for FAT12 */
> +#define MAX_FAT12	0xFF4
> +
>   /*
>    * Convert a string to lowercase.  Converts at most 'len' characters,
>    * 'len' may be larger than the length of 'str' if 'str' is NULL
> @@ -485,6 +488,32 @@ static __u8 mkcksum(struct nameext *nameext)
>   	return ret;
>   }
>   
> +/*
> + * Determine the FAT type
> + *
> + * Based on fat_fill_super() from the Linux kernel's fs/fat/inode.c
> + */
> +static int
> +determine_fat_bits(const boot_sector *bs)
> +{
> +	if (!bs->fat_length && bs->fat32_length) {
> +		return 32;
> +	} else {
> +		u16 fat_start = bs->reserved;
> +		u32 dir_start = fat_start + bs->fats * bs->fat_length;
> +		u32 rootdir_sectors = FATU8ARRAY2CPU16(bs->dir_entries) *
> +				      sizeof(dir_entry) /
> +				      FATU8ARRAY2CPU16(bs->sector_size);

Note that technically we are supposed to round up.

> +		u32 data_start = dir_start + rootdir_sectors;
> +		u16 sectors = FATU8ARRAY2CPU16(bs->sectors);
> +		u32 total_sectors = sectors ? sectors : bs->total_sect;
> +		u32 total_clusters = (total_sectors - data_start) /
> +				     bs->cluster_size;
> +
> +		return (total_clusters > MAX_FAT12) ? 16 : 12;
> +	}
> +}
> +

This should be merged with...

>   /*
>    * Read boot sector and volume info from a FAT filesystem
>    */
> @@ -493,7 +522,6 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   {
>   	__u8 *block;
>   	volume_info *vistart;
> -	int ret = 0;
>   
>   	if (cur_dev == NULL) {
>   		debug("Error: no device selected\n");
> @@ -508,7 +536,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   
>   	if (disk_read(0, 1, block) < 0) {
>   		debug("Error: reading block\n");
> -		goto fail;
> +		free(block);
> +		return -1;
>   	}
>   
>   	memcpy(bs, block, sizeof(boot_sector));
> @@ -527,33 +556,15 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize)
>   		bs->info_sector = FAT2CPU16(bs->info_sector);
>   		bs->backup_boot = FAT2CPU16(bs->backup_boot);
>   		vistart = (volume_info *)(block + sizeof(boot_sector));
> -		*fatsize = 32;
>   	} else {
>   		vistart = (volume_info *)&(bs->fat32_length);
> -		*fatsize = 0;
>   	}

... these if statements (above)

>   	memcpy(volinfo, vistart, sizeof(volume_info));
>   
> -	if (*fatsize == 32) {
> -		if (strncmp(FAT32_SIGN, vistart->fs_type, SIGNLEN) == 0)
> -			goto exit;
> -	} else {
> -		if (strncmp(FAT12_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> -			*fatsize = 12;
> -			goto exit;
> -		}
> -		if (strncmp(FAT16_SIGN, vistart->fs_type, SIGNLEN) == 0) {
> -			*fatsize = 16;
> -			goto exit;
> -		}
> -	}

I think we should still verify these as long as vistart->ext_boot_sign is 0x29.

--Sean

> +	*fatsize = determine_fat_bits(bs);
>   
> -	debug("Error: broken fs_type sign\n");
> -fail:
> -	ret = -1;
> -exit:
>   	free(block);
> -	return ret;
> +	return 0;
>   }
>   
>   static int get_fs_info(fsdata *mydata)
> @@ -1158,9 +1169,8 @@ int file_fat_detectfs(void)
>   
>   	memcpy(vol_label, volinfo.volume_label, 11);
>   	vol_label[11] = '\0';
> -	volinfo.fs_type[5] = '\0';
>   
> -	printf("Filesystem: %s \"%s\"\n", volinfo.fs_type, vol_label);
> +	printf("Filesystem: FAT%d \"%s\"\n", fatsize, vol_label);
>   
>   	return 0;
>   }
> diff --git a/include/fat.h b/include/fat.h
> index a9756fb4cd..3dce99a23c 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -34,12 +34,6 @@ struct disk_partition;
>   /* Maximum number of entry for long file name according to spec */
>   #define MAX_LFN_SLOT	20
>   
> -/* Filesystem identifiers */
> -#define FAT12_SIGN	"FAT12   "
> -#define FAT16_SIGN	"FAT16   "
> -#define FAT32_SIGN	"FAT32   "
> -#define SIGNLEN		8
> -
>   /* File attributes */
>   #define ATTR_RO	1
>   #define ATTR_HIDDEN	2
> diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
> index 297ab08a82..90e640b5de 100644
> --- a/test/image/spl_load_fs.c
> +++ b/test/image/spl_load_fs.c
> @@ -220,7 +220,7 @@ static size_t create_fat(void *dst, size_t size, const char *filename,
>   	bs->root_cluster = cpu_to_le32(root_sector);
>   
>   	vi->ext_boot_sign = 0x29;
> -	memcpy(vi->fs_type, FAT32_SIGN, sizeof(vi->fs_type));
> +	memcpy(vi->fs_type, "FAT32   ", sizeof(vi->fs_type));
>   
>   	memcpy(dst + 0x1fe, "\x55\xAA", 2);
>   

--Sean