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 <[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. > >> 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/[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) >
