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

[Varadarajan Narayanan]

On Wed, Nov 20, 2024 at 10:13:24PM +0530, Varadarajan Narayanan wrote:
> 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.

Have posted v2 addressing the comments. Please take a look.

Skipped initializing the variable to zero due to this checkpatch
error.
	-------------------------------------
	ERROR: do not initialise statics to 0
	#32: FILE: fs/fat/fat.c:47:
	+static int fat_sect_size = 0;
	-------------------------------------

Thanks
Varada

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