fs: fat: dynamically allocate memory for temporary buffer

PATCH New Stefan Agner 3 participants 6 messages Dec 6, 2018 VFAT/FAT/msdos filesystem

lore

Share & tools

lore

Stefan Agner

author

2018-12-06 13:58 UTC #0 lore raw

PATCH 1/1 fs: fat: dynamically allocate memory for temporary buffer

From: Stefan Agner <[email protected]>

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 <[email protected]>

---

 fs/fat/fat.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

fs/fat/fat.c +14 -6

index 4bc3030ab8..06bcd02835 100644
@@ -305,9 +305,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)
 {

@@ -318,7 +315,7 @@ static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos,
 	loff_t actsize;
 
 	*gotsize = 0;
-	debug("Filesize: %llu bytes\n", filesize);
+	debug("Filesize: %llu bytes, starting at pos %llu\n", filesize, pos);
 
 	if (pos >= filesize) {
 		debug("Read position past EOF: %llu\n", pos);

@@ -350,15 +347,26 @@ 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;
+
+		tmp_buffer = malloc_cache_aligned(MAX_CLUSTSIZE);
+		if (tmp_buffer == NULL) {
+			debug("Error: allocating buffer\n");
+			return -1;
+		}
+
 		actsize = min(filesize, (loff_t)bytesperclust);
-		if (get_cluster(mydata, curclust, get_contents_vfatname_block,
+		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(tmp_buffer, buffer + pos, actsize);
+		free(tmp_buffer);
+
 		*gotsize += actsize;
 		if (!filesize)
 			return 0;

Signature · 1 line

2.19.2

Lukasz Majewski

2018-12-06 14:42 UTC #1 lore raw

PATCH 1/1 fs: fat: dynamically allocate memory for temporary buffer

Quoted 65 lines · On Thu, 6 Dec 2018 14:58:52 +0100 Stefan Agner <[email protected]> wrote:

From: Stefan Agner <[email protected]>

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 <[email protected]>

---

 fs/fat/fat.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

fs/fat/fat.c +0 -0

index 4bc3030ab8..06bcd02835 100644
@@ -305,9 +305,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. */

fs/fat/fat.c +0 -3

-__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)
{

fs/fat/fat.c +14 -3

@@ -318,7 +315,7 @@ static int get_contents(fsdata *mydata, dir_entry
 *dentptr, loff_t pos, loff_t actsize;
 
 	*gotsize = 0;
-	debug("Filesize: %llu bytes\n", filesize);
+	debug("Filesize: %llu bytes, starting at pos %llu\n",
 filesize, pos); 
 	if (pos >= filesize) {
 		debug("Read position past EOF: %llu\n", pos);

@@ -350,15 +347,26 @@ 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;
+
+		tmp_buffer = malloc_cache_aligned(MAX_CLUSTSIZE);
+		if (tmp_buffer == NULL) {
+			debug("Error: allocating buffer\n");
+			return -1;
+		}
+
 		actsize = min(filesize, (loff_t)bytesperclust);
-		if (get_cluster(mydata, curclust,
 get_contents_vfatname_block,
+		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(tmp_buffer, buffer + pos, actsize);
+		free(tmp_buffer);
+
 		*gotsize += actsize;

fs/fat/fat.c static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, · line 372

Quoted 2 lines

fs/fat/fat.c +0 -0

 		if (!filesize)
 			return 0;

Reviewed-by Lukasz Majewski <[email protected]>

Best regards,

Lukasz Majewski

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de -------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181206/7c37379d/attachment.sig>

Tom Rini

2018-12-06 15:13 UTC #2 lore raw

PATCH 1/1 fs: fat: dynamically allocate memory for temporary buffer

In reply to Stefan Agner

Quoted 7 lines · Stefan Agner

From: Stefan Agner <[email protected]>
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 <[email protected]>

Is there a particular issue you're running into here, or just that what we're doing is just generally not a great idea? I ask since I need to poke git history to make sure that the thing at the back of my mind about us needing to do this for some annoying boot case is intentional. Thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181206/8781326a/attachment.sig>

Stefan Agner

author

2018-12-06 15:45 UTC #3 lore raw

PATCH 1/1 fs: fat: dynamically allocate memory for temporary buffer

Quoted 8 lines · Tom Rini

Quoted 6 lines · Stefan Agner
From: Stefan Agner <[email protected]>
Drop the statically allocated get_contents_vfatname_block and dynamically allocate a buffer only if required. This saves 64KiB of memory.

Quoted 6 lines

Quoted 1 line
Signed-off-by Stefan Agner <[email protected]>
Is there a particular issue you're running into here, or just that what we're doing is just generally not a great idea? I ask since I need to poke git history to make sure that the thing at the back of my mind about us needing to do this for some annoying boot case is intentional.

I was in the process checking large global variables since I had issues on a target with limited SRAM (used pre-relocation). I only after writing this patch I realized that the space for the .bss section is not required pre-relocation, so the patch did not help in my particular case.

That said, in git history I saw that get_contents_vfatname_block has been used more broadly in earlier FAT implementations. Now it is only used to handle a corner case: when loading from a file with an offset... I guess the .bss section is cleared, so we probably safe 64KiB of zeroing out... I feel it is still worth doing the change, but I don't have a really strong feeling.

--
Stefan

Tom Rini

2018-12-06 16:06 UTC #4 lore raw

PATCH 1/1 fs: fat: dynamically allocate memory for temporary buffer

Quoted 22 lines · Stefan Agner

Quoted 8 lines · Tom Rini
Quoted 6 lines · Stefan Agner
From: Stefan Agner <[email protected]>
Drop the statically allocated get_contents_vfatname_block and dynamically allocate a buffer only if required. This saves 64KiB of memory.
Quoted 6 lines
Quoted 1 line
Signed-off-by Stefan Agner <[email protected]>
Is there a particular issue you're running into here, or just that what we're doing is just generally not a great idea? I ask since I need to poke git history to make sure that the thing at the back of my mind about us needing to do this for some annoying boot case is intentional.
I was in the process checking large global variables since I had issues on a target with limited SRAM (used pre-relocation). I only after writing this patch I realized that the space for the .bss section is not required pre-relocation, so the patch did not help in my particular case.

Quoted 6 lines

That said, in git history I saw that get_contents_vfatname_block has been used more broadly in earlier FAT implementations. Now it is only used to handle a corner case: when loading from a file with an offset... I guess the .bss section is cleared, so we probably safe 64KiB of zeroing out... I feel it is still worth doing the change, but I don't have a really strong feeling.

Thanks for checking. Yes, this is probably a good thing to do overall and whatever historical reason we needed it like this isn't applicable now if we're doing offset loading only in this case which isn't an SPL thing anyhow.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181206/134eabdf/attachment.sig>

Stefan Agner

author

2018-12-06 15:46 UTC #5 lore raw

PATCH 1/1 fs: fat: dynamically allocate memory for temporary buffer

In reply to Stefan Agner

Quoted 52 lines · Stefan Agner

From: Stefan Agner <[email protected]>

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 <[email protected]>

---

 fs/fat/fat.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

fs/fat/fat.c +0 -3

index 4bc3030ab8..06bcd02835 100644
@@ -305,9 +305,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)

{

fs/fat/fat.c +1 -1

@@ -318,7 +315,7 @@ static int get_contents(fsdata *mydata, dir_entry
 *dentptr, loff_t pos,
 	loff_t actsize;
 
 	*gotsize = 0;
-	debug("Filesize: %llu bytes\n", filesize);
+	debug("Filesize: %llu bytes, starting at pos %llu\n", filesize, pos);
 
 	if (pos >= filesize) {

fs/fat/fat.c +8 -1

 		debug("Read position past EOF: %llu\n", pos);

@@ -350,15 +347,26 @@ 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;
+
+		tmp_buffer = malloc_cache_aligned(MAX_CLUSTSIZE);
+		if (tmp_buffer == NULL) {
+			debug("Error: allocating buffer\n");
+			return -1;
+		}
+
 		actsize = min(filesize, (loff_t)bytesperclust);
-		if (get_cluster(mydata, curclust, get_contents_vfatname_block,

fs/fat/fat.c static int get_contents(fsdata *mydata, · line 368

Quoted 10 lines

fs/fat/fat.c +3 -1

+		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(tmp_buffer, buffer + pos, actsize);

I somehow managed to mix up copy direction here. Will send a v2.

--
Stefan

Quoted 5 lines

fs/fat/fat.c +2 -0

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

return 0;