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

[Varadarajan Narayanan]

On Wed, Nov 20, 2024 at 02:55:48PM +0100, Caleb Connolly wrote:
> 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?

Accounts for it.

> There's a whole lot of logic added below which isn't really
> explained here.

Will post a new version with additional explanation.

> >> 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.
> >>

Sure.

> [...]
>
> >> +
> >> +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?

Will clean this.

Thanks for the inputs.

-Varada


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