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.c...@intel.com wrote:
>>>>>
>>>>>
>>>>> From: Tien Fong Chee <tien.fong.c...@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.c...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
