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_var...@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.conno...@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)
> >
