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

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

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@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...@toradex.com>
Signed-off-by: Tien Fong Chee <tien.fong.chee@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) {
 			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);
+		free(tmp_buffer);
 		*gotsize += actsize;
 		if (!filesize)
 			return 0;
-- 
2.2.0

[U-Boot] [PATCH v2 2/2] fs: fat: Reduce default max clustersize 64KiB from malloc pool

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Release cluster block immediately when no longer use would help to reduce
64KiB memory allocated to the memory pool.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

---

changes for v2
- Assigned NULL to itr after free.
- Added NULL checking to itr, avoid freeing twice.
---
 fs/fat/fat.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 347787e..fa846ff 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -1152,12 +1152,21 @@ int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
 		goto out_free_both;
 
 	debug("reading %s at pos %llu\n", filename, pos);
-	ret = get_contents(&fsdata, itr->dent, pos, buffer, maxsize, actread);
+
+	/* For saving default max clustersize memory allocated to malloc pool */
+	dir_entry *dentptr = itr->dent;
+
+	free(itr);
+
+	itr = NULL;
+
+	ret = get_contents(&fsdata, dentptr, pos, buffer, maxsize, actread);
 
 out_free_both:
 	free(fsdata.fatbuf);
 out_free_itr:
-	free(itr);
+	if (itr)
+		free(itr);
 	return ret;
 }
 
-- 
2.2.0

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

[Marek Vasut]

On 1/31/19 1:42 PM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@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...@toradex.com>
> Signed-off-by: Tien Fong Chee <tien.fong.chee@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 ?

>  			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?

> +		free(tmp_buffer);
>  		*gotsize += actsize;
>  		if (!filesize)
>  			return 0;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] fs: fat: Reduce default max clustersize 64KiB from malloc pool

[Marek Vasut]

On 1/31/19 1:42 PM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> Release cluster block immediately when no longer use would help to reduce
> 64KiB memory allocated to the memory pool.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> ---
> 
> changes for v2
> - Assigned NULL to itr after free.
> - Added NULL checking to itr, avoid freeing twice.
> ---
>  fs/fat/fat.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 347787e..fa846ff 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -1152,12 +1152,21 @@ int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
>  		goto out_free_both;
>  
>  	debug("reading %s at pos %llu\n", filename, pos);
> -	ret = get_contents(&fsdata, itr->dent, pos, buffer, maxsize, actread);
> +
> +	/* For saving default max clustersize memory allocated to malloc pool */
> +	dir_entry *dentptr = itr->dent;
> +
> +	free(itr);
> +
> +	itr = NULL;
> +
> +	ret = get_contents(&fsdata, dentptr, pos, buffer, maxsize, actread);
>  
>  out_free_both:
>  	free(fsdata.fatbuf);
>  out_free_itr:
> -	free(itr);
> +	if (itr)
> +		free(itr);

free(NULL) is valid, so you can drop the if conditional.

>  	return ret;
>  }
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] fs: fat: Reduce default max clustersize 64KiB from malloc pool

[Chee, Tien Fong]

On Thu, 2019-01-31 at 15:23 +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@intel.com>
> > 
> > Release cluster block immediately when no longer use would help to
> > reduce
> > 64KiB memory allocated to the memory pool.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > ---
> > 
> > changes for v2
> > - Assigned NULL to itr after free.
> > - Added NULL checking to itr, avoid freeing twice.
> > ---
> >  fs/fat/fat.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index 347787e..fa846ff 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -1152,12 +1152,21 @@ int file_fat_read_at(const char *filename,
> > loff_t pos, void *buffer,
> >  		goto out_free_both;
> >  
> >  	debug("reading %s at pos %llu\n", filename, pos);
> > -	ret = get_contents(&fsdata, itr->dent, pos, buffer,
> > maxsize, actread);
> > +
> > +	/* For saving default max clustersize memory allocated to
> > malloc pool */
> > +	dir_entry *dentptr = itr->dent;
> > +
> > +	free(itr);
> > +
> > +	itr = NULL;
> > +
> > +	ret = get_contents(&fsdata, dentptr, pos, buffer, maxsize,
> > actread);
> >  
> >  out_free_both:
> >  	free(fsdata.fatbuf);
> >  out_free_itr:
> > -	free(itr);
> > +	if (itr)
> > +		free(itr);
> free(NULL) is valid, so you can drop the if conditional.
Noted.
> 
> > 
> >  	return ret;
> >  }
> >  
> > 
> 

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

[Chee, Tien Fong]

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@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...@toradex.com>
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@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.
> 
> > 
> > +		free(tmp_buffer);
> >  		*gotsize += actsize;
> >  		if (!filesize)
> >  			return 0;
> > 
> 

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

[Marek Vasut]

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@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...@toradex.com>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@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 ?

-- 
Best regards,
Marek Vasut

[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@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...@toradex.com>
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@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.
> 

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

[Marek Vasut]

On 2/1/19 4:20 PM, Chee, Tien Fong wrote:
> 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@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...@toradex.com>
>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@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.

OK, reading the code again, after simplification we get:

tmp_buffer = malloc_cache_aligned(actsize);
...
actsize -= pos;
memcpy(buffer, tmp_buffer + pos, actsize);

so there shouldn't be any read past the end of buffer.

-- 
Best regards,
Marek Vasut