| Message ID | 20191021193343.41320-1-kdasu.kdev@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: set mtd partition panic write flag | expand |
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
----- 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
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
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
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
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
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
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
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
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); }
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(+)