Re: [U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer
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
>
> 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
> Signed-off-by: Tien Fong Chee
>
> ---
>
> 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
Re: [U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer
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
> > > >
> > > > 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
> > > > Signed-off-by: Tien Fong Chee
> > > >
> > > > ---
> > > >
> > > > 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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer
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
>>>
>>> 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
>>> Signed-off-by: Tien Fong Chee
>>>
>>> ---
>>>
>>> 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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer
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
> >
> > 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
> > Signed-off-by: Tien Fong Chee
> >
> > ---
> >
> > 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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer
On 1/31/19 1:42 PM, tien.fong.c...@intel.com wrote:
> From: Tien Fong Chee
>
> 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
> Signed-off-by: Tien Fong Chee
>
> ---
>
> 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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
