[U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer

[Chee, Tien Fong]

On Fri, 2019-02-01 at 09:19 +0100, Marek Vasut wrote:
> On 2/1/19 9:11 AM, Chee, Tien Fong wrote:
> > 
> > On Thu, 2019-01-31 at 15:22 +0100, Marek Vasut wrote:
> > > 
> > > On 1/31/19 1:42 PM, tien.fong.chee at intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee at intel.com>
> > > > 
> > > > Drop the statically allocated get_contents_vfatname_block and
> > > > dynamically allocate a buffer only if required. This saves
> > > > 64KiB of memory.
> > > > 
> > > > Signed-off-by: Stefan Agner <stefan.ag... at toradex.com>
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
> > > > 
> > > > ---
> > > > 
> > > > changes for v2
> > > > - Removed the change for debug message.
> > > > - Set allocation based on actual required size instead of
> > > > default
> > > > max
> > > >   cluster size
> > > > ---
> > > >  fs/fat/fat.c | 17 ++++++++++++-----
> > > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > > > index ecfa255..347787e 100644
> > > > --- a/fs/fat/fat.c
> > > > +++ b/fs/fat/fat.c
> > > > @@ -306,9 +306,6 @@ get_cluster(fsdata *mydata, __u32 clustnum,
> > > > __u8 *buffer, unsigned long size)
> > > >   * into 'buffer'.
> > > >   * Update the number of bytes read in *gotsize or return -1 on
> > > > fatal errors.
> > > >   */
> > > > -__u8 get_contents_vfatname_block[MAX_CLUSTSIZE]
> > > > -	__aligned(ARCH_DMA_MINALIGN);
> > > > -
> > > >  static int get_contents(fsdata *mydata, dir_entry *dentptr,
> > > > loff_t
> > > > pos,
> > > >  			__u8 *buffer, loff_t maxsize, loff_t
> > > > *gotsize)
> > > >  {
> > > > @@ -351,15 +348,25 @@ static int get_contents(fsdata *mydata,
> > > > dir_entry *dentptr, loff_t pos,
> > > >  
> > > >  	/* align to beginning of next cluster if any */
> > > >  	if (pos) {
> > > > +		__u8 *tmp_buffer;
> > > > +
> > > >  		actsize = min(filesize,
> > > > (loff_t)bytesperclust);
> > > > -		if (get_cluster(mydata, curclust,
> > > > get_contents_vfatname_block,
> > > > +		tmp_buffer = malloc_cache_aligned(actsize);
> > > > +		if (!tmp_buffer) {
> > > > +			debug("Error: allocating buffer\n");
> > > > +			return -ENOMEM;
> > > > +		}
> > > > +
> > > > +		if (get_cluster(mydata, curclust, tmp_buffer,
> > > >  				(int)actsize) != 0) {
> > > Is the cast of actsize needed ?
> > Okay, i would remove it.
> > > 
> > > 
> > > > 
> > > > 
> > > >  			printf("Error reading cluster\n");
> > > > +			free(tmp_buffer);
> > > >  			return -1;
> > > >  		}
> > > >  		filesize -= actsize;
> > > >  		actsize -= pos;
> > > > -		memcpy(buffer, get_contents_vfatname_block +
> > > > pos,
> > > > actsize);
> > > > +		memcpy(buffer, tmp_buffer + pos, actsize);
> > > Hmmm, tmp_buffer is actsize big , but you're memcpy-ing actsize
> > > here,
> > > so
> > > this would mean you memcpy data past the end of tmp_buffer if pos
> > > >
> > > 0, no?
> > This wouldn't happen because the pos and actsize are reset based on
> > beginning of current cluster instead of beginning of a file. So,
> > the
> > memcpy would start at pos based on beginning of current cluster
> > until
> > the end of current cluster, that means the size it copies is still
> > within a cluster size.
> So pos is always 0 ?
Nop, reset here means the pos offset would be adjusted accordingly when
the base change from beginning of the file to beginning of the cluster
where the pos resides in.

You know the driver getting the content based on cluster by cluster.
>