* [PATCH] Fix writing mtdoops to nand flash. @ 2017-10-30 4:23 motobud 2017-10-30 8:23 ` Boris Brezillon 2017-10-31 3:32 ` [PATCH v2] mtd: nand: " motobud 0 siblings, 2 replies; 7+ messages in thread From: motobud @ 2017-10-30 4:23 UTC (permalink / raw) To: boris.brezillon Cc: Brent Taylor, Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel From: Brent Taylor <motobud@gmail.com> When mtdoops calls mtd_panic_write, it eventually calls panic_nand_write in nand_base.c. In order to properly wait for the nand chip to be ready in panic_nand_wait, the chip must first be selected. When using the atmel nand flash controller, a panic would occur due to a NULL pointer exception. Signed-off-by: Brent Taylor <motobud@gmail.com> --- drivers/mtd/nand/nand_base.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 12edaae17d81..0a8058a66d93 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2802,9 +2802,14 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len, struct mtd_oob_ops ops; int ret; + int chipnr = (int)(to >> chip->chip_shift); + chip->select_chip(mtd, chipnr); + /* Wait for the device to get ready */ panic_nand_wait(mtd, chip, 400); + chip->select_chip(mtd, -1); + /* Grab the device */ panic_nand_get_device(chip, mtd, FL_WRITING); -- 2.14.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix writing mtdoops to nand flash. 2017-10-30 4:23 [PATCH] Fix writing mtdoops to nand flash motobud @ 2017-10-30 8:23 ` Boris Brezillon 2017-10-30 12:46 ` Brent Taylor 2017-10-31 3:32 ` [PATCH v2] mtd: nand: " motobud 1 sibling, 1 reply; 7+ messages in thread From: Boris Brezillon @ 2017-10-30 8:23 UTC (permalink / raw) To: motobud Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel Hi Brent, Subject should be prefixed by "mtd: nand: ", so "mtd: nand: Fix writing mtdoops to nand flash" On Sun, 29 Oct 2017 23:23:43 -0500 motobud@gmail.com wrote: > From: Brent Taylor <motobud@gmail.com> > > When mtdoops calls mtd_panic_write, it eventually calls > panic_nand_write in nand_base.c. In order to properly > wait for the nand chip to be ready in panic_nand_wait, > the chip must first be selected. > > When using the atmel nand flash controller, a panic > would occur due to a NULL pointer exception. > > Signed-off-by: Brent Taylor <motobud@gmail.com> > --- > drivers/mtd/nand/nand_base.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 12edaae17d81..0a8058a66d93 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2802,9 +2802,14 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len, > struct mtd_oob_ops ops; > int ret; > > + int chipnr = (int)(to >> chip->chip_shift); > + chip->select_chip(mtd, chipnr); > + > /* Wait for the device to get ready */ > panic_nand_wait(mtd, chip, 400); > > + chip->select_chip(mtd, -1); > + Duh! Looks like a piece of code that is never tested. Did you face the problem or did you find out by inspecting the code? Anyway, I fear the atmel driver is not the only one to rely on the chip to be selected when ->dev_ready() is called so this should definitely be fixed. Also, we should probably move the ->select_chip() and panic_nand_wait() calls after panic_nand_get_device(), and I don't think we need to unselect the chip (it will be taken care of by nand_do_write_ops()). > /* Grab the device */ > panic_nand_get_device(chip, mtd, FL_WRITING); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix writing mtdoops to nand flash. 2017-10-30 8:23 ` Boris Brezillon @ 2017-10-30 12:46 ` Brent Taylor 2017-10-30 13:15 ` Boris Brezillon 0 siblings, 1 reply; 7+ messages in thread From: Brent Taylor @ 2017-10-30 12:46 UTC (permalink / raw) To: Boris Brezillon Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel Hi Bors, thanks for your quick reply. On Mon, Oct 30, 2017 at 3:23 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Brent, > > Subject should be prefixed by "mtd: nand: ", so > > "mtd: nand: Fix writing mtdoops to nand flash" Oops, yes, will fix. > > On Sun, 29 Oct 2017 23:23:43 -0500 > motobud@gmail.com wrote: > >> From: Brent Taylor <motobud@gmail.com> >> >> When mtdoops calls mtd_panic_write, it eventually calls >> panic_nand_write in nand_base.c. In order to properly >> wait for the nand chip to be ready in panic_nand_wait, >> the chip must first be selected. >> >> When using the atmel nand flash controller, a panic >> would occur due to a NULL pointer exception. >> >> Signed-off-by: Brent Taylor <motobud@gmail.com> >> --- >> drivers/mtd/nand/nand_base.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index 12edaae17d81..0a8058a66d93 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -2802,9 +2802,14 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len, >> struct mtd_oob_ops ops; >> int ret; >> >> + int chipnr = (int)(to >> chip->chip_shift); >> + chip->select_chip(mtd, chipnr); >> + >> /* Wait for the device to get ready */ >> panic_nand_wait(mtd, chip, 400); >> >> + chip->select_chip(mtd, -1); >> + > > Duh! Looks like a piece of code that is never tested. Did you face the > problem or did you find out by inspecting the code? I was playing with another driver on an atmel development board (at91sam9m10g45ek) and caused a panic with mtdoops enabled. While writing the mtdoops to nand, another panic occurred which caused a storm of panics to generated. > > Anyway, I fear the atmel driver is not the only one to rely on the chip > to be selected when ->dev_ready() is called so this should definitely > be fixed. Also, we should probably move the ->select_chip() and > panic_nand_wait() calls after panic_nand_get_device(), and I don't > think we need to unselect the chip (it will be taken care of by > nand_do_write_ops()). > >> /* Grab the device */ >> panic_nand_get_device(chip, mtd, FL_WRITING); >> > After looking at this closer, a panic could happen at any point correct? If that's the case, the kernel could have been in the middle of a nand read/write operation (which may or may not be on the same chip). Would the chip the mtdoops is being written to need to be reset? I haven't drilled down into the nand_reset function yet, but can that be called in a "panic" state? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix writing mtdoops to nand flash. 2017-10-30 12:46 ` Brent Taylor @ 2017-10-30 13:15 ` Boris Brezillon 0 siblings, 0 replies; 7+ messages in thread From: Boris Brezillon @ 2017-10-30 13:15 UTC (permalink / raw) To: Brent Taylor Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel On Mon, 30 Oct 2017 07:46:18 -0500 Brent Taylor <motobud@gmail.com> wrote: > Hi Bors, thanks for your quick reply. > > On Mon, Oct 30, 2017 at 3:23 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Hi Brent, > > > > Subject should be prefixed by "mtd: nand: ", so > > > > "mtd: nand: Fix writing mtdoops to nand flash" > > Oops, yes, will fix. > > > > > On Sun, 29 Oct 2017 23:23:43 -0500 > > motobud@gmail.com wrote: > > > >> From: Brent Taylor <motobud@gmail.com> > >> > >> When mtdoops calls mtd_panic_write, it eventually calls > >> panic_nand_write in nand_base.c. In order to properly > >> wait for the nand chip to be ready in panic_nand_wait, > >> the chip must first be selected. > >> > >> When using the atmel nand flash controller, a panic > >> would occur due to a NULL pointer exception. > >> > >> Signed-off-by: Brent Taylor <motobud@gmail.com> > >> --- > >> drivers/mtd/nand/nand_base.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > >> index 12edaae17d81..0a8058a66d93 100644 > >> --- a/drivers/mtd/nand/nand_base.c > >> +++ b/drivers/mtd/nand/nand_base.c > >> @@ -2802,9 +2802,14 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len, > >> struct mtd_oob_ops ops; > >> int ret; > >> > >> + int chipnr = (int)(to >> chip->chip_shift); > >> + chip->select_chip(mtd, chipnr); > >> + > >> /* Wait for the device to get ready */ > >> panic_nand_wait(mtd, chip, 400); > >> > >> + chip->select_chip(mtd, -1); > >> + > > > > Duh! Looks like a piece of code that is never tested. Did you face the > > problem or did you find out by inspecting the code? > > I was playing with another driver on an atmel development board > (at91sam9m10g45ek) and caused a panic with mtdoops enabled. While > writing the mtdoops to nand, another panic occurred which caused a > storm of panics to generated. > > > > > Anyway, I fear the atmel driver is not the only one to rely on the chip > > to be selected when ->dev_ready() is called so this should definitely > > be fixed. Also, we should probably move the ->select_chip() and > > panic_nand_wait() calls after panic_nand_get_device(), and I don't > > think we need to unselect the chip (it will be taken care of by > > nand_do_write_ops()). > > > >> /* Grab the device */ > >> panic_nand_get_device(chip, mtd, FL_WRITING); > >> > > > > After looking at this closer, a panic could happen at any point correct? Yes. > If > that's the case, the kernel could have been in the middle of a nand read/write > operation (which may or may not be on the same chip). Would the chip the > mtdoops is being written to need to be reset? I'd prefer not. Actually, the code calls panic_nand_wait() to let the NAND finish the operation it's currently doing, and most of the time it will relax the busy pin within 400ms (the timeout passed to panic_nand_wait()). If we do a RESET in the middle of a write operation we'll just corrupt the data that is being written, and I'm not sure we want that to happen. > I haven't drilled down into the > nand_reset function yet, but can that be called in a "panic" state? Maybe, but I'm not sure we want to do that (see above). Anyway, the write-on-panic path is fragile, and I'm not sure all controllers will play well in this situation: just because the NAND is ready does not mean the controller is, and there's nothing ensuring that in the NAND framework. Regards, Boris ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] mtd: nand: Fix writing mtdoops to nand flash. 2017-10-30 4:23 [PATCH] Fix writing mtdoops to nand flash motobud 2017-10-30 8:23 ` Boris Brezillon @ 2017-10-31 3:32 ` motobud 2017-10-31 10:18 ` Boris Brezillon 2017-11-02 10:48 ` Boris Brezillon 1 sibling, 2 replies; 7+ messages in thread From: motobud @ 2017-10-31 3:32 UTC (permalink / raw) To: boris.brezillon Cc: Brent Taylor, Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel From: Brent Taylor <motobud@gmail.com> When mtdoops calls mtd_panic_write, it eventually calls panic_nand_write in nand_base.c. In order to properly wait for the nand chip to be ready in panic_nand_wait, the chip must first be selected. When using the atmel nand flash controller, a panic would occur due to a NULL pointer exception. Signed-off-by: Brent Taylor <motobud@gmail.com> --- Changes in v2: - drop deselecting the chip - move select_chip and panic_nand_wait after panic_nand_get_device drivers/mtd/nand/nand_base.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 12edaae17d81..2093667e2bcb 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2802,12 +2802,16 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len, struct mtd_oob_ops ops; int ret; - /* Wait for the device to get ready */ - panic_nand_wait(mtd, chip, 400); + int chipnr = (int)(to >> chip->chip_shift); /* Grab the device */ panic_nand_get_device(chip, mtd, FL_WRITING); + chip->select_chip(mtd, chipnr); + + /* Wait for the device to get ready */ + panic_nand_wait(mtd, chip, 400); + memset(&ops, 0, sizeof(ops)); ops.len = len; ops.datbuf = (uint8_t *)buf; -- 2.14.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mtd: nand: Fix writing mtdoops to nand flash. 2017-10-31 3:32 ` [PATCH v2] mtd: nand: " motobud @ 2017-10-31 10:18 ` Boris Brezillon 2017-11-02 10:48 ` Boris Brezillon 1 sibling, 0 replies; 7+ messages in thread From: Boris Brezillon @ 2017-10-31 10:18 UTC (permalink / raw) To: motobud Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel On Mon, 30 Oct 2017 22:32:45 -0500 motobud@gmail.com wrote: > From: Brent Taylor <motobud@gmail.com> > > When mtdoops calls mtd_panic_write, it eventually calls > panic_nand_write in nand_base.c. In order to properly > wait for the nand chip to be ready in panic_nand_wait, > the chip must first be selected. > > When using the atmel nand flash controller, a panic > would occur due to a NULL pointer exception. > > Signed-off-by: Brent Taylor <motobud@gmail.com> I'll add Fixes: 2af7c6539931 ("mtd: Add panic_write for NAND flashes") Cc: <stable@vger.kernel.org> when applying the patch. Thanks, Boris > --- > Changes in v2: > - drop deselecting the chip > - move select_chip and panic_nand_wait after panic_nand_get_device > > drivers/mtd/nand/nand_base.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 12edaae17d81..2093667e2bcb 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2802,12 +2802,16 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len, > struct mtd_oob_ops ops; > int ret; > > - /* Wait for the device to get ready */ > - panic_nand_wait(mtd, chip, 400); > + int chipnr = (int)(to >> chip->chip_shift); > > /* Grab the device */ > panic_nand_get_device(chip, mtd, FL_WRITING); > > + chip->select_chip(mtd, chipnr); > + > + /* Wait for the device to get ready */ > + panic_nand_wait(mtd, chip, 400); > + > memset(&ops, 0, sizeof(ops)); > ops.len = len; > ops.datbuf = (uint8_t *)buf; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mtd: nand: Fix writing mtdoops to nand flash. 2017-10-31 3:32 ` [PATCH v2] mtd: nand: " motobud 2017-10-31 10:18 ` Boris Brezillon @ 2017-11-02 10:48 ` Boris Brezillon 1 sibling, 0 replies; 7+ messages in thread From: Boris Brezillon @ 2017-11-02 10:48 UTC (permalink / raw) To: motobud Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel On Mon, 30 Oct 2017 22:32:45 -0500 motobud@gmail.com wrote: > From: Brent Taylor <motobud@gmail.com> > > When mtdoops calls mtd_panic_write, it eventually calls > panic_nand_write in nand_base.c. In order to properly > wait for the nand chip to be ready in panic_nand_wait, > the chip must first be selected. > > When using the atmel nand flash controller, a panic > would occur due to a NULL pointer exception. > Applied. Thanks, Boris > Signed-off-by: Brent Taylor <motobud@gmail.com> > --- > Changes in v2: > - drop deselecting the chip > - move select_chip and panic_nand_wait after panic_nand_get_device > > drivers/mtd/nand/nand_base.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 12edaae17d81..2093667e2bcb 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2802,12 +2802,16 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len, > struct mtd_oob_ops ops; > int ret; > > - /* Wait for the device to get ready */ > - panic_nand_wait(mtd, chip, 400); > + int chipnr = (int)(to >> chip->chip_shift); > > /* Grab the device */ > panic_nand_get_device(chip, mtd, FL_WRITING); > > + chip->select_chip(mtd, chipnr); > + > + /* Wait for the device to get ready */ > + panic_nand_wait(mtd, chip, 400); > + > memset(&ops, 0, sizeof(ops)); > ops.len = len; > ops.datbuf = (uint8_t *)buf; ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-02 10:48 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-30 4:23 [PATCH] Fix writing mtdoops to nand flash motobud 2017-10-30 8:23 ` Boris Brezillon 2017-10-30 12:46 ` Brent Taylor 2017-10-30 13:15 ` Boris Brezillon 2017-10-31 3:32 ` [PATCH v2] mtd: nand: " motobud 2017-10-31 10:18 ` Boris Brezillon 2017-11-02 10:48 ` Boris Brezillon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.