* [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
@ 2014-02-28 6:37 shuox.liu
2014-03-02 9:03 ` Zhang, Yanmin
2014-03-03 19:45 ` Kees Cook
0 siblings, 2 replies; 10+ messages in thread
From: shuox.liu @ 2014-02-28 6:37 UTC (permalink / raw)
To: linux-kernel; +Cc: tony.luck, keescook, ccross, anton, yanmin_zhang
From: Liu ShuoX <shuox.liu@intel.com>
ftrace_read_cnt need to be reset in open to support mutli times
getting the records.
Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
---
fs/pstore/ram.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index fa8cef2..a5d0cab 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
cxt->dump_read_cnt = 0;
cxt->console_read_cnt = 0;
+ cxt->ftrace_read_cnt = 0;
return 0;
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-02-28 6:37 [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open shuox.liu
@ 2014-03-02 9:03 ` Zhang, Yanmin
2014-03-03 19:45 ` Kees Cook
1 sibling, 0 replies; 10+ messages in thread
From: Zhang, Yanmin @ 2014-03-02 9:03 UTC (permalink / raw)
To: shuox.liu, linux-kernel, akpm; +Cc: tony.luck, keescook, ccross, anton
On 2014/2/28 14:37, shuox.liu@intel.com wrote:
> From: Liu ShuoX <shuox.liu@intel.com>
>
> ftrace_read_cnt need to be reset in open to support mutli times
> getting the records.
Andrew,
Would you like to merge it to your testing tree?
pstore is a very important feature for debugging hard issues on
Android mobiles.
Yanmin
>
> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
> ---
> fs/pstore/ram.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index fa8cef2..a5d0cab 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>
> cxt->dump_read_cnt = 0;
> cxt->console_read_cnt = 0;
> + cxt->ftrace_read_cnt = 0;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-02-28 6:37 [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open shuox.liu
2014-03-02 9:03 ` Zhang, Yanmin
@ 2014-03-03 19:45 ` Kees Cook
2014-03-04 1:40 ` Liu ShuoX
1 sibling, 1 reply; 10+ messages in thread
From: Kees Cook @ 2014-03-03 19:45 UTC (permalink / raw)
To: shuox.liu; +Cc: LKML, Tony Luck, Colin Cross, Anton Vorontsov, yanmin_zhang
On Thu, Feb 27, 2014 at 10:37 PM, <shuox.liu@intel.com> wrote:
> From: Liu ShuoX <shuox.liu@intel.com>
>
> ftrace_read_cnt need to be reset in open to support mutli times
> getting the records.
>
> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
> ---
> fs/pstore/ram.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index fa8cef2..a5d0cab 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>
> cxt->dump_read_cnt = 0;
> cxt->console_read_cnt = 0;
> + cxt->ftrace_read_cnt = 0;
> return 0;
> }
I think we need a separate function for "clear" for the
ramoops_context struct. IIUC, we're missing a similar initialization
in ramoops_probe, which lacks both console_read_cnt=0 and
ftrace_read_cnt=0. Then both places could call this?
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-03-03 19:45 ` Kees Cook
@ 2014-03-04 1:40 ` Liu ShuoX
2014-03-04 19:11 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Liu ShuoX @ 2014-03-04 1:40 UTC (permalink / raw)
To: Kees Cook; +Cc: LKML, Tony Luck, Colin Cross, Anton Vorontsov, yanmin_zhang
On Mon 3.Mar'14 at 11:45:59 -0800, Kees Cook wrote:
>On Thu, Feb 27, 2014 at 10:37 PM, <shuox.liu@intel.com> wrote:
>> From: Liu ShuoX <shuox.liu@intel.com>
>>
>> ftrace_read_cnt need to be reset in open to support mutli times
>> getting the records.
>>
>> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
>> ---
>> fs/pstore/ram.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index fa8cef2..a5d0cab 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>>
>> cxt->dump_read_cnt = 0;
>> cxt->console_read_cnt = 0;
>> + cxt->ftrace_read_cnt = 0;
>> return 0;
>> }
>
>I think we need a separate function for "clear" for the
>ramoops_context struct. IIUC, we're missing a similar initialization
>in ramoops_probe, which lacks both console_read_cnt=0 and
>ftrace_read_cnt=0. Then both places could call this?
Hi Kees,
Currently, we have only one static ramoops_context named oops_cxt.
*_read_cnt should be initialized to 0 as default. Need we still add
such function for 'clear'?
>
>-Kees
>
>--
>Kees Cook
>Chrome OS Security
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-03-04 1:40 ` Liu ShuoX
@ 2014-03-04 19:11 ` Kees Cook
2014-03-07 2:58 ` Liu ShuoX
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2014-03-04 19:11 UTC (permalink / raw)
To: Liu ShuoX; +Cc: LKML, Tony Luck, Colin Cross, Anton Vorontsov, yanmin_zhang
On Mon, Mar 3, 2014 at 5:40 PM, Liu ShuoX <shuox.liu@intel.com> wrote:
> On Mon 3.Mar'14 at 11:45:59 -0800, Kees Cook wrote:
>>
>> On Thu, Feb 27, 2014 at 10:37 PM, <shuox.liu@intel.com> wrote:
>>>
>>> From: Liu ShuoX <shuox.liu@intel.com>
>>>
>>> ftrace_read_cnt need to be reset in open to support mutli times
>>> getting the records.
>>>
>>> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
>>> ---
>>> fs/pstore/ram.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>>> index fa8cef2..a5d0cab 100644
>>> --- a/fs/pstore/ram.c
>>> +++ b/fs/pstore/ram.c
>>> @@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info
>>> *psi)
>>>
>>> cxt->dump_read_cnt = 0;
>>> cxt->console_read_cnt = 0;
>>> + cxt->ftrace_read_cnt = 0;
>>> return 0;
>>> }
>>
>>
>> I think we need a separate function for "clear" for the
>> ramoops_context struct. IIUC, we're missing a similar initialization
>> in ramoops_probe, which lacks both console_read_cnt=0 and
>> ftrace_read_cnt=0. Then both places could call this?
>
> Hi Kees,
> Currently, we have only one static ramoops_context named oops_cxt.
> *_read_cnt should be initialized to 0 as default. Need we still add
> such function for 'clear'?
We have the pstore-global "oops_cxt" context. It is "initialized" only
once in ramoops_probe (and seems to needlessly set dump_read_cnt to
0). Otherwise, the context is initialized via ramoops_register_dummy
from module parameters (and initialized to zero with kzalloc).
So, I think my initial comment about "clear" is probably not right,
but that ramoops_pstore_open should be doing that (i.e. your original
patch is close). However, I think I'd like to see the needless zeroing
in ramoops_probe removed, and the "variables that need clearing on
open" documented in comments in "struct ramoops_context". That should
make this code more clear to read next time. :)
Thanks!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-03-04 19:11 ` Kees Cook
@ 2014-03-07 2:58 ` Liu ShuoX
2014-03-07 5:43 ` Kees Cook
2014-03-07 21:25 ` Andrew Morton
0 siblings, 2 replies; 10+ messages in thread
From: Liu ShuoX @ 2014-03-07 2:58 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Tony Luck, Colin Cross, Anton Vorontsov, yanmin_zhang, akpm
On Tue 4.Mar'14 at 11:11:11 -0800, Kees Cook wrote:
>On Mon, Mar 3, 2014 at 5:40 PM, Liu ShuoX <shuox.liu@intel.com> wrote:
>> On Mon 3.Mar'14 at 11:45:59 -0800, Kees Cook wrote:
>>>
>>> On Thu, Feb 27, 2014 at 10:37 PM, <shuox.liu@intel.com> wrote:
>>>>
>>>> From: Liu ShuoX <shuox.liu@intel.com>
>>>>
>>>> ftrace_read_cnt need to be reset in open to support mutli times
>>>> getting the records.
>>>>
>>>> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
>>>> ---
>>>> fs/pstore/ram.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>>>> index fa8cef2..a5d0cab 100644
>>>> --- a/fs/pstore/ram.c
>>>> +++ b/fs/pstore/ram.c
>>>> @@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info
>>>> *psi)
>>>>
>>>> cxt->dump_read_cnt = 0;
>>>> cxt->console_read_cnt = 0;
>>>> + cxt->ftrace_read_cnt = 0;
>>>> return 0;
>>>> }
>>>
>>>
>>> I think we need a separate function for "clear" for the
>>> ramoops_context struct. IIUC, we're missing a similar initialization
>>> in ramoops_probe, which lacks both console_read_cnt=0 and
>>> ftrace_read_cnt=0. Then both places could call this?
>>
>> Hi Kees,
>> Currently, we have only one static ramoops_context named oops_cxt.
>> *_read_cnt should be initialized to 0 as default. Need we still add
>> such function for 'clear'?
>
>We have the pstore-global "oops_cxt" context. It is "initialized" only
>once in ramoops_probe (and seems to needlessly set dump_read_cnt to
>0). Otherwise, the context is initialized via ramoops_register_dummy
>from module parameters (and initialized to zero with kzalloc).
>
>So, I think my initial comment about "clear" is probably not right,
>but that ramoops_pstore_open should be doing that (i.e. your original
>patch is close). However, I think I'd like to see the needless zeroing
>in ramoops_probe removed, and the "variables that need clearing on
>open" documented in comments in "struct ramoops_context". That should
>make this code more clear to read next time. :)
Sorry for delay reply. How about below patch?
From: Liu ShuoX <shuox.liu@intel.com>
ftrace_read_cnt need to be reset in open to support mutli times
getting the records.
Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
---
fs/pstore/ram.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index fa8cef2..9fe5b13 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -86,6 +86,7 @@ struct ramoops_context {
struct persistent_ram_ecc_info ecc_info;
unsigned int max_dump_cnt;
unsigned int dump_write_cnt;
+ /* _read_cnt need clear on ramoops_pstore_open */
unsigned int dump_read_cnt;
unsigned int console_read_cnt;
unsigned int ftrace_read_cnt;
@@ -101,6 +102,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
cxt->dump_read_cnt = 0;
cxt->console_read_cnt = 0;
+ cxt->ftrace_read_cnt = 0;
return 0;
}
@@ -428,7 +430,6 @@ static int ramoops_probe(struct platform_device *pdev)
if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
- cxt->dump_read_cnt = 0;
cxt->size = pdata->mem_size;
cxt->phys_addr = pdata->mem_address;
cxt->record_size = pdata->record_size;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-03-07 2:58 ` Liu ShuoX
@ 2014-03-07 5:43 ` Kees Cook
2014-03-07 21:25 ` Andrew Morton
1 sibling, 0 replies; 10+ messages in thread
From: Kees Cook @ 2014-03-07 5:43 UTC (permalink / raw)
To: Liu ShuoX
Cc: LKML, Tony Luck, Colin Cross, Anton Vorontsov, yanmin_zhang,
Andrew Morton
On Thu, Mar 6, 2014 at 6:58 PM, Liu ShuoX <shuox.liu@intel.com> wrote:
> On Tue 4.Mar'14 at 11:11:11 -0800, Kees Cook wrote:
>>
>> On Mon, Mar 3, 2014 at 5:40 PM, Liu ShuoX <shuox.liu@intel.com> wrote:
>>>
>>> On Mon 3.Mar'14 at 11:45:59 -0800, Kees Cook wrote:
>>>>
>>>>
>>>> On Thu, Feb 27, 2014 at 10:37 PM, <shuox.liu@intel.com> wrote:
>>>>>
>>>>>
>>>>> From: Liu ShuoX <shuox.liu@intel.com>
>>>>>
>>>>> ftrace_read_cnt need to be reset in open to support mutli times
>>>>> getting the records.
>>>>>
>>>>> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
>>>>> ---
>>>>> fs/pstore/ram.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>>>>> index fa8cef2..a5d0cab 100644
>>>>> --- a/fs/pstore/ram.c
>>>>> +++ b/fs/pstore/ram.c
>>>>> @@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info
>>>>> *psi)
>>>>>
>>>>> cxt->dump_read_cnt = 0;
>>>>> cxt->console_read_cnt = 0;
>>>>> + cxt->ftrace_read_cnt = 0;
>>>>> return 0;
>>>>> }
>>>>
>>>>
>>>>
>>>> I think we need a separate function for "clear" for the
>>>> ramoops_context struct. IIUC, we're missing a similar initialization
>>>> in ramoops_probe, which lacks both console_read_cnt=0 and
>>>> ftrace_read_cnt=0. Then both places could call this?
>>>
>>>
>>> Hi Kees,
>>> Currently, we have only one static ramoops_context named oops_cxt.
>>> *_read_cnt should be initialized to 0 as default. Need we still add
>>> such function for 'clear'?
>>
>>
>> We have the pstore-global "oops_cxt" context. It is "initialized" only
>> once in ramoops_probe (and seems to needlessly set dump_read_cnt to
>> 0). Otherwise, the context is initialized via ramoops_register_dummy
>> from module parameters (and initialized to zero with kzalloc).
>>
>> So, I think my initial comment about "clear" is probably not right,
>> but that ramoops_pstore_open should be doing that (i.e. your original
>> patch is close). However, I think I'd like to see the needless zeroing
>> in ramoops_probe removed, and the "variables that need clearing on
>> open" documented in comments in "struct ramoops_context". That should
>> make this code more clear to read next time. :)
>
> Sorry for delay reply. How about below patch?
>
>
> From: Liu ShuoX <shuox.liu@intel.com>
>
> ftrace_read_cnt need to be reset in open to support mutli times
> getting the records.
>
> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
> ---
> fs/pstore/ram.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index fa8cef2..9fe5b13 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -86,6 +86,7 @@ struct ramoops_context {
> struct persistent_ram_ecc_info ecc_info;
> unsigned int max_dump_cnt;
> unsigned int dump_write_cnt;
> + /* _read_cnt need clear on ramoops_pstore_open */
> unsigned int dump_read_cnt;
> unsigned int console_read_cnt;
> unsigned int ftrace_read_cnt;
> @@ -101,6 +102,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>
> cxt->dump_read_cnt = 0;
> cxt->console_read_cnt = 0;
> + cxt->ftrace_read_cnt = 0;
> return 0;
> }
> @@ -428,7 +430,6 @@ static int ramoops_probe(struct platform_device *pdev)
> if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
> pdata->ftrace_size =
> rounddown_pow_of_two(pdata->ftrace_size);
> - cxt->dump_read_cnt = 0;
> cxt->size = pdata->mem_size;
> cxt->phys_addr = pdata->mem_address;
> cxt->record_size = pdata->record_size;
> --
> 1.8.3.2
>
Thanks! That looks right to me.
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-03-07 2:58 ` Liu ShuoX
2014-03-07 5:43 ` Kees Cook
@ 2014-03-07 21:25 ` Andrew Morton
2014-03-08 0:23 ` Kees Cook
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2014-03-07 21:25 UTC (permalink / raw)
To: Liu ShuoX
Cc: Kees Cook, LKML, Tony Luck, Colin Cross, Anton Vorontsov, yanmin_zhang
On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX <shuox.liu@intel.com> wrote:
>
> ftrace_read_cnt need to be reset in open to support mutli times
> getting the records.
>
> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
> ---
> fs/pstore/ram.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index fa8cef2..9fe5b13 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -86,6 +86,7 @@ struct ramoops_context {
> struct persistent_ram_ecc_info ecc_info;
> unsigned int max_dump_cnt;
> unsigned int dump_write_cnt;
> + /* _read_cnt need clear on ramoops_pstore_open */
> unsigned int dump_read_cnt;
> unsigned int console_read_cnt;
> unsigned int ftrace_read_cnt;
> @@ -101,6 +102,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>
> cxt->dump_read_cnt = 0;
> cxt->console_read_cnt = 0;
> + cxt->ftrace_read_cnt = 0;
> return 0;
> }
>
> @@ -428,7 +430,6 @@ static int ramoops_probe(struct platform_device *pdev)
> if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
> pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
>
> - cxt->dump_read_cnt = 0;
> cxt->size = pdata->mem_size;
> cxt->phys_addr = pdata->mem_address;
> cxt->record_size = pdata->record_size;
The dump_read_cnt changes appear to be unrelated to the actual bugfix?
If so, please send this along as a separate patch. With a full
changelog - this one doesn't explain the dump_read_cnt changes at all.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open
2014-03-07 21:25 ` Andrew Morton
@ 2014-03-08 0:23 ` Kees Cook
2014-03-08 4:09 ` [PATCH] pstore: clarify clearing of _read_cnt in ramoops_context Liu Shuo
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2014-03-08 0:23 UTC (permalink / raw)
To: Andrew Morton
Cc: Liu ShuoX, LKML, Tony Luck, Colin Cross, Anton Vorontsov, yanmin_zhang
On Fri, Mar 7, 2014 at 1:25 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX <shuox.liu@intel.com> wrote:
>
>>
>> ftrace_read_cnt need to be reset in open to support mutli times
>> getting the records.
>>
>> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
>> ---
>> fs/pstore/ram.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index fa8cef2..9fe5b13 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -86,6 +86,7 @@ struct ramoops_context {
>> struct persistent_ram_ecc_info ecc_info;
>> unsigned int max_dump_cnt;
>> unsigned int dump_write_cnt;
>> + /* _read_cnt need clear on ramoops_pstore_open */
>> unsigned int dump_read_cnt;
>> unsigned int console_read_cnt;
>> unsigned int ftrace_read_cnt;
>> @@ -101,6 +102,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>>
>> cxt->dump_read_cnt = 0;
>> cxt->console_read_cnt = 0;
>> + cxt->ftrace_read_cnt = 0;
>> return 0;
>> }
>>
>> @@ -428,7 +430,6 @@ static int ramoops_probe(struct platform_device *pdev)
>> if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
>> pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
>>
>> - cxt->dump_read_cnt = 0;
>> cxt->size = pdata->mem_size;
>> cxt->phys_addr = pdata->mem_address;
>> cxt->record_size = pdata->record_size;
>
> The dump_read_cnt changes appear to be unrelated to the actual bugfix?
I recommended this change since it is being cleared during open, and
this zeroing was confusing.
> If so, please send this along as a separate patch. With a full
> changelog - this one doesn't explain the dump_read_cnt changes at all.
Liu, I would recommend a changelog along the lines of:
Clarify which variables need to be cleared during pstore_open. Added
missed ftrace_read_cnt clearing and removed duplicate clearing in
ramoops_probe.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] pstore: clarify clearing of _read_cnt in ramoops_context
2014-03-08 0:23 ` Kees Cook
@ 2014-03-08 4:09 ` Liu Shuo
0 siblings, 0 replies; 10+ messages in thread
From: Liu Shuo @ 2014-03-08 4:09 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Liu ShuoX, LKML, Tony Luck, Colin Cross,
Anton Vorontsov, yanmin_zhang
On Fri 7.Mar'14 at 16:23:29 -0800, Kees Cook wrote:
>On Fri, Mar 7, 2014 at 1:25 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX <shuox.liu@intel.com> wrote:
>>
>>>
>>> ftrace_read_cnt need to be reset in open to support mutli times
>>> getting the records.
>>>
>>> Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
>>> ---
>>> fs/pstore/ram.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>> ...
>>
>> The dump_read_cnt changes appear to be unrelated to the actual bugfix?
>
>I recommended this change since it is being cleared during open, and
>this zeroing was confusing.
>
>> If so, please send this along as a separate patch. With a full
>> changelog - this one doesn't explain the dump_read_cnt changes at all.
>
>Liu, I would recommend a changelog along the lines of:
>
>Clarify which variables need to be cleared during pstore_open. Added
>missed ftrace_read_cnt clearing and removed duplicate clearing in
>ramoops_probe.
>
>-Kees
Thanks. It's more clearly. I copied some of your comments into changelog. :)
Re-sent the patch with the new changelog. The subject also changed.
----
From: Liu ShuoX <shuox.liu@intel.com>
*_read_cnt in ramoops_context need to be cleared during pstore ->open
to support mutli times getting the records. The patch added missed
ftrace_read_cnt clearing and removed duplicate clearing in ramoops_probe.
Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
---
fs/pstore/ram.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index fa8cef2..9fe5b13 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -86,6 +86,7 @@ struct ramoops_context {
struct persistent_ram_ecc_info ecc_info;
unsigned int max_dump_cnt;
unsigned int dump_write_cnt;
+ /* _read_cnt need clear on ramoops_pstore_open */
unsigned int dump_read_cnt;
unsigned int console_read_cnt;
unsigned int ftrace_read_cnt;
@@ -101,6 +102,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
cxt->dump_read_cnt = 0;
cxt->console_read_cnt = 0;
+ cxt->ftrace_read_cnt = 0;
return 0;
}
@@ -428,7 +430,6 @@ static int ramoops_probe(struct platform_device *pdev)
if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
- cxt->dump_read_cnt = 0;
cxt->size = pdata->mem_size;
cxt->phys_addr = pdata->mem_address;
cxt->record_size = pdata->record_size;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-03-08 4:09 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-28 6:37 [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open shuox.liu
2014-03-02 9:03 ` Zhang, Yanmin
2014-03-03 19:45 ` Kees Cook
2014-03-04 1:40 ` Liu ShuoX
2014-03-04 19:11 ` Kees Cook
2014-03-07 2:58 ` Liu ShuoX
2014-03-07 5:43 ` Kees Cook
2014-03-07 21:25 ` Andrew Morton
2014-03-08 0:23 ` Kees Cook
2014-03-08 4:09 ` [PATCH] pstore: clarify clearing of _read_cnt in ramoops_context Liu Shuo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).