[v3] mtd: mtdoops: Add timestamp to the dumped oops header.

[PATCH v3 0/3] mtd: mtdoops: Add timestamp to the dumped oops header.

Posted by Jean-Marc Eurin 4 years ago

The current header consists of 2 32-bit values which makes it difficult
and error prone to expand. This creates a structure for the header.

Some oops only have time relative to boot, this adds an absolute timestamp.

Changelog since v2:
- Add a timestamp to the header

Changelog since v1:
- Create a header structure to simplify code.
- Patches in series.
- Patch prefix.

Jean-Marc Eurin (3):
  mtd: mtdoops: Fix the size of the header read buffer.
  mtd: mtdoops: Create a header structure for the saved mtdoops.
  mtd: mtdoops: Add a timestamp to the mtdoops header.

 drivers/mtd/mtdoops.c | 61 +++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

-- 
2.36.0.rc0.470.gd361397f0d-goog

[PATCH v4 0/3] mtd: mtdoops: Add timestamp to the dumped oops header.

Posted by Jean-Marc Eurin 4 years ago

The current header consists of 2 32-bit values which makes it difficult
and error prone to expand. This creates a structure for the header.

Some oops only have time relative to boot, this adds an absolute timestamp.

Changelog since v3:
  Fix the printk format for sizeof to %zu.

Changelog since v2:
- Add a timestamp to the header.

Changelog since v1:
- Create a header structure to simplify code.
- Patches in series.
- Patch prefix.

Jean-Marc Eurin (3):
  mtd: mtdoops: Fix the size of the header read buffer.
  mtd: mtdoops: Create a header structure for the saved mtdoops.
  mtd: mtdoops: Add a timestamp to the mtdoops header.

 drivers/mtd/mtdoops.c | 61 +++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [PATCH v4 0/3] mtd: mtdoops: Add timestamp to the dumped oops header.

Posted by Miquel Raynal 4 years ago

Hi Jean-Marc,

jmeurin@google.com wrote on Thu, 21 Apr 2022 16:42:41 -0700:

> The current header consists of 2 32-bit values which makes it difficult
> and error prone to expand. This creates a structure for the header.
> 
> Some oops only have time relative to boot, this adds an absolute timestamp.
> 
> Changelog since v3:
>   Fix the printk format for sizeof to %zu.
> 
> Changelog since v2:
> - Add a timestamp to the header.
> 
> Changelog since v1:
> - Create a header structure to simplify code.
> - Patches in series.
> - Patch prefix.
> 
> Jean-Marc Eurin (3):
>   mtd: mtdoops: Fix the size of the header read buffer.
>   mtd: mtdoops: Create a header structure for the saved mtdoops.
>   mtd: mtdoops: Add a timestamp to the mtdoops header.

Looks like the last patch was not sent to the list?

Re: [PATCH v4 0/3] mtd: mtdoops: Add timestamp to the dumped oops header.

Posted by Jean-Marc Eurin 4 years ago

Hi Miquel,


On Mon, Apr 25, 2022 at 1:42 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Jean-Marc,
>
> jmeurin@google.com wrote on Thu, 21 Apr 2022 16:42:41 -0700:
>
> > The current header consists of 2 32-bit values which makes it difficult
> > and error prone to expand. This creates a structure for the header.
> >
> > Some oops only have time relative to boot, this adds an absolute timestamp.
> >
> > Changelog since v3:
> >   Fix the printk format for sizeof to %zu.
> >
> > Changelog since v2:
> > - Add a timestamp to the header.
> >
> > Changelog since v1:
> > - Create a header structure to simplify code.
> > - Patches in series.
> > - Patch prefix.
> >
> > Jean-Marc Eurin (3):
> >   mtd: mtdoops: Fix the size of the header read buffer.
> >   mtd: mtdoops: Create a header structure for the saved mtdoops.
> >   mtd: mtdoops: Add a timestamp to the mtdoops header.
>
> Looks like the last patch was not sent to the list?
>

I'm not sure what happened.  I've just sent it and then noticed that
the mail threading is wrong.  Let me know if you want me to resend the
full thread.

I'm really sorry about that :-( and thanks for all your help.

Jean-Marc

Re: [PATCH v4 0/3] mtd: mtdoops: Add timestamp to the dumped oops header.

Posted by Miquel Raynal 4 years ago

Hi Jean-Marc,

jmeurin@google.com wrote on Mon, 25 Apr 2022 09:14:08 -0700:

> Hi Miquel,
> 
> 
> On Mon, Apr 25, 2022 at 1:42 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Jean-Marc,
> >
> > jmeurin@google.com wrote on Thu, 21 Apr 2022 16:42:41 -0700:
> >  
> > > The current header consists of 2 32-bit values which makes it difficult
> > > and error prone to expand. This creates a structure for the header.
> > >
> > > Some oops only have time relative to boot, this adds an absolute timestamp.
> > >
> > > Changelog since v3:
> > >   Fix the printk format for sizeof to %zu.
> > >
> > > Changelog since v2:
> > > - Add a timestamp to the header.
> > >
> > > Changelog since v1:
> > > - Create a header structure to simplify code.
> > > - Patches in series.
> > > - Patch prefix.
> > >
> > > Jean-Marc Eurin (3):
> > >   mtd: mtdoops: Fix the size of the header read buffer.
> > >   mtd: mtdoops: Create a header structure for the saved mtdoops.
> > >   mtd: mtdoops: Add a timestamp to the mtdoops header.  
> >
> > Looks like the last patch was not sent to the list?
> >  
> 
> I'm not sure what happened.  I've just sent it and then noticed that
> the mail threading is wrong.  Let me know if you want me to resend the
> full thread.

It's fine like that.

> 
> I'm really sorry about that :-( and thanks for all your help.

No problem!

Cheers,
Miquèl

[PATCH v4 1/3] mtd: mtdoops: Fix the size of the header read buffer.

Posted by Jean-Marc Eurin 4 years ago

The read buffer size depends on the MTDOOPS_HEADER_SIZE.

Tested: Changed the header size, it doesn't panic, header is still
read/written correctly.

Signed-off-by: Jean-Marc Eurin <jmeurin@google.com>
---
 drivers/mtd/mtdoops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 227df24387df..09a26747f490 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -223,7 +223,7 @@ static void find_next_position(struct mtdoops_context *cxt)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	int ret, page, maxpos = 0;
-	u32 count[2], maxcount = 0xffffffff;
+	u32 count[MTDOOPS_HEADER_SIZE/sizeof(u32)], maxcount = 0xffffffff;
 	size_t retlen;
 
 	for (page = 0; page < cxt->oops_pages; page++) {
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [PATCH v4 1/3] mtd: mtdoops: Fix the size of the header read buffer.

Posted by Miquel Raynal 4 years ago

On Thu, 2022-04-21 at 23:42:42 UTC, Jean-Marc Eurin wrote:
> The read buffer size depends on the MTDOOPS_HEADER_SIZE.
> 
> Tested: Changed the header size, it doesn't panic, header is still
> read/written correctly.
> 
> Signed-off-by: Jean-Marc Eurin <jmeurin@google.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

[PATCH v4 2/3] mtd: mtdoops: Create a header structure for the saved mtdoops.

Posted by Jean-Marc Eurin 4 years ago

Create a dump header to enable the addition of fields without having
to modify the rest of the code.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Jean-Marc Eurin <jmeurin@google.com>
---
 drivers/mtd/mtdoops.c | 55 +++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 09a26747f490..186eeb01bee1 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -22,9 +22,6 @@
 /* Maximum MTD partition size */
 #define MTDOOPS_MAX_MTD_SIZE (8 * 1024 * 1024)
 
-#define MTDOOPS_KERNMSG_MAGIC 0x5d005d00
-#define MTDOOPS_HEADER_SIZE   8
-
 static unsigned long record_size = 4096;
 module_param(record_size, ulong, 0400);
 MODULE_PARM_DESC(record_size,
@@ -40,6 +37,13 @@ module_param(dump_oops, int, 0600);
 MODULE_PARM_DESC(dump_oops,
 		"set to 1 to dump oopses, 0 to only dump panics (default 1)");
 
+#define MTDOOPS_KERNMSG_MAGIC 0x5d005d00
+
+struct mtdoops_hdr {
+	u32 seq;
+	u32 magic;
+} __packed;
+
 static struct mtdoops_context {
 	struct kmsg_dumper dump;
 
@@ -178,16 +182,16 @@ static void mtdoops_write(struct mtdoops_context *cxt, int panic)
 {
 	struct mtd_info *mtd = cxt->mtd;
 	size_t retlen;
-	u32 *hdr;
+	struct mtdoops_hdr *hdr;
 	int ret;
 
 	if (test_and_set_bit(0, &cxt->oops_buf_busy))
 		return;
 
 	/* Add mtdoops header to the buffer */
-	hdr = cxt->oops_buf;
-	hdr[0] = cxt->nextcount;
-	hdr[1] = MTDOOPS_KERNMSG_MAGIC;
+	hdr = (struct mtdoops_hdr *)cxt->oops_buf;
+	hdr->seq = cxt->nextcount;
+	hdr->magic = MTDOOPS_KERNMSG_MAGIC;
 
 	if (panic) {
 		ret = mtd_panic_write(mtd, cxt->nextpage * record_size,
@@ -222,8 +226,9 @@ static void mtdoops_workfunc_write(struct work_struct *work)
 static void find_next_position(struct mtdoops_context *cxt)
 {
 	struct mtd_info *mtd = cxt->mtd;
+	struct mtdoops_hdr hdr;
 	int ret, page, maxpos = 0;
-	u32 count[MTDOOPS_HEADER_SIZE/sizeof(u32)], maxcount = 0xffffffff;
+	u32 maxcount = 0xffffffff;
 	size_t retlen;
 
 	for (page = 0; page < cxt->oops_pages; page++) {
@@ -231,32 +236,31 @@ static void find_next_position(struct mtdoops_context *cxt)
 			continue;
 		/* Assume the page is used */
 		mark_page_used(cxt, page);
-		ret = mtd_read(mtd, page * record_size, MTDOOPS_HEADER_SIZE,
-			       &retlen, (u_char *)&count[0]);
-		if (retlen != MTDOOPS_HEADER_SIZE ||
+		ret = mtd_read(mtd, page * record_size, sizeof(hdr),
+			       &retlen, (u_char *)&hdr);
+		if (retlen != sizeof(hdr) ||
 				(ret < 0 && !mtd_is_bitflip(ret))) {
-			printk(KERN_ERR "mtdoops: read failure at %ld (%td of %d read), err %d\n",
-			       page * record_size, retlen,
-			       MTDOOPS_HEADER_SIZE, ret);
+			printk(KERN_ERR "mtdoops: read failure at %ld (%zu of %zu read), err %d\n",
+			       page * record_size, retlen, sizeof(hdr), ret);
 			continue;
 		}
 
-		if (count[0] == 0xffffffff && count[1] == 0xffffffff)
+		if (hdr.seq == 0xffffffff && hdr.magic == 0xffffffff)
 			mark_page_unused(cxt, page);
-		if (count[0] == 0xffffffff || count[1] != MTDOOPS_KERNMSG_MAGIC)
+		if (hdr.seq == 0xffffffff || hdr.magic != MTDOOPS_KERNMSG_MAGIC)
 			continue;
 		if (maxcount == 0xffffffff) {
-			maxcount = count[0];
+			maxcount = hdr.seq;
 			maxpos = page;
-		} else if (count[0] < 0x40000000 && maxcount > 0xc0000000) {
-			maxcount = count[0];
+		} else if (hdr.seq < 0x40000000 && maxcount > 0xc0000000) {
+			maxcount = hdr.seq;
 			maxpos = page;
-		} else if (count[0] > maxcount && count[0] < 0xc0000000) {
-			maxcount = count[0];
+		} else if (hdr.seq > maxcount && hdr.seq < 0xc0000000) {
+			maxcount = hdr.seq;
 			maxpos = page;
-		} else if (count[0] > maxcount && count[0] > 0xc0000000
+		} else if (hdr.seq > maxcount && hdr.seq > 0xc0000000
 					&& maxcount > 0x80000000) {
-			maxcount = count[0];
+			maxcount = hdr.seq;
 			maxpos = page;
 		}
 	}
@@ -287,8 +291,9 @@ static void mtdoops_do_dump(struct kmsg_dumper *dumper,
 
 	if (test_and_set_bit(0, &cxt->oops_buf_busy))
 		return;
-	kmsg_dump_get_buffer(&iter, true, cxt->oops_buf + MTDOOPS_HEADER_SIZE,
-			     record_size - MTDOOPS_HEADER_SIZE, NULL);
+	kmsg_dump_get_buffer(&iter, true,
+			     cxt->oops_buf + sizeof(struct mtdoops_hdr),
+			     record_size - sizeof(struct mtdoops_hdr), NULL);
 	clear_bit(0, &cxt->oops_buf_busy);
 
 	if (reason != KMSG_DUMP_OOPS) {
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [PATCH v4 2/3] mtd: mtdoops: Create a header structure for the saved mtdoops.

Posted by Miquel Raynal 4 years ago

On Thu, 2022-04-21 at 23:42:43 UTC, Jean-Marc Eurin wrote:
> Create a dump header to enable the addition of fields without having
> to modify the rest of the code.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Jean-Marc Eurin <jmeurin@google.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel