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

[U-Boot] [PATCH v3 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 v3
- Removed the cast on actsize
---
 fs/fat/fat.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index ecfa255..ea11250 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,24 @@ 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,
-				(int)actsize) != 0) {
+		tmp_buffer = malloc_cache_aligned(actsize);
+		if (!tmp_buffer) {
+			debug("Error: allocating buffer\n");
+			return -ENOMEM;
+		}
+
+		if (get_cluster(mydata, curclust, tmp_buffer, 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 v3 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 v3
- Dropped the if conditional because free(NULL) is valid.
---
 fs/fat/fat.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index ea11250..26ae101 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -1151,7 +1151,15 @@ 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);
-- 
2.2.0

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

[Tom Rini]

On Mon, Feb 11, 2019 at 02:56:20PM +0800, 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>

Applied to u-boot/master, thanks!

-- 
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/20190219/3418b56c/attachment.sig>

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

[Tom Rini]

On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>

Applied to u-boot/master, thanks!

-- 
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/20190219/6ab82a55/attachment.sig>

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

[Michal Simek]

Hi Tom,

On 20. 02. 19 2:58, Tom Rini wrote:
> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
> 
> Applied to u-boot/master, thanks!

please remove this patch (better both of them because they were in
series) because they are breaking at least ZynqMP SPL. It is also too
late in cycle to create random fix.

You can't simply move 64KB from code to malloc without reflecting this
by changing MALLOC space size.

Other boards with SPL fat could be also affected by this if they don't
allocate big malloc space.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

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

[Chee, Tien Fong]

On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
> Hi Tom,
> 
> On 20. 02. 19 2:58, Tom Rini wrote:
> > 
> > On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
> > Applied to u-boot/master, thanks!
> please remove this patch (better both of them because they were in
> series)
I think patch 2/2 should be safe, because no memory size is changed.
Basically, it just to release the allocated memory immediately when
it's not required, so other can re-use it.

> because they are breaking at least ZynqMP SPL. It is also too
> late in cycle to create random fix.
> 
> You can't simply move 64KB from code to malloc without reflecting
> this
> by changing MALLOC space size.
> 
> Other boards with SPL fat could be also affected by this if they
> don't
> allocate big malloc space.
So, any suggestion to get the patch 1/2 accepted? inform all board
maintainers to test it out?
> 
> Thanks,
> Michal
> 

Thanks,
Tien Fong.

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

[Alexander Graf]

On 21.02.19 09:23, Chee, Tien Fong wrote:
> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>> Hi Tom,
>>
>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>
>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>> Applied to u-boot/master, thanks!
>> please remove this patch (better both of them because they were in
>> series)
> I think patch 2/2 should be safe, because no memory size is changed.
> Basically, it just to release the allocated memory immediately when
> it's not required, so other can re-use it.
> 
>> because they are breaking at least ZynqMP SPL. It is also too
>> late in cycle to create random fix.
>>
>> You can't simply move 64KB from code to malloc without reflecting
>> this
>> by changing MALLOC space size.
>>
>> Other boards with SPL fat could be also affected by this if they
>> don't
>> allocate big malloc space.
> So, any suggestion to get the patch 1/2 accepted? inform all board
> maintainers to test it out?

You already received feedback that it does break ZynqMP, so the current
approach won't work.

How about you create a new kconfig option that allows you to say whether
you want to use malloc or .bss for temporary data in the FAT driver. You
can then have an _SPL_ version of that kconfig and check for it with
IS_ENABLED() which should automatically tell you the right answer
depending on whether you're in an SPL build or not.

Then you can set the SPL version to default malloc and the non-SPL
version to default .bss.

That should give you the fix you want, without the problems it
introduces for SPL (where malloc space is really constrained, and
discouraged to use because you can't check whether it fits at compile time).


Alex

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

[Chee, Tien Fong]

On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
> 
> On 21.02.19 09:23, Chee, Tien Fong wrote:
> > 
> > On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
> > > 
> > > Hi Tom,
> > > 
> > > On 20. 02. 19 2:58, Tom Rini wrote:
> > > > 
> > > > 
> > > > On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
> > > > Applied to u-boot/master, thanks!
> > > please remove this patch (better both of them because they were
> > > in
> > > series)
> > I think patch 2/2 should be safe, because no memory size is
> > changed.
> > Basically, it just to release the allocated memory immediately when
> > it's not required, so other can re-use it.
> > 
> > > 
> > > because they are breaking at least ZynqMP SPL. It is also too
> > > late in cycle to create random fix.
> > > 
> > > You can't simply move 64KB from code to malloc without reflecting
> > > this
> > > by changing MALLOC space size.
> > > 
> > > Other boards with SPL fat could be also affected by this if they
> > > don't
> > > allocate big malloc space.
> > So, any suggestion to get the patch 1/2 accepted? inform all board
> > maintainers to test it out?
> You already received feedback that it does break ZynqMP, so the
> current
> approach won't work.
> 
> How about you create a new kconfig option that allows you to say
> whether
> you want to use malloc or .bss for temporary data in the FAT driver.
> You
> can then have an _SPL_ version of that kconfig and check for it with
> IS_ENABLED() which should automatically tell you the right answer
> depending on whether you're in an SPL build or not.
> 
> Then you can set the SPL version to default malloc and the non-SPL
> version to default .bss.
Marek and Tom rini,

Are you guys okay with Alex's suggestion?
> 
> That should give you the fix you want, without the problems it
> introduces for SPL (where malloc space is really constrained, and
> discouraged to use because you can't check whether it fits at compile
> time).
> 
> 
> Alex

Thanks.
TF.

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

[Marek Vasut]

On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>
>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>
>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>
>>>> Hi Tom,
>>>>
>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>
>>>>>
>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>>>> Applied to u-boot/master, thanks!
>>>> please remove this patch (better both of them because they were
>>>> in
>>>> series)
>>> I think patch 2/2 should be safe, because no memory size is
>>> changed.
>>> Basically, it just to release the allocated memory immediately when
>>> it's not required, so other can re-use it.
>>>
>>>>
>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>> late in cycle to create random fix.
>>>>
>>>> You can't simply move 64KB from code to malloc without reflecting
>>>> this
>>>> by changing MALLOC space size.
>>>>
>>>> Other boards with SPL fat could be also affected by this if they
>>>> don't
>>>> allocate big malloc space.
>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>> maintainers to test it out?
>> You already received feedback that it does break ZynqMP, so the
>> current
>> approach won't work.
>>
>> How about you create a new kconfig option that allows you to say
>> whether
>> you want to use malloc or .bss for temporary data in the FAT driver.
>> You
>> can then have an _SPL_ version of that kconfig and check for it with
>> IS_ENABLED() which should automatically tell you the right answer
>> depending on whether you're in an SPL build or not.
>>
>> Then you can set the SPL version to default malloc and the non-SPL
>> version to default .bss.
> Marek and Tom rini,
> 
> Are you guys okay with Alex's suggestion?

I'm not a big fan of adding more and more ifdeffery.
Is there some other option ?

-- 
Best regards,
Marek Vasut

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

[Alexander Graf]

On 21.02.19 09:41, Marek Vasut wrote:
> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>>
>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>>
>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>>
>>>>> Hi Tom,
>>>>>
>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>>
>>>>>>
>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>>>>> Applied to u-boot/master, thanks!
>>>>> please remove this patch (better both of them because they were
>>>>> in
>>>>> series)
>>>> I think patch 2/2 should be safe, because no memory size is
>>>> changed.
>>>> Basically, it just to release the allocated memory immediately when
>>>> it's not required, so other can re-use it.
>>>>
>>>>>
>>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>>> late in cycle to create random fix.
>>>>>
>>>>> You can't simply move 64KB from code to malloc without reflecting
>>>>> this
>>>>> by changing MALLOC space size.
>>>>>
>>>>> Other boards with SPL fat could be also affected by this if they
>>>>> don't
>>>>> allocate big malloc space.
>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>>> maintainers to test it out?
>>> You already received feedback that it does break ZynqMP, so the
>>> current
>>> approach won't work.
>>>
>>> How about you create a new kconfig option that allows you to say
>>> whether
>>> you want to use malloc or .bss for temporary data in the FAT driver.
>>> You
>>> can then have an _SPL_ version of that kconfig and check for it with
>>> IS_ENABLED() which should automatically tell you the right answer
>>> depending on whether you're in an SPL build or not.
>>>
>>> Then you can set the SPL version to default malloc and the non-SPL
>>> version to default .bss.
>> Marek and Tom rini,
>>
>> Are you guys okay with Alex's suggestion?
> 
> I'm not a big fan of adding more and more ifdeffery.
> Is there some other option ?

Is RAM up already at this point? Maybe we could improve the SPL malloc
mechanism to move allocations into DRAM once it's available.


Alex

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

[Marek Vasut]

On 2/21/19 9:44 AM, Alexander Graf wrote:
> 
> 
> On 21.02.19 09:41, Marek Vasut wrote:
>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>>>
>>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>>>
>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>>>
>>>>>> Hi Tom,
>>>>>>
>>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>>>>>> Applied to u-boot/master, thanks!
>>>>>> please remove this patch (better both of them because they were
>>>>>> in
>>>>>> series)
>>>>> I think patch 2/2 should be safe, because no memory size is
>>>>> changed.
>>>>> Basically, it just to release the allocated memory immediately when
>>>>> it's not required, so other can re-use it.
>>>>>
>>>>>>
>>>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>>>> late in cycle to create random fix.
>>>>>>
>>>>>> You can't simply move 64KB from code to malloc without reflecting
>>>>>> this
>>>>>> by changing MALLOC space size.
>>>>>>
>>>>>> Other boards with SPL fat could be also affected by this if they
>>>>>> don't
>>>>>> allocate big malloc space.
>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>>>> maintainers to test it out?
>>>> You already received feedback that it does break ZynqMP, so the
>>>> current
>>>> approach won't work.
>>>>
>>>> How about you create a new kconfig option that allows you to say
>>>> whether
>>>> you want to use malloc or .bss for temporary data in the FAT driver.
>>>> You
>>>> can then have an _SPL_ version of that kconfig and check for it with
>>>> IS_ENABLED() which should automatically tell you the right answer
>>>> depending on whether you're in an SPL build or not.
>>>>
>>>> Then you can set the SPL version to default malloc and the non-SPL
>>>> version to default .bss.
>>> Marek and Tom rini,
>>>
>>> Are you guys okay with Alex's suggestion?
>>
>> I'm not a big fan of adding more and more ifdeffery.
>> Is there some other option ?
> 
> Is RAM up already at this point? Maybe we could improve the SPL malloc
> mechanism to move allocations into DRAM once it's available.

Well, the FAT buffers waste some 64kiB of bss, so we can use that area
for malloc instead, no ?

-- 
Best regards,
Marek Vasut

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

[Alexander Graf]

On 21.02.19 09:49, Marek Vasut wrote:
> On 2/21/19 9:44 AM, Alexander Graf wrote:
>>
>>
>> On 21.02.19 09:41, Marek Vasut wrote:
>>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
>>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>>>>
>>>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>>>>
>>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>>>>
>>>>>>> Hi Tom,
>>>>>>>
>>>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>> please remove this patch (better both of them because they were
>>>>>>> in
>>>>>>> series)
>>>>>> I think patch 2/2 should be safe, because no memory size is
>>>>>> changed.
>>>>>> Basically, it just to release the allocated memory immediately when
>>>>>> it's not required, so other can re-use it.
>>>>>>
>>>>>>>
>>>>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>>>>> late in cycle to create random fix.
>>>>>>>
>>>>>>> You can't simply move 64KB from code to malloc without reflecting
>>>>>>> this
>>>>>>> by changing MALLOC space size.
>>>>>>>
>>>>>>> Other boards with SPL fat could be also affected by this if they
>>>>>>> don't
>>>>>>> allocate big malloc space.
>>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>>>>> maintainers to test it out?
>>>>> You already received feedback that it does break ZynqMP, so the
>>>>> current
>>>>> approach won't work.
>>>>>
>>>>> How about you create a new kconfig option that allows you to say
>>>>> whether
>>>>> you want to use malloc or .bss for temporary data in the FAT driver.
>>>>> You
>>>>> can then have an _SPL_ version of that kconfig and check for it with
>>>>> IS_ENABLED() which should automatically tell you the right answer
>>>>> depending on whether you're in an SPL build or not.
>>>>>
>>>>> Then you can set the SPL version to default malloc and the non-SPL
>>>>> version to default .bss.
>>>> Marek and Tom rini,
>>>>
>>>> Are you guys okay with Alex's suggestion?
>>>
>>> I'm not a big fan of adding more and more ifdeffery.
>>> Is there some other option ?
>>
>> Is RAM up already at this point? Maybe we could improve the SPL malloc
>> mechanism to move allocations into DRAM once it's available.
> 
> Well, the FAT buffers waste some 64kiB of bss, so we can use that area
> for malloc instead, no ?

Yes, but that means you need to review every single board that uses FAT
in SPL today and adjust its malloc region size.

Alex

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

[Marek Vasut]

On 2/21/19 9:55 AM, Alexander Graf wrote:
> 
> 
> On 21.02.19 09:49, Marek Vasut wrote:
>> On 2/21/19 9:44 AM, Alexander Graf wrote:
>>>
>>>
>>> On 21.02.19 09:41, Marek Vasut wrote:
>>>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
>>>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>>>>>
>>>>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>>>>>
>>>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>>>>>
>>>>>>>> Hi Tom,
>>>>>>>>
>>>>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>> please remove this patch (better both of them because they were
>>>>>>>> in
>>>>>>>> series)
>>>>>>> I think patch 2/2 should be safe, because no memory size is
>>>>>>> changed.
>>>>>>> Basically, it just to release the allocated memory immediately when
>>>>>>> it's not required, so other can re-use it.
>>>>>>>
>>>>>>>>
>>>>>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>>>>>> late in cycle to create random fix.
>>>>>>>>
>>>>>>>> You can't simply move 64KB from code to malloc without reflecting
>>>>>>>> this
>>>>>>>> by changing MALLOC space size.
>>>>>>>>
>>>>>>>> Other boards with SPL fat could be also affected by this if they
>>>>>>>> don't
>>>>>>>> allocate big malloc space.
>>>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>>>>>> maintainers to test it out?
>>>>>> You already received feedback that it does break ZynqMP, so the
>>>>>> current
>>>>>> approach won't work.
>>>>>>
>>>>>> How about you create a new kconfig option that allows you to say
>>>>>> whether
>>>>>> you want to use malloc or .bss for temporary data in the FAT driver.
>>>>>> You
>>>>>> can then have an _SPL_ version of that kconfig and check for it with
>>>>>> IS_ENABLED() which should automatically tell you the right answer
>>>>>> depending on whether you're in an SPL build or not.
>>>>>>
>>>>>> Then you can set the SPL version to default malloc and the non-SPL
>>>>>> version to default .bss.
>>>>> Marek and Tom rini,
>>>>>
>>>>> Are you guys okay with Alex's suggestion?
>>>>
>>>> I'm not a big fan of adding more and more ifdeffery.
>>>> Is there some other option ?
>>>
>>> Is RAM up already at this point? Maybe we could improve the SPL malloc
>>> mechanism to move allocations into DRAM once it's available.
>>
>> Well, the FAT buffers waste some 64kiB of bss, so we can use that area
>> for malloc instead, no ?
> 
> Yes, but that means you need to review every single board that uses FAT
> in SPL today and adjust its malloc region size.

That's quite likely ... I still think this patch is beneficial, it's
much better to dynamically allocate the cluster size than have this
64kiB chunk of BSS carved out.

-- 
Best regards,
Marek Vasut

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

[Michal Simek]

On 21. 02. 19 10:04, Marek Vasut wrote:
> On 2/21/19 9:55 AM, Alexander Graf wrote:
>>
>>
>> On 21.02.19 09:49, Marek Vasut wrote:
>>> On 2/21/19 9:44 AM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 21.02.19 09:41, Marek Vasut wrote:
>>>>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
>>>>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>>>>>>
>>>>>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>>>>>>
>>>>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>>>>>>
>>>>>>>>> Hi Tom,
>>>>>>>>>
>>>>>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>> please remove this patch (better both of them because they were
>>>>>>>>> in
>>>>>>>>> series)
>>>>>>>> I think patch 2/2 should be safe, because no memory size is
>>>>>>>> changed.
>>>>>>>> Basically, it just to release the allocated memory immediately when
>>>>>>>> it's not required, so other can re-use it.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>>>>>>> late in cycle to create random fix.
>>>>>>>>>
>>>>>>>>> You can't simply move 64KB from code to malloc without reflecting
>>>>>>>>> this
>>>>>>>>> by changing MALLOC space size.
>>>>>>>>>
>>>>>>>>> Other boards with SPL fat could be also affected by this if they
>>>>>>>>> don't
>>>>>>>>> allocate big malloc space.
>>>>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>>>>>>> maintainers to test it out?
>>>>>>> You already received feedback that it does break ZynqMP, so the
>>>>>>> current
>>>>>>> approach won't work.
>>>>>>>
>>>>>>> How about you create a new kconfig option that allows you to say
>>>>>>> whether
>>>>>>> you want to use malloc or .bss for temporary data in the FAT driver.
>>>>>>> You
>>>>>>> can then have an _SPL_ version of that kconfig and check for it with
>>>>>>> IS_ENABLED() which should automatically tell you the right answer
>>>>>>> depending on whether you're in an SPL build or not.
>>>>>>>
>>>>>>> Then you can set the SPL version to default malloc and the non-SPL
>>>>>>> version to default .bss.
>>>>>> Marek and Tom rini,
>>>>>>
>>>>>> Are you guys okay with Alex's suggestion?
>>>>>
>>>>> I'm not a big fan of adding more and more ifdeffery.
>>>>> Is there some other option ?
>>>>
>>>> Is RAM up already at this point? Maybe we could improve the SPL malloc
>>>> mechanism to move allocations into DRAM once it's available.
>>>
>>> Well, the FAT buffers waste some 64kiB of bss, so we can use that area
>>> for malloc instead, no ?
>>
>> Yes, but that means you need to review every single board that uses FAT
>> in SPL today and adjust its malloc region size.
> 
> That's quite likely ... I still think this patch is beneficial, it's
> much better to dynamically allocate the cluster size than have this
> 64kiB chunk of BSS carved out.
> 

ok. I have played with it a little bit and the patch exposed different
problem with one of my out of tree patch I am working on.

Anyway that's being said I still think that patches like this shouldn't
come to the tree at this stage because it requires checking on other
boards. IIRC similar patch was around in past and there was also any
issue with it.

Tom: up2you if you want to keep it in the tree or not.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

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

[Marek Vasut]

On 2/21/19 1:13 PM, Michal Simek wrote:
> On 21. 02. 19 10:04, Marek Vasut wrote:
>> On 2/21/19 9:55 AM, Alexander Graf wrote:
>>>
>>>
>>> On 21.02.19 09:49, Marek Vasut wrote:
>>>> On 2/21/19 9:44 AM, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> On 21.02.19 09:41, Marek Vasut wrote:
>>>>>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
>>>>>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>>>>>>>
>>>>>>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>>>>>>>
>>>>>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Tom,
>>>>>>>>>>
>>>>>>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>> please remove this patch (better both of them because they were
>>>>>>>>>> in
>>>>>>>>>> series)
>>>>>>>>> I think patch 2/2 should be safe, because no memory size is
>>>>>>>>> changed.
>>>>>>>>> Basically, it just to release the allocated memory immediately when
>>>>>>>>> it's not required, so other can re-use it.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>>>>>>>> late in cycle to create random fix.
>>>>>>>>>>
>>>>>>>>>> You can't simply move 64KB from code to malloc without reflecting
>>>>>>>>>> this
>>>>>>>>>> by changing MALLOC space size.
>>>>>>>>>>
>>>>>>>>>> Other boards with SPL fat could be also affected by this if they
>>>>>>>>>> don't
>>>>>>>>>> allocate big malloc space.
>>>>>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>>>>>>>> maintainers to test it out?
>>>>>>>> You already received feedback that it does break ZynqMP, so the
>>>>>>>> current
>>>>>>>> approach won't work.
>>>>>>>>
>>>>>>>> How about you create a new kconfig option that allows you to say
>>>>>>>> whether
>>>>>>>> you want to use malloc or .bss for temporary data in the FAT driver.
>>>>>>>> You
>>>>>>>> can then have an _SPL_ version of that kconfig and check for it with
>>>>>>>> IS_ENABLED() which should automatically tell you the right answer
>>>>>>>> depending on whether you're in an SPL build or not.
>>>>>>>>
>>>>>>>> Then you can set the SPL version to default malloc and the non-SPL
>>>>>>>> version to default .bss.
>>>>>>> Marek and Tom rini,
>>>>>>>
>>>>>>> Are you guys okay with Alex's suggestion?
>>>>>>
>>>>>> I'm not a big fan of adding more and more ifdeffery.
>>>>>> Is there some other option ?
>>>>>
>>>>> Is RAM up already at this point? Maybe we could improve the SPL malloc
>>>>> mechanism to move allocations into DRAM once it's available.
>>>>
>>>> Well, the FAT buffers waste some 64kiB of bss, so we can use that area
>>>> for malloc instead, no ?
>>>
>>> Yes, but that means you need to review every single board that uses FAT
>>> in SPL today and adjust its malloc region size.
>>
>> That's quite likely ... I still think this patch is beneficial, it's
>> much better to dynamically allocate the cluster size than have this
>> 64kiB chunk of BSS carved out.
>>
> 
> ok. I have played with it a little bit and the patch exposed different
> problem with one of my out of tree patch I am working on.
> 
> Anyway that's being said I still think that patches like this shouldn't
> come to the tree at this stage because it requires checking on other
> boards. IIRC similar patch was around in past and there was also any
> issue with it.
> 
> Tom: up2you if you want to keep it in the tree or not.

This shouldn't have come in after RC2, so revert and let's fix it for
next release.

-- 
Best regards,
Marek Vasut

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

[Adam Ford]

On Thu, Feb 21, 2019 at 6:51 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/21/19 1:13 PM, Michal Simek wrote:
> > On 21. 02. 19 10:04, Marek Vasut wrote:
> >> On 2/21/19 9:55 AM, Alexander Graf wrote:
> >>>
> >>>
> >>> On 21.02.19 09:49, Marek Vasut wrote:
> >>>> On 2/21/19 9:44 AM, Alexander Graf wrote:
> >>>>>
> >>>>>
> >>>>> On 21.02.19 09:41, Marek Vasut wrote:
> >>>>>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
> >>>>>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
> >>>>>>>>
> >>>>>>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
> >>>>>>>>>
> >>>>>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi Tom,
> >>>>>>>>>>
> >>>>>>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
> >>>>>>>>>>> Applied to u-boot/master, thanks!
> >>>>>>>>>> please remove this patch (better both of them because they were
> >>>>>>>>>> in
> >>>>>>>>>> series)
> >>>>>>>>> I think patch 2/2 should be safe, because no memory size is
> >>>>>>>>> changed.
> >>>>>>>>> Basically, it just to release the allocated memory immediately when
> >>>>>>>>> it's not required, so other can re-use it.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> because they are breaking at least ZynqMP SPL. It is also too
> >>>>>>>>>> late in cycle to create random fix.
> >>>>>>>>>>
> >>>>>>>>>> You can't simply move 64KB from code to malloc without reflecting
> >>>>>>>>>> this
> >>>>>>>>>> by changing MALLOC space size.
> >>>>>>>>>>
> >>>>>>>>>> Other boards with SPL fat could be also affected by this if they
> >>>>>>>>>> don't
> >>>>>>>>>> allocate big malloc space.
> >>>>>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
> >>>>>>>>> maintainers to test it out?
> >>>>>>>> You already received feedback that it does break ZynqMP, so the
> >>>>>>>> current
> >>>>>>>> approach won't work.
> >>>>>>>>
> >>>>>>>> How about you create a new kconfig option that allows you to say
> >>>>>>>> whether
> >>>>>>>> you want to use malloc or .bss for temporary data in the FAT driver.
> >>>>>>>> You
> >>>>>>>> can then have an _SPL_ version of that kconfig and check for it with
> >>>>>>>> IS_ENABLED() which should automatically tell you the right answer
> >>>>>>>> depending on whether you're in an SPL build or not.
> >>>>>>>>
> >>>>>>>> Then you can set the SPL version to default malloc and the non-SPL
> >>>>>>>> version to default .bss.
> >>>>>>> Marek and Tom rini,
> >>>>>>>
> >>>>>>> Are you guys okay with Alex's suggestion?
> >>>>>>
> >>>>>> I'm not a big fan of adding more and more ifdeffery.
> >>>>>> Is there some other option ?
> >>>>>
> >>>>> Is RAM up already at this point? Maybe we could improve the SPL malloc
> >>>>> mechanism to move allocations into DRAM once it's available.
> >>>>
> >>>> Well, the FAT buffers waste some 64kiB of bss, so we can use that area
> >>>> for malloc instead, no ?
> >>>
> >>> Yes, but that means you need to review every single board that uses FAT
> >>> in SPL today and adjust its malloc region size.

This happened to me for a different board where the SPI-NOR flash
driver was updated, but it required me to increase my malloc size.  I
don't think it would have been appropriate for me to ask the
maintainer to reject a patch when where was a simple solution, and the
author helped me identify how to make the updated infrastructure work.
   I think the benefits outweigh the cons if (and only if) the
solution is only to increase the malloc size.  Many of us have very
restricted SPL space.

> >>
> >> That's quite likely ... I still think this patch is beneficial, it's
> >> much better to dynamically allocate the cluster size than have this
> >> 64kiB chunk of BSS carved out.
> >>
> >
> > ok. I have played with it a little bit and the patch exposed different
> > problem with one of my out of tree patch I am working on.
> >
> > Anyway that's being said I still think that patches like this shouldn't
> > come to the tree at this stage because it requires checking on other
> > boards. IIRC similar patch was around in past and there was also any
> > issue with it.
> >
> > Tom: up2you if you want to keep it in the tree or not.
>
> This shouldn't have come in after RC2, so revert and let's fix it for
> next release.
>
> --
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

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

[Tom Rini]

On Thu, Feb 21, 2019 at 08:45:37AM +0100, Michal Simek wrote:
> Hi Tom,
> 
> On 20. 02. 19 2:58, Tom Rini wrote:
> > On Mon, Feb 11, 2019 at 02:56:19PM +0800, 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>
> > 
> > Applied to u-boot/master, thanks!
> 
> please remove this patch (better both of them because they were in
> series) because they are breaking at least ZynqMP SPL. It is also too
> late in cycle to create random fix.
> 
> You can't simply move 64KB from code to malloc without reflecting this
> by changing MALLOC space size.
> 
> Other boards with SPL fat could be also affected by this if they don't
> allocate big malloc space.

I see from later in on the thread your specific problem is elsewhere.
But to address the root question, we have fairly large malloc
requirements in SPL when FAT is involved as there's a lot of other
mallocs going on there.  It's indeed not impossible there's a board that
was on-edge of it's 1MB pool, and now goes over, but that seems
unlikely.  Thanks all!

-- 
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/20190221/4a914ae2/attachment.sig>

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

[Chee, Tien Fong]

On Thu, 2019-02-21 at 22:22 -0500, Tom Rini wrote:
> On Thu, Feb 21, 2019 at 08:45:37AM +0100, Michal Simek wrote:
> > 
> > Hi Tom,
> > 
> > On 20. 02. 19 2:58, Tom Rini wrote:
> > > 
> > > On Mon, Feb 11, 2019 at 02:56:19PM +0800, tien.fong.chee at intel.co
> > > m 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>
> > > Applied to u-boot/master, thanks!
> > please remove this patch (better both of them because they were in
> > series) because they are breaking at least ZynqMP SPL. It is also
> > too
> > late in cycle to create random fix.
> > 
> > You can't simply move 64KB from code to malloc without reflecting
> > this
> > by changing MALLOC space size.
> > 
> > Other boards with SPL fat could be also affected by this if they
> > don't
> > allocate big malloc space.
> I see from later in on the thread your specific problem is elsewhere.
> But to address the root question, we have fairly large malloc
> requirements in SPL when FAT is involved as there's a lot of other
> mallocs going on there.  It's indeed not impossible there's a board
> that
> was on-edge of it's 1MB pool, and now goes over, but that seems
> unlikely.  Thanks all!
I'm curious what's the actual problems you found? running out of
memory, no?

Increasing Malloc space size is definitely required for The patch[1/2],
but patch[2/2] would maximize the re-usable of memory, so in other
words, no change on Malloc space size required. Both patches would
share the same memory.

You might need to take a look these patches, they are part of vfat
optimization.
https://patchwork.ozlabs.org/patch/1029679/
https://patchwork.ozlabs.org/patch/1029681/
https://patchwork.ozlabs.org/patch/1029682/
https://patchwork.ozlabs.org/patch/1029683/
> 

Thanks,
TF

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

[Michal Simek]

On 22. 02. 19 4:49, Chee, Tien Fong wrote:
> On Thu, 2019-02-21 at 22:22 -0500, Tom Rini wrote:
>> On Thu, Feb 21, 2019 at 08:45:37AM +0100, Michal Simek wrote:
>>>
>>> Hi Tom,
>>>
>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>
>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, tien.fong.chee at intel.co
>>>> m 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>
>>>> Applied to u-boot/master, thanks!
>>> please remove this patch (better both of them because they were in
>>> series) because they are breaking at least ZynqMP SPL. It is also
>>> too
>>> late in cycle to create random fix.
>>>
>>> You can't simply move 64KB from code to malloc without reflecting
>>> this
>>> by changing MALLOC space size.
>>>
>>> Other boards with SPL fat could be also affected by this if they
>>> don't
>>> allocate big malloc space.
>> I see from later in on the thread your specific problem is elsewhere.
>> But to address the root question, we have fairly large malloc
>> requirements in SPL when FAT is involved as there's a lot of other
>> mallocs going on there.  It's indeed not impossible there's a board
>> that
>> was on-edge of it's 1MB pool, and now goes over, but that seems
>> unlikely.  Thanks all!
> I'm curious what's the actual problems you found? running out of
> memory, no?

TBH I don't know now. I tried to debug that. I want to support option to
use DT from fixed location to support qemu DT generation which we have
enabled for next generation. And I use 0x1000 location which is in
conflict with bss section. That's why I have moved BSS section out of
this space to make sure that I am not breaking dtb out there.
And it is working quite well but when this patch is applied DTB is being
corrupted by SPL(don't know which code) and on reboot dtb magic is
correct but content is broken.
I don't know the first address and I need to find some time to run this
on Qemu and see activities in that area to find out which code is
breaking this.
Anyway I don't have a time for debugging this more now but bisect
pointed to this patch but as I said it just exposed likely problem
somewhere else.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

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

[Chee, Tien Fong]

On Fri, 2019-02-22 at 10:16 +0100, Michal Simek wrote:
> On 22. 02. 19 4:49, Chee, Tien Fong wrote:
> > 
> > On Thu, 2019-02-21 at 22:22 -0500, Tom Rini wrote:
> > > 
> > > On Thu, Feb 21, 2019 at 08:45:37AM +0100, Michal Simek wrote:
> > > > 
> > > > 
> > > > Hi Tom,
> > > > 
> > > > On 20. 02. 19 2:58, Tom Rini wrote:
> > > > > 
> > > > > 
> > > > > On Mon, Feb 11, 2019 at 02:56:19PM +0800, tien.fong.chee at inte
> > > > > l.co
> > > > > m 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>
> > > > > Applied to u-boot/master, thanks!
> > > > please remove this patch (better both of them because they were
> > > > in
> > > > series) because they are breaking at least ZynqMP SPL. It is
> > > > also
> > > > too
> > > > late in cycle to create random fix.
> > > > 
> > > > You can't simply move 64KB from code to malloc without
> > > > reflecting
> > > > this
> > > > by changing MALLOC space size.
> > > > 
> > > > Other boards with SPL fat could be also affected by this if
> > > > they
> > > > don't
> > > > allocate big malloc space.
> > > I see from later in on the thread your specific problem is
> > > elsewhere.
> > > But to address the root question, we have fairly large malloc
> > > requirements in SPL when FAT is involved as there's a lot of
> > > other
> > > mallocs going on there.  It's indeed not impossible there's a
> > > board
> > > that
> > > was on-edge of it's 1MB pool, and now goes over, but that seems
> > > unlikely.  Thanks all!
> > I'm curious what's the actual problems you found? running out of
> > memory, no?
> TBH I don't know now. I tried to debug that. I want to support option
> to
> use DT from fixed location to support qemu DT generation which we
> have
> enabled for next generation. And I use 0x1000 location which is in
> conflict with bss section. That's why I have moved BSS section out of
> this space to make sure that I am not breaking dtb out there.
> And it is working quite well but when this patch is applied DTB is
> being
> corrupted by SPL(don't know which code) and on reboot dtb magic is
> correct but content is broken.
> I don't know the first address and I need to find some time to run
> this
> on Qemu and see activities in that area to find out which code is
> breaking this.
> Anyway I don't have a time for debugging this more now but bisect
> pointed to this patch but as I said it just exposed likely problem
> somewhere else.
> 
Okay. You might need to consider Tom's suggestion, 1MB is too large for
SPL malloc pool, some other places might overflow and corruption.
> 
>