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(+)
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
Hi Kamal,
Richard, something to look into below :)
Kamal Dasu <[email protected]> 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 <[email protected]>
> ---
> 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
----- Ursprüngliche Mail -----
> 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
> 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 <[email protected]> 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
Richard,
On Tue, Nov 5, 2019 at 6:03 PM Richard Weinberger <[email protected]> wrote:
>
> ----- Ursprüngliche Mail -----
> > 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
>
> > 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 <[email protected]> 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
Hello,
Richard Weinberger <[email protected]> wrote on Wed, 6 Nov 2019 00:03:42
+0100 (CET):
> ----- Ursprüngliche Mail -----
> > 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
>
> > 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 <[email protected]> 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
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 <[email protected]> wrote:
>
> Hello,
>
> Richard Weinberger <[email protected]> wrote on Wed, 6 Nov 2019 00:03:42
> +0100 (CET):
>
> > ----- Ursprüngliche Mail -----
> > > 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
> >
> > > 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 <[email protected]> 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
Hi Kamal,
Kamal Dasu <[email protected]> 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
Hi Kamal,
Miquel Raynal <[email protected]> wrote on Thu, 9 Jan 2020
18:28:07 +0100:
> Hi Kamal,
>
> Kamal Dasu <[email protected]> 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
On Sat, May 2, 2020 at 2:08 PM Miquel Raynal <[email protected]> wrote:
>
> Hi Kamal,
>
> Miquel Raynal <[email protected]> wrote on Thu, 9 Jan 2020
> 18:28:07 +0100:
>
> > Hi Kamal,
> >
> > Kamal Dasu <[email protected]> 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
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Mon, 4 May 2020 11:20:16
-0400:
> On Sat, May 2, 2020 at 2:08 PM Miquel Raynal <[email protected]> wrote:
> >
> > Hi Kamal,
> >
> > Miquel Raynal <[email protected]> wrote on Thu, 9 Jan 2020
> > 18:28:07 +0100:
> >
> > > Hi Kamal,
> > >
> > > Kamal Dasu <[email protected]> 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