pstore: reset ftrace_read_cnt at ramoops_pstore_open

PATCH In review [email protected] 5 participants 10 messages Feb 28, 2014 – Mar 8, 2014 Pstore filesystem

lore

Share & tools

lore

[email protected]

author

2014-02-28 06:38 UTC #0 lore raw

PATCH 1/1 pstore: reset ftrace_read_cnt at ramoops_pstore_open

From: Liu ShuoX <[email protected]>

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

Signed-off-by Liu ShuoX <[email protected]>

---
 fs/pstore/ram.c | 1 +
 1 file changed, 1 insertion(+)

fs/pstore/ram.c +1 -0

index fa8cef2..a5d0cab 100644
@@ -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;
 }

Signature · 1 line

1.8.3.2

Zhang, Yanmin

2014-03-02 09:04 UTC #1 lore raw

Quoted 4 lines · On 2014/2/28 14:37, [email protected] wrote:

From: Liu ShuoX <[email protected]>
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

Quoted 18 lines

Signed-off-by Liu ShuoX <[email protected]>

---
  fs/pstore/ram.c | 1 +
  1 file changed, 1 insertion(+)

fs/pstore/ram.c +1 -0

index fa8cef2..a5d0cab 100644
@@ -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;
  }

Kees Cook

2014-03-03 19:46 UTC #2 lore raw

In reply to [email protected]

Quoted 15 lines · On Thu, Feb 27, 2014 at 10:37 PM, <[email protected]> wrote:

From: Liu ShuoX <[email protected]>
ftrace_read_cnt need to be reset in open to support mutli times getting the records.
Signed-off-by Liu ShuoX <[email protected]>
---
 fs/pstore/ram.c | 1 +
 1 file changed, 1 insertion(+)
fs/pstore/ram.c +0 -0
index fa8cef2..a5d0cab 100644

fs/pstore/ram.c static int ramoops_pstore_open(struct pstore_info *psi) · line 106

Quoted 6 lines

fs/pstore/ram.c +1 -0

@@ -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

Signature · 2 lines

Kees Cook
Chrome OS Security

Liu ShuoX

author

2014-03-04 01:40 UTC #3 lore raw

Quoted 17 lines · Kees Cook

Quoted 16 lines · On Thu, Feb 27, 2014 at 10:37 PM, <[email protected]> wrote:
From: Liu ShuoX <[email protected]>
ftrace_read_cnt need to be reset in open to support mutli times getting the records.
Signed-off-by Liu ShuoX <[email protected]>
---
 fs/pstore/ram.c | 1 +
 1 file changed, 1 insertion(+)
fs/pstore/ram.c +0 -0
index fa8cef2..a5d0cab 100644
@@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
 

Quoted 10 lines

fs/pstore/ram.c static int ramoops_pstore_open(struct pstore_info *psi) · line 106
Quoted 5 lines
fs/pstore/ram.c +1 -0
        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'?

Quoted 6 lines

-Kees
Signature · 2 lines
Kees Cook
Chrome OS Security

Kees Cook

2014-03-04 19:11 UTC #4 lore raw

Quoted 33 lines · Liu ShuoX

Quoted 20 lines · Kees Cook
Quoted 18 lines · On Thu, Feb 27, 2014 at 10:37 PM, <[email protected]> wrote:
From: Liu ShuoX <[email protected]>
ftrace_read_cnt need to be reset in open to support mutli times getting the records.
Signed-off-by Liu ShuoX <[email protected]>
---
 fs/pstore/ram.c | 1 +
 1 file changed, 1 insertion(+)
fs/pstore/ram.c +0 -0
index fa8cef2..a5d0cab 100644
@@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info
 *psi)
 
Quoted 11 lines
fs/pstore/ram.c static int ramoops_pstore_open(struct pstore_info *psi) · line 106
Quoted 5 lines
fs/pstore/ram.c +1 -0
        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?

Quoted 4 lines

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

Signature · 2 lines

Kees Cook
Chrome OS Security

Liu ShuoX

author

2014-03-07 02:58 UTC #5 lore raw

Quoted 44 lines · Kees Cook

Quoted 33 lines · Liu ShuoX
Quoted 20 lines · Kees Cook
Quoted 18 lines · On Thu, Feb 27, 2014 at 10:37 PM, <[email protected]> wrote:
From: Liu ShuoX <[email protected]>
ftrace_read_cnt need to be reset in open to support mutli times getting the records.
Signed-off-by Liu ShuoX <[email protected]>
---
 fs/pstore/ram.c | 1 +
 1 file changed, 1 insertion(+)
fs/pstore/ram.c +0 -0
index fa8cef2..a5d0cab 100644
@@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info
 *psi)
 
Quoted 11 lines
fs/pstore/ram.c static int ramoops_pstore_open(struct pstore_info · line 107
Quoted 5 lines
fs/pstore/ram.c +1 -0
        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?
Quoted 4 lines
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).

Quoted 6 lines

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 <[email protected]>

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

Signed-off-by Liu ShuoX <[email protected]>

---
 fs/pstore/ram.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/pstore/ram.c +2 -1

index fa8cef2..9fe5b13 100644
@@ -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;

Signature · 1 line

1.8.3.2

Kees Cook

Reviewed-by

2014-03-07 05:44 UTC #6 lore raw

Quoted 99 lines · Liu ShuoX

Quoted 51 lines · Kees Cook
Quoted 38 lines · Liu ShuoX
Quoted 22 lines · Kees Cook
Quoted 19 lines · On Thu, Feb 27, 2014 at 10:37 PM, <[email protected]> wrote:
From: Liu ShuoX <[email protected]>
ftrace_read_cnt need to be reset in open to support mutli times getting the records.
Signed-off-by Liu ShuoX <[email protected]>
---
 fs/pstore/ram.c | 1 +
 1 file changed, 1 insertion(+)
fs/pstore/ram.c +0 -0
index fa8cef2..a5d0cab 100644
@@ -101,6 +101,7 @@ static int ramoops_pstore_open(struct pstore_info
 *psi)
 
Quoted 12 lines
fs/pstore/ram.c static int ramoops_pstore_open(struct pstore_info *psi) · line 107
Quoted 5 lines
fs/pstore/ram.c +1 -0
        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?
Quoted 4 lines
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).
Quoted 6 lines
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 <[email protected]>
ftrace_read_cnt need to be reset in open to support mutli times getting the records.
Signed-off-by Liu ShuoX <[email protected]>
---
 fs/pstore/ram.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
fs/pstore/ram.c +2 -0
index fa8cef2..9fe5b13 100644
@@ -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;

Quoted 3 lines

--
1.8.3.2

Thanks! That looks right to me.

Reviewed-by Kees Cook <[email protected]>

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security

Andrew Morton

2014-03-07 21:25 UTC #7 lore raw

In reply to Liu ShuoX

Quoted 30 lines · On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX <[email protected]> wrote:

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

Signed-off-by Liu ShuoX <[email protected]>

---
  fs/pstore/ram.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

fs/pstore/ram.c +2 -0

index fa8cef2..9fe5b13 100644
@@ -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;
  }

fs/pstore/ram.c static int ramoops_probe(struct platform_device *pdev) · line 435

Quoted 7 lines

fs/pstore/ram.c +0 -1

@@ -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.

Kees Cook

2014-03-08 00:23 UTC #8 lore raw

Quoted 35 lines · Andrew Morton

Quoted 33 lines · On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX <[email protected]> wrote:

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

Signed-off-by Liu ShuoX <[email protected]>

---
  fs/pstore/ram.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

fs/pstore/ram.c +2 -0

index fa8cef2..9fe5b13 100644
@@ -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);

Quoted 6 lines

fs/pstore/ram.c static int ramoops_probe(struct platform_device *pdev) · line 435
Quoted 4 lines
fs/pstore/ram.c +0 -1
-     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.

Quoted 2 lines

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

Signature · 2 lines

Kees Cook
Chrome OS Security

Liu Shuo

2014-03-08 04:09 UTC #9 lore raw

PATCH 1/1 pstore: clarify clearing of _read_cnt in ramoops_context

Quoted 21 lines · Kees Cook

Quoted 6 lines · Andrew Morton
Quoted 4 lines · On Fri, 7 Mar 2014 10:58:43 +0800 Liu ShuoX <[email protected]> wrote:
ftrace_read_cnt need to be reset in open to support mutli times getting the records.
Quoted 7 lines
Quoted 5 lines
Signed-off-by Liu ShuoX <[email protected]>
---
  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.
Quoted 2 lines
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.

Quoted 7 lines

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 <[email protected]>

*_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 <[email protected]>

---
fs/pstore/ram.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

fs/pstore/ram.c +0 -0

index fa8cef2..9fe5b13 100644
@@ -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;

Signature · 1 line

1.8.3.2