[U-Boot] [PATCH 2/2] fs/fat/fat: Optimizes memory size with single global variable instead of 3

[Tien Fong Chee]

On Fri, 2016-07-15 at 01:37 +0200, Benoît Thébaudeau wrote:
> Dear Tien Fong,
>
> On Thu, Jul 14, 2016 at 12:48 PM, Tien Fong Chee <tfchee at altera.com>
> wrote:
> > Dear Benoît,
> >
> > On Wed, 2016-07-13 at 12:56 +0200, Benoît Thébaudeau wrote:
> > > Dear Tien Fong Chee,
> > >
> > > On Jul 13, 2016 at 11:01 AM, Tien Fong Chee wrote:
> > > > Single 64KB get_contents_vfatname_block global variable would
> > > > be
> > > > used for
> > > > all FAT implementation instead of allocating additional two
> > > > global
> > > > variables
> > > > which are get_denfromdir_block and do_fat_read_at_block. This
> > > > implementation
> > > > can help in saving up 128KB memory space.
> > > >
> > > > Signed-off-by: Tien Fong Chee <tfchee at altera.com>
> > > > Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
> > > > Cc: Dinh Nguyen <dinh.linux at gmail.com>
> > > > Cc: ChinLiang <clsee at altera.com>
> > > > Cc: Vagrant Cascadian <vagrant at debian.org>
> > > > Cc: Simon Glass <sjg at chromium.org>
> > > > Cc: Stephen Warren <swarren at nvidia.com>
> > > > Cc: Benoît Thébaudeau <benoit at wsystem.com>
> > > > ---
> > > >  fs/fat/fat.c |    6 ++----
> > > >  1 files changed, 2 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > > > index 826bd85..5d1afe6 100644
> > > > --- a/fs/fat/fat.c
> > > > +++ b/fs/fat/fat.c
> > > > @@ -579,8 +579,7 @@ static __u8 mkcksum(const char name[8],
> > > > const
> > > > char ext[3])
> > > >   * Get the directory entry associated with 'filename' from the
> > > > directory
> > > >   * starting at 'startsect'
> > > >   */
> > > > -__u8 get_dentfromdir_block[MAX_CLUSTSIZE]
> > > > -   __aligned(ARCH_DMA_MINALIGN);
> > > > +__u8 *get_dentfromdir_block = get_contents_vfatname_block;
> > > >
> > > >  static dir_entry *get_dentfromdir(fsdata *mydata, int
> > > > startsect,
> > > >                               char *filename, dir_entry
> > > > *retdent,
> > > > @@ -811,8 +810,7 @@ exit:
> > > >     return ret;
> > > >  }
> > > >
> > > > -__u8 do_fat_read_at_block[MAX_CLUSTSIZE]
> > > > -   __aligned(ARCH_DMA_MINALIGN);
> > > > +__u8 *do_fat_read_at_block = get_contents_vfatname_block;
> > > >
> > > >  int do_fat_read_at(const char *filename, loff_t pos, void
> > > > *buffer,
> > > >                loff_t maxsize, int dols, int dogetsize, loff_t
> > > > *size)
> > >
> > > This probably breaks at least fat_write.c, which uses:
> > >   memcpy(get_dentfromdir_block, get_contents_vfatname_block,
> >
> > With this patch, this line code "memcpy(get_dentfromdir_block,
> > get_contents_vfatname_block," is not required anymore since both
> >  get_dentfromdir_block and get_contents_vfatname_block are sharing
> > the
> > same content and memory. So, this line code can be removed or
> > leaving
> > in there. However, there is only one place within
> > fill_dir_slot_buffer
> > function where it can corrupt the the global memory, and it is
> > fixed by
> > replacing with local buffer. This was sent out with another patch
> > called "[PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to
> > store
> > dir_slot entries". Overall, applying these two patches, a lot
> > memory
> > space can be saved and fitting into small OCRAM, but need to be
> > very
> > careful when modifying the code related to global memory.
>
> I get the point, but I am a bit concerned because these changes make
> the code even more fragile and hard to maintain than it currently is.
Yeah, i agree with you, and there is trade-off in saving the memory
space.
> Perhaps it would be time to switch to FatFs as previously suggested.
> Here is its memory usage:
> http://elm-chan.org/fsw/ff/en/appnote.html#memory
>
Cool. If i am not mistaken, this implementation would impact a lot of
areas, especially interface level. What's about the testing? I am still
voting for this simple patch changes, until we have enough resources to
do the switching.
> I've not checked in detail all the possible code paths, but for
> instance, if I look at get_vfatname(), it's called twice in fat.c,
> once with cluster and retdent pointing to somewhere in
> get_dentfromdir_block, and once with them pointing to somewhere in
> do_fat_read_at_block (through other pointer variables), while
> get_vfatname() may write to get_contents_vfatname_block.
> get_vfatname() then uses the original data pointed to by slotptr >=
> retdent. To make things worse, there is a memcpy (not memmove) from
> realdent, which may point to somewhere in
> get_contents_vfatname_block,
> to retdent. It's almost impossible for the involved buffer areas not
> to overlap in that case since a whole cluster is read.
>
Yeah, i know your concern. For our codes design at this moment, it
still safe for those involved buffer areas to get overlaped, because
most of them are referring the data after overlaping. fill_dir_slot
function is the only corruption area i have found so far.
> I also agree with Stephen's recommendations.
>
> Best regards,
> Benoît