From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Fri, 1 Feb 2019 08:11:45 +0000 Subject: [U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer In-Reply-To: References: <1548938578-7414-1-git-send-email-tien.fong.chee@intel.com> Message-ID: <1549008704.9768.5.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > > > > 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 > > Signed-off-by: Tien Fong Chee > > > > --- > > > > 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. > > > > > + free(tmp_buffer); > >   *gotsize += actsize; > >   if (!filesize) > >   return 0; > > >