[U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer
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;
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; }
On 1/31/19 1:42 PM, tien.fong.chee@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; }
On Thu, 2019-01-31 at 15:23 +0100, Marek Vasut wrote:
On 1/31/19 1:42 PM, tien.fong.chee@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; }
On 1/31/19 1:42 PM, tien.fong.chee@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;
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);
On Thu, 2019-01-31 at 15:22 +0100, Marek Vasut wrote:
On 1/31/19 1:42 PM, tien.fong.chee@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;
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@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 ?
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@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.
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@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.
participants (3)
-
Chee, Tien Fong -
Marek Vasut -
tien.fong.chee＠intel.com