mtd: set mtd partition panic write flag

PATCH In review Kamal Dasu 4 participants 10 messages Oct 21, 2019 – May 4, 2020 Memory technology devices (MTD)

Share & tools

Kamal Dasu

author

2019-10-21 19:34 UTC #0 lore raw

PATCH 1/1 mtd: set mtd partition panic write flag

Check mtd panic write flag and set the mtd partition panic write flag so that low level drivers can use it to take required action to ensure oops data gets written to assigned mtd partition.

Fixes 9f897bfdd8 ("mtd: Add flag to indicate panic_write")

Signed-off-by Kamal Dasu <[email protected]>

---
 drivers/mtd/mtdpart.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/mtd/mtdpart.c +4 -0

index 7328c066c5ba..b4f6479abeda 100644
@@ -159,6 +159,10 @@ static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
 		size_t *retlen, const u_char *buf)
 {
 	struct mtd_part *part = mtd_to_part(mtd);
+
+	if (mtd->oops_panic_write)
+		part->parent->oops_panic_write = true;
+
 	return part->parent->_panic_write(part->parent, to + part->offset, len,
 					  retlen, buf);
 }

Signature · 1 line

2.17.1

Miquel Raynal

maintainer

2019-11-05 19:03 UTC #1 lore raw

Hi Kamal,

Richard, something to look into below :)

Kamal Dasu <[email protected]> wrote on Mon, 21 Oct 2019 15:32:52 -0400:

Quoted 4 lines

Check mtd panic write flag and set the mtd partition panic write flag so that low level drivers can use it to take required action to ensure oops data gets written to assigned mtd partition.

I feel there is something wrong with the current implementation regarding partitions but I am not sure this is the right fix. Is this something you detected with some kind of static checker or did you actually experience an issue?

In the commit log you say "check mtd (I suppose you mean the master) panic write flag and set the mtd partition panic write flag" which makes sense, but in reality my understanding is that you do the
opposite: you check mtd->oops_panic_write which is the partitions' structure, and set part->parent->oops_panic_write which is the master's flag.

Also, I am not sure if it is worth checking anything, why not just set mtd->oops_panic_write in this function?

Same comment for the -already existing- condition in mtd_panic_write. As soon as we are in these functions, we know there is a panic, right? So why checking if the bit is already set before forcing it?

Quoted 12 lines

Fixes 9f897bfdd8 ("mtd: Add flag to indicate panic_write")
Signed-off-by Kamal Dasu <[email protected]>
---
 drivers/mtd/mtdpart.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mtd/mtdpart.c +0 -0
index 7328c066c5ba..b4f6479abeda 100644

drivers/mtd/mtdpart.c static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len, · line 168

Quoted 10 lines

drivers/mtd/mtdpart.c +4 -0

@@ -159,6 +159,10 @@ static int part_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
 		size_t *retlen, const u_char *buf)
 {
 	struct mtd_part *part = mtd_to_part(mtd);
+
+	if (mtd->oops_panic_write)
+		part->parent->oops_panic_write = true;
+
 	return part->parent->_panic_write(part->parent, to + part->offset, len,
 					  retlen, buf);
 }

Thanks,
Miquèl

Richard Weinberger

maintainer

2019-11-05 23:03 UTC #2 lore raw

----- Ursprüngliche Mail -----

Quoted 9 lines

Von: "Miquel Raynal" <[email protected]>
An: "Kamal Dasu" <[email protected]>
CC: "linux-mtd" <[email protected]>, "bcm-kernel-feedback-list" <[email protected]>, "linux-kernel" <[email protected]>, "David Woodhouse" <[email protected]>, "Brian Norris" <[email protected]>, "Marek Vasut" <[email protected]>, "richard" <[email protected]>, "Vignesh Raghavendra" <[email protected]>
Gesendet: Dienstag, 5. November 2019 20:03:44
Betreff: Re: [PATCH] mtd: set mtd partition panic write flag

Quoted 3 lines

Hi Kamal,
Richard, something to look into below :)

I'm still recovering from a bad cold. So my brain is not fully working ;)

Quoted 13 lines

Kamal Dasu <[email protected]> wrote on Mon, 21 Oct 2019 15:32:52 -0400:
Quoted 4 lines
Check mtd panic write flag and set the mtd partition panic write flag so that low level drivers can use it to take required action to ensure oops data gets written to assigned mtd partition.
I feel there is something wrong with the current implementation regarding partitions but I am not sure this is the right fix. Is this something you detected with some kind of static checker or did you actually experience an issue?

Quoted 6 lines

In the commit log you say "check mtd (I suppose you mean the master) panic write flag and set the mtd partition panic write flag" which makes sense, but in reality my understanding is that you do the
opposite: you check mtd->oops_panic_write which is the partitions' structure, and set part->parent->oops_panic_write which is the master's flag.

IIUC the problem happens when you run mtdoops on a mtd partition. The the flag is only set for the partition instead for the master.

So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c

Thanks,
//richard

Kamal Dasu

author

2019-11-11 20:36 UTC #3 lore raw

Richard,

Quoted 11 lines · Richard Weinberger

----- Ursprüngliche Mail -----
Quoted 8 lines
Von: "Miquel Raynal" <[email protected]>
An: "Kamal Dasu" <[email protected]>
CC: "linux-mtd" <[email protected]>, "bcm-kernel-feedback-list" <[email protected]>, "linux-kernel" <[email protected]>, "David Woodhouse" <[email protected]>, "Brian Norris" <[email protected]>, "Marek Vasut" <[email protected]>, "richard" <[email protected]>, "Vignesh Raghavendra" <[email protected]>
Gesendet: Dienstag, 5. November 2019 20:03:44
Betreff: Re: [PATCH] mtd: set mtd partition panic write flag

Quoted 5 lines

Quoted 3 lines
Hi Kamal,
Richard, something to look into below :)
I'm still recovering from a bad cold. So my brain is not fully working ;)

Thanks for reviewing this. Hope you are feeling better now.

Quoted 21 lines

Quoted 14 lines
Kamal Dasu <[email protected]> wrote on Mon, 21 Oct 2019 15:32:52 -0400:
Quoted 4 lines
Check mtd panic write flag and set the mtd partition panic write flag so that low level drivers can use it to take required action to ensure oops data gets written to assigned mtd partition.
I feel there is something wrong with the current implementation regarding partitions but I am not sure this is the right fix. Is this something you detected with some kind of static checker or did you actually experience an issue?
Quoted 6 lines
In the commit log you say "check mtd (I suppose you mean the master) panic write flag and set the mtd partition panic write flag" which makes sense, but in reality my understanding is that you do the
opposite: you check mtd->oops_panic_write which is the partitions' structure, and set part->parent->oops_panic_write which is the master's flag.

Quoted 4 lines

IIUC the problem happens when you run mtdoops on a mtd partition. The the flag is only set for the partition instead for the master.
So the right fix would be setting the parent's oops_panic_write in

How do I get access to the parts parent in the core ?. Maybe I am missing something.

Quoted 5 lines

mtd_panic_write().
Then we don't have to touch mtdpart.c
Thanks,
//richard

Miquel Raynal

maintainer

2020-01-09 15:03 UTC #4 lore raw

In reply to Richard Weinberger

Hello,

Richard Weinberger <[email protected]> wrote on Wed, 6 Nov 2019 00:03:42 +0100 (CET):

Quoted 36 lines

----- Ursprüngliche Mail -----
Quoted 9 lines
Von: "Miquel Raynal" <[email protected]>
An: "Kamal Dasu" <[email protected]>
CC: "linux-mtd" <[email protected]>, "bcm-kernel-feedback-list" <[email protected]>, "linux-kernel" <[email protected]>, "David Woodhouse" <[email protected]>, "Brian Norris" <[email protected]>, "Marek Vasut" <[email protected]>, "richard" <[email protected]>, "Vignesh Raghavendra" <[email protected]>
Gesendet: Dienstag, 5. November 2019 20:03:44
Betreff: Re: [PATCH] mtd: set mtd partition panic write flag
Quoted 3 lines
Hi Kamal,
Richard, something to look into below :)
I'm still recovering from a bad cold. So my brain is not fully working ;)
Quoted 13 lines
Kamal Dasu <[email protected]> wrote on Mon, 21 Oct 2019 15:32:52 -0400:
Quoted 4 lines
Check mtd panic write flag and set the mtd partition panic write flag so that low level drivers can use it to take required action to ensure oops data gets written to assigned mtd partition.
I feel there is something wrong with the current implementation regarding partitions but I am not sure this is the right fix. Is this something you detected with some kind of static checker or did you actually experience an issue?
Quoted 6 lines
In the commit log you say "check mtd (I suppose you mean the master) panic write flag and set the mtd partition panic write flag" which makes sense, but in reality my understanding is that you do the
opposite: you check mtd->oops_panic_write which is the partitions' structure, and set part->parent->oops_panic_write which is the master's flag.

Quoted 7 lines

IIUC the problem happens when you run mtdoops on a mtd partition. The the flag is only set for the partition instead for the master.
So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c

This issue is still open, right? Kamal can you send an updated version?

Thanks,
Miquèl

Kamal Dasu

2020-01-09 15:26 UTC #5 lore raw

Miquel,

Yes the issue is still open. I was trying to understand the suggestion and did not get a reply on the question I had

Richard wrote :
"So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c"

How do I get access to the parts parent in the core ?. Maybe I am missing something.

Kamal

Quoted 55 lines · Miquel Raynal

Hello,
Richard Weinberger <[email protected]> wrote on Wed, 6 Nov 2019 00:03:42 +0100 (CET):
Quoted 36 lines
----- Ursprüngliche Mail -----
Quoted 9 lines
Von: "Miquel Raynal" <[email protected]>
An: "Kamal Dasu" <[email protected]>
CC: "linux-mtd" <[email protected]>, "bcm-kernel-feedback-list" <[email protected]>, "linux-kernel" <[email protected]>, "David Woodhouse" <[email protected]>, "Brian Norris" <[email protected]>, "Marek Vasut" <[email protected]>, "richard" <[email protected]>, "Vignesh Raghavendra" <[email protected]>
Gesendet: Dienstag, 5. November 2019 20:03:44
Betreff: Re: [PATCH] mtd: set mtd partition panic write flag
Quoted 3 lines
Hi Kamal,
Richard, something to look into below :)
I'm still recovering from a bad cold. So my brain is not fully working ;)
Quoted 13 lines
Kamal Dasu <[email protected]> wrote on Mon, 21 Oct 2019 15:32:52 -0400:
Quoted 4 lines
Check mtd panic write flag and set the mtd partition panic write flag so that low level drivers can use it to take required action to ensure oops data gets written to assigned mtd partition.
I feel there is something wrong with the current implementation regarding partitions but I am not sure this is the right fix. Is this something you detected with some kind of static checker or did you actually experience an issue?
Quoted 6 lines
In the commit log you say "check mtd (I suppose you mean the master) panic write flag and set the mtd partition panic write flag" which makes sense, but in reality my understanding is that you do the
opposite: you check mtd->oops_panic_write which is the partitions' structure, and set part->parent->oops_panic_write which is the master's flag.
Quoted 7 lines
IIUC the problem happens when you run mtdoops on a mtd partition. The the flag is only set for the partition instead for the master.
So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c
This issue is still open, right? Kamal can you send an updated version?
Thanks,
Miquèl

Miquel Raynal

maintainer

2020-01-09 17:28 UTC #6 lore raw

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Thu, 9 Jan 2020 10:25:59 -0500:

Quoted 5 lines

Miquel,
Yes the issue is still open. I was trying to understand the suggestion and did not get a reply on the question I had

Quoted 7 lines

Richard wrote :
"So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c"
How do I get access to the parts parent in the core ?. Maybe I am missing something.

I think the solution is to set the oops_panic_write of the root parent, instead of updating the flag of the mtd device itself (which is maybe a partition).

Would this help?

https://www.spinics.net/lists/linux-mtd/msg10454.html

Thanks,
Miquèl

Miquel Raynal

maintainer

2020-05-02 18:08 UTC #7 lore raw

Hi Kamal,

Miquel Raynal <[email protected]> wrote on Thu, 9 Jan 2020 18:28:07 +0100:

Quoted 15 lines

Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 9 Jan 2020 10:25:59 -0500:
Quoted 10 lines
Miquel,
Yes the issue is still open. I was trying to understand the suggestion and did not get a reply on the question I had
Richard wrote :
"So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c"

Quoted 8 lines

Quoted 2 lines
How do I get access to the parts parent in the core ?. Maybe I am missing something.
I think the solution is to set the oops_panic_write of the root parent, instead of updating the flag of the mtd device itself (which is maybe a partition).
Would this help?
https://www.spinics.net/lists/linux-mtd/msg10454.html

I'm pinging you here as well, as I think you raise a real issue, and we agreed on a solution, which can now be easily setup with the above change which has been applied and support for functions like:

static inline struct mtd_info *mtd_get_master(struct mtd_info *mtd) static inline u64 mtd_get_master_ofs(struct mtd_info *mtd, u64 ofs) static inline bool mtd_is_partition(const struct mtd_info *mtd) static inline bool mtd_has_partitions(const struct mtd_info *mtd)

Thanks,
Miquèl

Kamal Dasu

2020-05-04 15:20 UTC #8 lore raw

Quoted 30 lines · Miquel Raynal

Hi Kamal,
Miquel Raynal <[email protected]> wrote on Thu, 9 Jan 2020 18:28:07 +0100:
Quoted 15 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 9 Jan 2020 10:25:59 -0500:
Quoted 10 lines
Miquel,
Yes the issue is still open. I was trying to understand the suggestion and did not get a reply on the question I had
Richard wrote :
"So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c"
Quoted 8 lines
Quoted 2 lines
How do I get access to the parts parent in the core ?. Maybe I am missing something.
I think the solution is to set the oops_panic_write of the root parent, instead of updating the flag of the mtd device itself (which is maybe a partition).
Would this help?
https://www.spinics.net/lists/linux-mtd/msg10454.html

Quoted 9 lines

I'm pinging you here as well, as I think you raise a real issue, and we agreed on a solution, which can now be easily setup with the above change which has been applied and support for functions like:
static inline struct mtd_info *mtd_get_master(struct mtd_info *mtd) static inline u64 mtd_get_master_ofs(struct mtd_info *mtd, u64 ofs) static inline bool mtd_is_partition(const struct mtd_info *mtd) static inline bool mtd_has_partitions(const struct mtd_info *mtd)

So I should only set master->oops_panic_write with the new code ?.

Quoted 2 lines

Thanks,
Miquèl

Kamal

Miquel Raynal

maintainer

2020-05-04 17:30 UTC #9 lore raw

Hi Kamal,

Kamal Dasu <[email protected]> wrote on Mon, 4 May 2020 11:20:16 -0400:

Quoted 35 lines · Miquel Raynal

Quoted 34 lines
Hi Kamal,
Miquel Raynal <[email protected]> wrote on Thu, 9 Jan 2020 18:28:07 +0100:
Quoted 15 lines
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Thu, 9 Jan 2020 10:25:59 -0500:
Quoted 10 lines
Miquel,
Yes the issue is still open. I was trying to understand the suggestion and did not get a reply on the question I had
Richard wrote :
"So the right fix would be setting the parent's oops_panic_write in mtd_panic_write().
Then we don't have to touch mtdpart.c"
Quoted 8 lines
Quoted 2 lines
How do I get access to the parts parent in the core ?. Maybe I am missing something.
I think the solution is to set the oops_panic_write of the root parent, instead of updating the flag of the mtd device itself (which is maybe a partition).
Would this help?
https://www.spinics.net/lists/linux-mtd/msg10454.html
I'm pinging you here as well, as I think you raise a real issue, and we agreed on a solution, which can now be easily setup with the above change which has been applied and support for functions like:

Quoted 7 lines

Quoted 5 lines
static inline struct mtd_info *mtd_get_master(struct mtd_info *mtd) static inline u64 mtd_get_master_ofs(struct mtd_info *mtd, u64 ofs) static inline bool mtd_is_partition(const struct mtd_info *mtd) static inline bool mtd_has_partitions(const struct mtd_info *mtd)
So I should only set master->oops_panic_write with the new code ?.

Yes, if you can still reproduce the issue and it solves your problem, then it's I think a nice fix.

Thanks,
Miquèl