[U-Boot] [PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to store dir_slot entries

[U-Boot] [PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to store dir_slot entries

[Tien Fong Chee]

fill_dir_slot use get_contents_vfatname_block as a temporary buffer for
constructing a list of dir_slot entries. To save the memory and providing
correct type of memory for above usage, a local buffer with accurate size
declaration is introduced.

The local array size 640 is used because for long file name entry,
each entry use 32 bytes, one entry can store up to 13 characters.
The maximum number of entry possible is 20. So, total size is
32*20=640bytes.

Signed-off-by: Genevieve Chan <ccheauya@altera.com>
Signed-off-by: Tien Fong Chee <tfchee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: ChinLiang <clsee@altera.com>
Cc: Vagrant Cascadian <vagrant@debian.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Beno?t Th?baudeau <benoit@wsystem.com>
---
 fs/fat/fat_write.c |    3 ++-
 include/fat.h      |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index eb3a916..c1d48c5 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -323,7 +323,8 @@ static void flush_dir_table(fsdata *mydata, dir_entry **dentptr);
 static void
 fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
 {
-	dir_slot *slotptr = (dir_slot *)get_contents_vfatname_block;
+	__u8 temp_dir_slot_buffer[MAX_LFN_SLOT * sizeof(dir_slot)];
+	dir_slot *slotptr = (dir_slot *)temp_dir_slot_buffer;
 	__u8 counter = 0, checksum;
 	int idx = 0, ret;
 	char s_name[16];
diff --git a/include/fat.h b/include/fat.h
index 9d053e6..90fdeba 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -33,6 +33,9 @@
 #define FAT16BUFSIZE	(FATBUFSIZE/2)
 #define FAT32BUFSIZE	(FATBUFSIZE/4)
 
+/* Maximum number of entry for long file name according to spec */
+#define MAX_LFN_SLOT	20
+
 
 /* Filesystem identifiers */
 #define FAT12_SIGN	"FAT12   "
-- 
1.7.7.4

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

[Tien Fong Chee]

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@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: ChinLiang <clsee@altera.com>
Cc: Vagrant Cascadian <vagrant@debian.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Beno?t Th?baudeau <benoit@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)
-- 
1.7.7.4

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

[Benoît Thébaudeau]

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@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: ChinLiang <clsee@altera.com>
> Cc: Vagrant Cascadian <vagrant@debian.org>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Beno?t Th?baudeau <benoit@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,

Best regards,
Beno?t

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

[Tien Fong Chee]

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@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: ChinLiang <clsee@altera.com>
> > Cc: Vagrant Cascadian <vagrant@debian.org>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > Cc: Beno?t Th?baudeau <benoit@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,
>
> Best regards,
> Beno?t

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.

Best regards,
Tien Fong

[U-Boot] [PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to store dir_slot entries

[Stephen Warren]

On 07/13/2016 03:01 AM, Tien Fong Chee wrote:
> fill_dir_slot use get_contents_vfatname_block as a temporary buffer for
> constructing a list of dir_slot entries. To save the memory and providing
> correct type of memory for above usage, a local buffer with accurate size
> declaration is introduced.
>
> The local array size 640 is used because for long file name entry,
> each entry use 32 bytes, one entry can store up to 13 characters.
> The maximum number of entry possible is 20. So, total size is
> 32*20=640bytes.

I'm fairly sure this series is correct[1], although the FAT write code 
is a little hard to follow.

[1] except in the face of disk read errors or corrupted data while 
parsing long filename entries, but the code already looks broken in the 
case of disk errors, and the corrupted data case probably isn't much 
worse now.

> diff --git a/include/fat.h b/include/fat.h

> +/* Maximum number of entry for long file name according to spec */
> +#define MAX_LFN_SLOT	20
> +
>
>   /* Filesystem identifiers */
>   #define FAT12_SIGN	"FAT12   "

Why 2 blank lines there?

Also, I'd suggest simply deleting get_contents_vfatname_block and 
renaming all references to it to use get_dentfromdir_block instead. That 
way, anyone reading the code in the future won't assume the two "block" 
variables refer to different memory, when in fact they share storage. 
Related, there's little point keeping the now-redundant memcpy() call at 
the end of get_long_file_name():

> 		memcpy(get_dentfromdir_block, get_contents_vfatname_block,
> 			mydata->clust_size * mydata->sect_size);

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

[Benoît Thébaudeau]

Dear Tien Fong,

On Thu, Jul 14, 2016 at 12:48 PM, Tien Fong Chee <tfchee@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@altera.com>
>> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
>> > Cc: ChinLiang <clsee@altera.com>
>> > Cc: Vagrant Cascadian <vagrant@debian.org>
>> > Cc: Simon Glass <sjg@chromium.org>
>> > Cc: Stephen Warren <swarren@nvidia.com>
>> > Cc: Beno?t Th?baudeau <benoit@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.
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

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.

I also agree with Stephen's recommendations.

Best regards,
Beno?t

[U-Boot] [PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to store dir_slot entries

[Tien Fong Chee]

On Thu, 2016-07-14 at 15:00 -0600, Stephen Warren wrote:
> On 07/13/2016 03:01 AM, Tien Fong Chee wrote:
> > fill_dir_slot use get_contents_vfatname_block as a temporary buffer
> > for
> > constructing a list of dir_slot entries. To save the memory and
> > providing
> > correct type of memory for above usage, a local buffer with
> > accurate size
> > declaration is introduced.
> >
> > The local array size 640 is used because for long file name entry,
> > each entry use 32 bytes, one entry can store up to 13 characters.
> > The maximum number of entry possible is 20. So, total size is
> > 32*20=640bytes.
>
> I'm fairly sure this series is correct[1], although the FAT write
> code
> is a little hard to follow.
>
> [1] except in the face of disk read errors or corrupted data while
> parsing long filename entries, but the code already looks broken in
> the
> case of disk errors, and the corrupted data case probably isn't much
> worse now.
>
> > diff --git a/include/fat.h b/include/fat.h
>
> > +/* Maximum number of entry for long file name according to spec */
> > +#define MAX_LFN_SLOT       20
> > +
> >
> >   /* Filesystem identifiers */
> >   #define FAT12_SIGN        "FAT12   "
>
> Why 2 blank lines there?
Good catch!!
>
> Also, I'd suggest simply deleting get_contents_vfatname_block and
> renaming all references to it to use get_dentfromdir_block instead.
> That
> way, anyone reading the code in the future won't assume the two
> "block"
> variables refer to different memory, when in fact they share storage.
> Related, there's little point keeping the now-redundant memcpy() call
> at
> the end of get_long_file_name():
>
> >             memcpy(get_dentfromdir_block,
> > get_contents_vfatname_block,
> >                     mydata->clust_size * mydata->sect_size);
>
Yeah, i agree with you that we should renaming all to single name to
avoid any confusion. I would remove those redundant codes.

Thanks.

[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@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@altera.com>
> > > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > > > Cc: ChinLiang <clsee@altera.com>
> > > > Cc: Vagrant Cascadian <vagrant@debian.org>
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Cc: Stephen Warren <swarren@nvidia.com>
> > > > Cc: Beno?t Th?baudeau <benoit@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

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

[Stephen Warren]

On 07/18/2016 09:53 PM, Tien Fong Chee wrote:
> 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@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@altera.com>
>>>>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>> Cc: Dinh Nguyen <dinh.linux@gmail.com>
>>>>> Cc: ChinLiang <clsee@altera.com>
>>>>> Cc: Vagrant Cascadian <vagrant@debian.org>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Stephen Warren <swarren@nvidia.com>
>>>>> Cc: Beno?t Th?baudeau <benoit@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 think switching to the FF library is a non-starter. It's 
excruciatingly slow since it always reads even contiguous files one 
cluster at a time. I did propose a change to the library, but the 
maintainer didn't seem interested in fixing the problem. If we were to 
switch, the Tianocore implementation might be worth looking at, now 
they've fixed the license of the FAT code to remove the requirement that 
it only be used in EFI implementations. I haven't looked the code to 
know whether it'd be possible/good to switch though.

[U-Boot] [PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to store dir_slot entries

[Tien Fong Chee]

fill_dir_slot use get_contents_vfatname_block as a temporary buffer for
constructing a list of dir_slot entries. To save the memory and providing
correct type of memory for above usage, a local buffer with accurate size
declaration is introduced.

The local array size 640 is used because for long file name entry,
each entry use 32 bytes, one entry can store up to 13 characters.
The maximum number of entry possible is 20. So, total size is
32*20=640bytes.

Signed-off-by: Genevieve Chan <ccheauya@altera.com>
Signed-off-by: Tien Fong Chee <tfchee@altera.com>
---
Changes for V2
- Removed extra space
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: ChinLiang <clsee@altera.com>
Cc: Vagrant Cascadian <vagrant@debian.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Beno?t Th?baudeau <benoit@wsystem.com>
---
 fs/fat/fat_write.c | 3 ++-
 include/fat.h      | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index eb3a916..c1d48c5 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -323,7 +323,8 @@ static void flush_dir_table(fsdata *mydata, dir_entry **dentptr);
 static void
 fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
 {
-	dir_slot *slotptr = (dir_slot *)get_contents_vfatname_block;
+	__u8 temp_dir_slot_buffer[MAX_LFN_SLOT * sizeof(dir_slot)];
+	dir_slot *slotptr = (dir_slot *)temp_dir_slot_buffer;
 	__u8 counter = 0, checksum;
 	int idx = 0, ret;
 	char s_name[16];
diff --git a/include/fat.h b/include/fat.h
index 9d053e6..483ff7c 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -33,6 +33,8 @@
 #define FAT16BUFSIZE	(FATBUFSIZE/2)
 #define FAT32BUFSIZE	(FATBUFSIZE/4)
 
+/* Maximum number of entry for long file name according to spec */
+#define MAX_LFN_SLOT	20
 
 /* Filesystem identifiers */
 #define FAT12_SIGN	"FAT12   "
-- 
1.8.2.1

[U-Boot] [PATCH 1/2] fs/fat/fatwrite: Local variable as buffer to store dir_slot entries

[Tien Fong Chee]

fill_dir_slot use get_contents_vfatname_block as a temporary buffer for
constructing a list of dir_slot entries. To save the memory and providing
correct type of memory for above usage, a local buffer with accurate size
declaration is introduced.

The local array size 640 is used because for long file name entry,
each entry use 32 bytes, one entry can store up to 13 characters.
The maximum number of entry possible is 20. So, total size is
32*20=640bytes.

Signed-off-by: Genevieve Chan <ccheauya@altera.com>
Signed-off-by: Tien Fong Chee <tfchee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: ChinLiang <clsee@altera.com>
Cc: Vagrant Cascadian <vagrant@debian.org>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Beno?t Th?baudeau <benoit@wsystem.com>
---
 fs/fat/fat_write.c |    3 ++-
 include/fat.h      |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index eb3a916..c1d48c5 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -323,7 +323,8 @@ static void flush_dir_table(fsdata *mydata, dir_entry **dentptr);
 static void
 fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
 {
-	dir_slot *slotptr = (dir_slot *)get_contents_vfatname_block;
+	__u8 temp_dir_slot_buffer[MAX_LFN_SLOT * sizeof(dir_slot)];
+	dir_slot *slotptr = (dir_slot *)temp_dir_slot_buffer;
 	__u8 counter = 0, checksum;
 	int idx = 0, ret;
 	char s_name[16];
diff --git a/include/fat.h b/include/fat.h
index 9d053e6..90fdeba 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -33,6 +33,9 @@
 #define FAT16BUFSIZE	(FATBUFSIZE/2)
 #define FAT32BUFSIZE	(FATBUFSIZE/4)
 
+/* Maximum number of entry for long file name according to spec */
+#define MAX_LFN_SLOT	20
+
 
 /* Filesystem identifiers */
 #define FAT12_SIGN	"FAT12   "
-- 
1.7.7.4