Re: [PATCH] pstore: reset ftrace_read_cnt at ramoops_pstore_open

[Liu ShuoX]

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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> > > > From: Liu ShuoX <shuox.liu@xxxxxxxxx>
> > > >
> > > > ftrace_read_cnt need to be reset in open to support mutli times
> > > > getting the records.
> > > >
> > > > Signed-off-by: Liu ShuoX <shuox.liu@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxx>

ftrace_read_cnt need to be reset in open to support mutli times
getting the records.

Signed-off-by: Liu ShuoX <shuox.liu@xxxxxxxxx>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/