* [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