Patchwork [U-Boot,3/7] JFFS2: Calculate buf_len before we read data from flash

login
register
mail settings
Submitter Baidu Boy
Date April 24, 2011, 3:36 a.m.
Message ID <001e01cc0230$e0268b90$6401a8c0@LENOVOE5CA6843>
Download mbox | patch
Permalink /patch/92632/
State New
Delegated to: Detlev Zundel
Headers show

Comments

Baidu Boy - April 24, 2011, 3:36 a.m.
1/ We should calculate the buf_len before we call
 get_fl_mem().

Signed-off-by: Baidu Liu <liucai.lfn@gmail.com>
---
 fs/jffs2/jffs2_1pass.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
Detlev Zundel - April 29, 2011, 1:11 p.m.
Hi Baidu,

>  1/ We should calculate the buf_len before we call
>  get_fl_mem().

If I read your change correctly, then do you mean the following?

  When we know what we want to read, we can calculate buf_len to be the
  maximum size of the data to be read.  Without this, we usually read
  EMPTY_SCAN_SIZE which is too much.

  This is only an optimization change.

Maybe this text will help people understand better what the change tries
to do.

> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
> index 8eb77b1..be6ac78 100644
> --- a/fs/jffs2/jffs2_1pass.c
> +++ b/fs/jffs2/jffs2_1pass.c
> @@ -1643,6 +1643,8 @@ jffs2_1pass_build_lists(struct part_info * part)
>  			case JFFS2_NODETYPE_INODE:
>  				if (buf_ofs + buf_len < ofs + sizeof(struct
>  							jffs2_raw_inode)) {
> +					buf_len = min_t(uint32_t, buf_size, sector_ofs
> +						+ part->sector_size - ofs);		

I am somewhat uncomfortable that "buf_len" is used in the if condition
before it is recalculated inside.  Are you sure that this cannot lead to
problems, i.e. can buf_len become larger than it was when entering the
condition?

>  					get_fl_mem((u32)part->offset + ofs,
>  						   buf_len, buf);
>  					buf_ofs = ofs;
> @@ -1659,11 +1661,11 @@ jffs2_1pass_build_lists(struct part_info * part)
>  				}
>  				break;
>  			case JFFS2_NODETYPE_DIRENT:
> -				if (buf_ofs + buf_len < ofs + sizeof(struct
> -							jffs2_raw_dirent) +
> -							((struct
> -							 jffs2_raw_dirent *)
> -							node)->nsize) {
> +				if (buf_ofs + buf_len < ofs + 
> +					sizeof(struct jffs2_raw_dirent) + 
> +					((struct jffs2_raw_dirent *)node)->nsize) {
> +					buf_len = min_t(uint32_t, buf_size, sector_ofs
> +						+ part->sector_size - ofs);	

The same question for this case.

Cheers
  Detlev
Baidu Boy - April 29, 2011, 2:17 p.m.
Hi,Detlev

2011/4/29 Detlev Zundel <dzu@denx.de>:
> Hi Baidu,
>
>>  1/ We should calculate the buf_len before we call
>>  get_fl_mem().
>
> If I read your change correctly, then do you mean the following?
>
>  When we know what we want to read, we can calculate buf_len to be the
>  maximum size of the data to be read.  Without this, we usually read
>  EMPTY_SCAN_SIZE which is too much.
Yes,you are right.

>  This is only an optimization change.
>
> Maybe this text will help people understand better what the change tries
> to do.
The patch comments may be enough to explain what my changes is. We can see the
code:
buf_len = min_t() everywhere the get_fl_mem() called.

>> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
>> index 8eb77b1..be6ac78 100644
>> --- a/fs/jffs2/jffs2_1pass.c
>> +++ b/fs/jffs2/jffs2_1pass.c
>> @@ -1643,6 +1643,8 @@ jffs2_1pass_build_lists(struct part_info * part)
>>                       case JFFS2_NODETYPE_INODE:
>>                               if (buf_ofs + buf_len < ofs + sizeof(struct
>>                                                       jffs2_raw_inode)) {
>> +                                     buf_len = min_t(uint32_t, buf_size, sector_ofs
>> +                                             + part->sector_size - ofs);
>
> I am somewhat uncomfortable that "buf_len" is used in the if condition
> before it is recalculated inside.  Are you sure that this cannot lead to
> problems, i.e. can buf_len become larger than it was when entering the
> condition?

It works well in my boards.
Baidu Boy - April 29, 2011, 2:28 p.m.
Hi,Detlev

2011/4/29 Detlev Zundel <dzu@denx.de>:
> Hi Baidu,
>
>>  1/ We should calculate the buf_len before we call
>>  get_fl_mem().
>
> If I read your change correctly, then do you mean the following?
>
>  When we know what we want to read, we can calculate buf_len to be the
>  maximum size of the data to be read.  Without this, we usually read
>  EMPTY_SCAN_SIZE which is too much.
Yes,you are right.

>  This is only an optimization change.
>
> Maybe this text will help people understand better what the change tries
> to do.
The patch comments may be enough to explain what my changes is. We can see the
code:
buf_len = min_t() everywhere the get_fl_mem() called.

>> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
>> index 8eb77b1..be6ac78 100644
>> --- a/fs/jffs2/jffs2_1pass.c
>> +++ b/fs/jffs2/jffs2_1pass.c
>> @@ -1643,6 +1643,8 @@ jffs2_1pass_build_lists(struct part_info * part)
>>                       case JFFS2_NODETYPE_INODE:
>>                               if (buf_ofs + buf_len < ofs + sizeof(struct
>>                                                       jffs2_raw_inode)) {
>> +                                     buf_len = min_t(uint32_t, buf_size, sector_ofs
>> +                                             + part->sector_size - ofs);
>
> I am somewhat uncomfortable that "buf_len" is used in the if condition
> before it is recalculated inside.  Are you sure that this cannot lead to
> problems, i.e. can buf_len become larger than it was when entering the
> condition?

It works well in my boards.
Detlev Zundel - April 29, 2011, 5:22 p.m.
Hi Baidu,

[...]

>>> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
>>> index 8eb77b1..be6ac78 100644
>>> --- a/fs/jffs2/jffs2_1pass.c
>>> +++ b/fs/jffs2/jffs2_1pass.c
>>> @@ -1643,6 +1643,8 @@ jffs2_1pass_build_lists(struct part_info * part)
>>>                       case JFFS2_NODETYPE_INODE:
>>>                               if (buf_ofs + buf_len < ofs + sizeof(struct
>>>                                                       jffs2_raw_inode)) {
>>> +                                     buf_len = min_t(uint32_t, buf_size, sector_ofs
>>> +                                             + part->sector_size - ofs);
>>
>> I am somewhat uncomfortable that "buf_len" is used in the if condition
>> before it is recalculated inside.  Are you sure that this cannot lead to
>> problems, i.e. can buf_len become larger than it was when entering the
>> condition?
>
> It works well in my boards.

Aha.  I am all happy for you that this works, but we really need to be
as sure as we can be that the code does the right thing.  "It works for
me" is not enough.

Cheers
  Detlev
Baidu Boy - April 30, 2011, 12:48 a.m.
Hi, Detlev :

2011/4/30 Detlev Zundel <dzu@denx.de>:
> Hi Baidu,
>
> [...]
>
>>>> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
>>>> index 8eb77b1..be6ac78 100644
>>>> --- a/fs/jffs2/jffs2_1pass.c
>>>> +++ b/fs/jffs2/jffs2_1pass.c
>>>> @@ -1643,6 +1643,8 @@ jffs2_1pass_build_lists(struct part_info * part)
>>>>                       case JFFS2_NODETYPE_INODE:
>>>>                               if (buf_ofs + buf_len < ofs + sizeof(struct
>>>>                                                       jffs2_raw_inode)) {
>>>> +                                     buf_len = min_t(uint32_t, buf_size, sector_ofs
>>>> +                                             + part->sector_size - ofs);
>>>
>>> I am somewhat uncomfortable that "buf_len" is used in the if condition
>>> before it is recalculated inside.  Are you sure that this cannot lead to
>>> problems, i.e. can buf_len become larger than it was when entering the
>>> condition?
>>
>> It works well in my boards.
>
> Aha.  I am all happy for you that this works, but we really need to be
> as sure as we can be that the code does the right thing.  "It works for
> me" is not enough.

Please read every line code in the uboot jffs2 carefully. Know more about JFFS2.
Detlev Zundel - May 2, 2011, 12:25 p.m.
Hi Baidu,

> Hi, Detlev :
>
> 2011/4/30 Detlev Zundel <dzu@denx.de>:
>> Hi Baidu,
>>
>> [...]
>>
>>>>> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
>>>>> index 8eb77b1..be6ac78 100644
>>>>> --- a/fs/jffs2/jffs2_1pass.c
>>>>> +++ b/fs/jffs2/jffs2_1pass.c
>>>>> @@ -1643,6 +1643,8 @@ jffs2_1pass_build_lists(struct part_info * part)
>>>>>                       case JFFS2_NODETYPE_INODE:
>>>>>                               if (buf_ofs + buf_len < ofs + sizeof(struct
>>>>>                                                       jffs2_raw_inode)) {
>>>>> +                                     buf_len = min_t(uint32_t, buf_size, sector_ofs
>>>>> +                                             + part->sector_size - ofs);
>>>>
>>>> I am somewhat uncomfortable that "buf_len" is used in the if condition
>>>> before it is recalculated inside.  Are you sure that this cannot lead to
>>>> problems, i.e. can buf_len become larger than it was when entering the
>>>> condition?
>>>
>>> It works well in my boards.
>>
>> Aha.  I am all happy for you that this works, but we really need to be
>> as sure as we can be that the code does the right thing.  "It works for
>> me" is not enough.
>
> Please read every line code in the uboot jffs2 carefully. Know more
> about JFFS2.

I was asking you a question about a potential problem that I see with
your changes. Can you answer the question or not?  If you cannot answer
it, then you are changing things that you do not understand.  Personally
I don't trust bug-fixes that are not understood completely.

Cheers
  Detlev

Patch

diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c
index 8eb77b1..be6ac78 100644
--- a/fs/jffs2/jffs2_1pass.c
+++ b/fs/jffs2/jffs2_1pass.c
@@ -1643,6 +1643,8 @@  jffs2_1pass_build_lists(struct part_info * part)
 			case JFFS2_NODETYPE_INODE:
 				if (buf_ofs + buf_len < ofs + sizeof(struct
 							jffs2_raw_inode)) {
+					buf_len = min_t(uint32_t, buf_size, sector_ofs
+						+ part->sector_size - ofs);		
 					get_fl_mem((u32)part->offset + ofs,
 						   buf_len, buf);
 					buf_ofs = ofs;
@@ -1659,11 +1661,11 @@  jffs2_1pass_build_lists(struct part_info * part)
 				}
 				break;
 			case JFFS2_NODETYPE_DIRENT:
-				if (buf_ofs + buf_len < ofs + sizeof(struct
-							jffs2_raw_dirent) +
-							((struct
-							 jffs2_raw_dirent *)
-							node)->nsize) {
+				if (buf_ofs + buf_len < ofs + 
+					sizeof(struct jffs2_raw_dirent) + 
+					((struct jffs2_raw_dirent *)node)->nsize) {
+					buf_len = min_t(uint32_t, buf_size, sector_ofs
+						+ part->sector_size - ofs);	
 					get_fl_mem((u32)part->offset + ofs,
 						   buf_len, buf);
 					buf_ofs = ofs;