[PATCH] fs: fat: Handle 'FAT sector size mismatch'

[Caleb Connolly]

Hi both,

Varadarajan: Thanks for sending this patch.

On 19/11/2024 16:44, Tom Rini wrote:
> On Tue, Nov 19, 2024 at 01:00:48PM +0530, Varadarajan Narayanan wrote:
> 
>> Do FAT read and write based on the device sector size
>> instead of the size recorded in the FAT meta data. This
>> helps to overcome the 'FAT sector size mismatch' error
>> and enables U-Boot to read/write those partitions.

Does this ignore the filesystem sector size or account for it? There's a
whole lot of logic added below which isn't really explained here.
>>
>> Signed-off-by: Varadarajan Narayanan <quic_varada at quicinc.com>
>> ---
>>  fs/fat/fat.c       | 227 ++++++++++++++++++++++++++++++++++++++++++---
>>  fs/fat/fat_write.c |  19 ----
>>  2 files changed, 214 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index e2570e8167..f4bad99335 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -44,24 +44,223 @@ static void downcase(char *str, size_t len)
>>  
>>  static struct blk_desc *cur_dev;
>>  static struct disk_partition cur_part_info;
>> +int fat_sect_size;

This variable should be static, no harm in 0 initializing it here either.
>>  

[...]

>> +
>> +int disk_write(__u32 sect, __u32 nr_sect, void *buf)
>> +{
>> +	int ret;
>> +	__u8 *block = NULL;
>> +	__u32 rem, size;
>> +	__u32 s, n;
>> +
>> +	rem = nr_sect * fat_sect_size;
>> +	/*
>> +	 *           block N       block N + 1      block N + 2
>> +	 * +-+-+--+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>> +	 * | | | | |s|e|c|t|o|r| | |s|e|c|t|o|r| | |s|e|c|t|o|r| | | | |
>> +	 * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>> +	 * . . . |               |               |               | . . .
>> +	 * ------+---------------+---------------+---------------+------
>> +	 *            |<--- FAT reads in sectors          --->|
>> +	 *
>> +	 *            |  part 1  |   part 2      |  part 3    |
>> +	 *
>> +	 */
>> +
>> +	/* Do part 1 */
>> +	if (fat_sect_size) {
>> +		__u32 offset;
>> +
>> +		/* Read one block and overwrite the leading sectors */
>> +		block = malloc_cache_aligned(cur_dev->blksz);
>> +		if (!block) {
>> +			printf("Error: allocating block: %lu\n", cur_dev->blksz);
>> +			return -1;
>> +		}
>> +
>> +		s = sect_to_block(sect, &offset);
>> +		offset = offset * fat_sect_size;
>> +
>> +		ret = blk_dread(cur_dev, cur_part_info.start + s, 1, block);
>> +		if (ret != 1) {
>> +			ret = -1;
>> +			goto exit;
>> +		}
>> +
>> +		if (rem > (cur_part_info.blksz - offset))
>> +			size = cur_part_info.blksz - offset;
>> +		else
>> +			size = rem;
>> +
>> +		memcpy(block + offset, buf, size);
>> +		ret = blk_dwrite(cur_dev, cur_part_info.start + s, 1, block);
>> +		if (ret != 1) {
>> +			ret = -1;
>> +			goto exit;
>> +		}
>> +
>> +		rem -= size;
>> +		buf += size;
>> +		s++;
>> +	} else {
>> +		/*
>> +		 * fat_sect_size not being set implies, this is the first read
>> +		 * to partition. The first sector is being read to get the
>> +		 * FS meta data. The FAT sector size is got from this meta data.
>> +		 */

This is the disk_write() function though, I don't think this behaviour
is correct?
>> +		ret = blk_dread(cur_dev, cur_part_info.start + s, 1, buf);
>> +		if (ret != 1)
>> +			return -1;
>> +	}
>> +
>> +	/* Do part 2, write directly from the given buffer */
>> +	if (rem > cur_part_info.blksz) {
>> +		n = rem / cur_part_info.blksz;
>> +		ret = blk_dwrite(cur_dev, cur_part_info.start + s, n, buf);
>> +		if (ret != n) {
>> +			ret = -1;
>> +			goto exit;
>> +		}
>> +		buf += n * cur_part_info.blksz;
>> +		rem = rem % cur_part_info.blksz;
>> +		s += n;
>> +	}
>> +
>> +	/* Do part 3, read a block and copy the trailing sectors */
>> +	if (rem) {
>> +		ret = blk_dread(cur_dev, cur_part_info.start + s, 1, block);
>> +		if (ret != 1) {
>> +			ret = -1;
>> +			goto exit;
>> +		} else {
>> +			memcpy(block, buf, rem);
>> +		}
>> +		ret = blk_dwrite(cur_dev, cur_part_info.start + s, 1, block);
>> +		if (ret != 1) {
>> +			ret = -1;
>> +			goto exit;
>> +		}
>> +	}
>> +exit:
>> +	if (block)
>> +		free(block);
>> +
>> +	return (ret == -1) ? -1 : nr_sect;
>>  }


[...]

Hi Tom,

> With this patch, we now have
> https://patchwork.ozlabs.org/project/uboot/patch/20241113044956.1836896-1-caleb.connolly@linaro.org/

I suspect my patch would corrupt storage on write, I'll give it another
test and see.
> and
> https://patchwork.ozlabs.org/project/uboot/patch/20230730111516.v2.1.Ia13846500fab3d5a1d5573db11a040d233994fa6@changeid/
> for seemingly the same issue. Can you please try these other two
> patches and report which ones do / don't handle your use case as well?
> Thanks!
Kind regards,
> 

-- 
// Caleb (they/them)