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 <[email protected]> >> --- >> 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/[email protected]/ 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)
