[PATCH] mtd: set mtd partition panic write flag

[PATCH] mtd: set mtd partition panic write flag

[Kamal Dasu]

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 <kdasu.kdev@gmail.com>
---
 drivers/mtd/mtdpart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 7328c066c5ba..b4f6479abeda 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -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);
 }
-- 
2.17.1

Re: [PATCH] mtd: set mtd partition panic write flag

[Miquel Raynal]

Hi Kamal,

Richard, something to look into below :)

Kamal Dasu <kdasu.kdev@gmail.com> wrote on Mon, 21 Oct 2019 15:32:52
-0400:

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

> 
> Fixes: 9f897bfdd8 ("mtd: Add flag to indicate panic_write")
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/mtdpart.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 7328c066c5ba..b4f6479abeda 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -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

Re: [PATCH] mtd: set mtd partition panic write flag

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> An: "Kamal Dasu" <kdasu.kdev@gmail.com>
> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "bcm-kernel-feedback-list" <bcm-kernel-feedback-list@broadcom.com>,
> "linux-kernel" <linux-kernel@vger.kernel.org>, "David Woodhouse" <dwmw2@infradead.org>, "Brian Norris"
> <computersforpeace@gmail.com>, "Marek Vasut" <marek.vasut@gmail.com>, "richard" <richard@nod.at>, "Vignesh Raghavendra"
> <vigneshr@ti.com>
> Gesendet: Dienstag, 5. November 2019 20:03:44
> Betreff: Re: [PATCH] mtd: set mtd partition panic write flag

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

I'm still recovering from a bad cold. So my brain is not fully working ;)
 
> Kamal Dasu <kdasu.kdev@gmail.com> wrote on Mon, 21 Oct 2019 15:32:52
> -0400:
> 
>> 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.

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

Re: [PATCH] mtd: set mtd partition panic write flag

[Kamal Dasu]

Richard,

On Tue, Nov 5, 2019 at 6:03 PM Richard Weinberger <richard@nod.at> wrote:
>
> ----- Ursprüngliche Mail -----
> > Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> > An: "Kamal Dasu" <kdasu.kdev@gmail.com>
> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "bcm-kernel-feedback-list" <bcm-kernel-feedback-list@broadcom.com>,
> > "linux-kernel" <linux-kernel@vger.kernel.org>, "David Woodhouse" <dwmw2@infradead.org>, "Brian Norris"
> > <computersforpeace@gmail.com>, "Marek Vasut" <marek.vasut@gmail.com>, "richard" <richard@nod.at>, "Vignesh Raghavendra"
> > <vigneshr@ti.com>
> > Gesendet: Dienstag, 5. November 2019 20:03:44
> > Betreff: Re: [PATCH] mtd: set mtd partition panic write flag
>
> > 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.

>
> > Kamal Dasu <kdasu.kdev@gmail.com> wrote on Mon, 21 Oct 2019 15:32:52
> > -0400:
> >
> >> 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.
>
> 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.

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

Re: [PATCH] mtd: set mtd partition panic write flag

[Miquel Raynal]

Hello,

Richard Weinberger <richard@nod.at> wrote on Wed, 6 Nov 2019 00:03:42
+0100 (CET):

> ----- Ursprüngliche Mail -----
> > Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> > An: "Kamal Dasu" <kdasu.kdev@gmail.com>
> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "bcm-kernel-feedback-list" <bcm-kernel-feedback-list@broadcom.com>,
> > "linux-kernel" <linux-kernel@vger.kernel.org>, "David Woodhouse" <dwmw2@infradead.org>, "Brian Norris"
> > <computersforpeace@gmail.com>, "Marek Vasut" <marek.vasut@gmail.com>, "richard" <richard@nod.at>, "Vignesh Raghavendra"
> > <vigneshr@ti.com>
> > Gesendet: Dienstag, 5. November 2019 20:03:44
> > Betreff: Re: [PATCH] mtd: set mtd partition panic write flag  
> 
> > Hi Kamal,
> > 
> > Richard, something to look into below :)  
> 
> I'm still recovering from a bad cold. So my brain is not fully working ;)
>  
> > Kamal Dasu <kdasu.kdev@gmail.com> wrote on Mon, 21 Oct 2019 15:32:52
> > -0400:
> >   
> >> 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.  
> 
> 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

Re: [PATCH] mtd: set mtd partition panic write flag

[Kamal Dasu]

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

On Thu, Jan 9, 2020 at 10:03 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hello,
>
> Richard Weinberger <richard@nod.at> wrote on Wed, 6 Nov 2019 00:03:42
> +0100 (CET):
>
> > ----- Ursprüngliche Mail -----
> > > Von: "Miquel Raynal" <miquel.raynal@bootlin.com>
> > > An: "Kamal Dasu" <kdasu.kdev@gmail.com>
> > > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "bcm-kernel-feedback-list" <bcm-kernel-feedback-list@broadcom.com>,
> > > "linux-kernel" <linux-kernel@vger.kernel.org>, "David Woodhouse" <dwmw2@infradead.org>, "Brian Norris"
> > > <computersforpeace@gmail.com>, "Marek Vasut" <marek.vasut@gmail.com>, "richard" <richard@nod.at>, "Vignesh Raghavendra"
> > > <vigneshr@ti.com>
> > > Gesendet: Dienstag, 5. November 2019 20:03:44
> > > Betreff: Re: [PATCH] mtd: set mtd partition panic write flag
> >
> > > Hi Kamal,
> > >
> > > Richard, something to look into below :)
> >
> > I'm still recovering from a bad cold. So my brain is not fully working ;)
> >
> > > Kamal Dasu <kdasu.kdev@gmail.com> wrote on Mon, 21 Oct 2019 15:32:52
> > > -0400:
> > >
> > >> 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.
> >
> > 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

Re: [PATCH] mtd: set mtd partition panic write flag

[Miquel Raynal]

Hi Kamal,

Kamal Dasu <kamal.dasu@broadcom.com> wrote on Thu, 9 Jan 2020 10:25:59
-0500:

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

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

Re: [PATCH] mtd: set mtd partition panic write flag

[Miquel Raynal]

Hi Kamal,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Thu, 9 Jan 2020
18:28:07 +0100:

> Hi Kamal,
> 
> Kamal Dasu <kamal.dasu@broadcom.com> wrote on Thu, 9 Jan 2020 10:25:59
> -0500:
> 
> > 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.  
> 
> 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

Re: [PATCH] mtd: set mtd partition panic write flag

[Kamal Dasu]

On Sat, May 2, 2020 at 2:08 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Kamal,
>
> Miquel Raynal <miquel.raynal@bootlin.com> wrote on Thu, 9 Jan 2020
> 18:28:07 +0100:
>
> > Hi Kamal,
> >
> > Kamal Dasu <kamal.dasu@broadcom.com> wrote on Thu, 9 Jan 2020 10:25:59
> > -0500:
> >
> > > 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.
> >
> > 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)
>

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

> Thanks,
> Miquèl


Kamal

Re: [PATCH] mtd: set mtd partition panic write flag

[Miquel Raynal]

Hi Kamal,

Kamal Dasu <kamal.dasu@broadcom.com> wrote on Mon, 4 May 2020 11:20:16
-0400:

> On Sat, May 2, 2020 at 2:08 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Kamal,
> >
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote on Thu, 9 Jan 2020
> > 18:28:07 +0100:
> >  
> > > Hi Kamal,
> > >
> > > Kamal Dasu <kamal.dasu@broadcom.com> wrote on Thu, 9 Jan 2020 10:25:59
> > > -0500:
> > >  
> > > > 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.  
> > >
> > > 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)
> >  
> 
> 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