| Message ID | 20200211163452.25442-2-frieder.schrempf@kontron.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: spinand: Fix reading and writing of bad block markers | expand |
On 11.02.20 17:35, Schrempf Frieder wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > For reading and writing the bad block markers, spinand->oobbuf is > currently used as a buffer for the marker bytes. During the > underlying read and write operations to actually get/set the content > of the OOB area, the content of spinand->oobbuf is reused and changed > by accessing it through spinand->oobbuf and/or spinand->databuf. > > This is a flaw in the original design of the SPI MEM core and at the This should be SPI NAND, of course. ^ > latest from 13c15e07eedf ("mtd: spinand: Handle the case where > PROGRAM LOAD does not reset the cache") on, it results in not having > the bad block marker written at all, as the spinand->oobbuf is > cleared to 0xff after setting the marker bytes to zero. > > To fix it, we now just store the two bytes for the marker on the > stack and let the read/write operations copy it from/to the page > buffer later. > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs") > Cc: stable@vger.kernel.org > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/mtd/nand/spi/core.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index 89f6beefb01c..5d267a67a5f7 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -568,18 +568,18 @@ static int spinand_mtd_write(struct mtd_info *mtd, loff_t to, > static bool spinand_isbad(struct nand_device *nand, const struct nand_pos *pos) > { > struct spinand_device *spinand = nand_to_spinand(nand); > + u8 marker[] = { 0, 0 }; > struct nand_page_io_req req = { > .pos = *pos, > .ooblen = 2, > .ooboffs = 0, > - .oobbuf.in = spinand->oobbuf, > + .oobbuf.in = marker, > .mode = MTD_OPS_RAW, > }; > > - memset(spinand->oobbuf, 0, 2); > spinand_select_target(spinand, pos->target); > spinand_read_page(spinand, &req, false); > - if (spinand->oobbuf[0] != 0xff || spinand->oobbuf[1] != 0xff) > + if (marker[0] != 0xff || marker[1] != 0xff) > return true; > > return false; > @@ -603,11 +603,12 @@ static int spinand_mtd_block_isbad(struct mtd_info *mtd, loff_t offs) > static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos) > { > struct spinand_device *spinand = nand_to_spinand(nand); > + u8 marker[] = { 0, 0 }; > struct nand_page_io_req req = { > .pos = *pos, > .ooboffs = 0, > .ooblen = 2, > - .oobbuf.out = spinand->oobbuf, > + .oobbuf.out = marker, > }; > int ret; > > @@ -622,7 +623,6 @@ static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos) > > spinand_erase_op(spinand, pos); > > - memset(spinand->oobbuf, 0, 2); > return spinand_write_page(spinand, &req); > } > >
On Tue, 11 Feb 2020 16:35:33 +0000 Schrempf Frieder <frieder.schrempf@kontron.de> wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > For reading and writing the bad block markers, spinand->oobbuf is > currently used as a buffer for the marker bytes. During the > underlying read and write operations to actually get/set the content > of the OOB area, the content of spinand->oobbuf is reused and changed > by accessing it through spinand->oobbuf and/or spinand->databuf. > > This is a flaw in the original design of the SPI MEM core and at the > latest from 13c15e07eedf ("mtd: spinand: Handle the case where > PROGRAM LOAD does not reset the cache") on, it results in not having > the bad block marker written at all, as the spinand->oobbuf is > cleared to 0xff after setting the marker bytes to zero. > > To fix it, we now just store the two bytes for the marker on the > stack and let the read/write operations copy it from/to the page > buffer later. > > Fixes: 7529df465248 ("mtd: nand: Add core infrastructure to support SPI NANDs") > Cc: stable@vger.kernel.org > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/mtd/nand/spi/core.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index 89f6beefb01c..5d267a67a5f7 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -568,18 +568,18 @@ static int spinand_mtd_write(struct mtd_info *mtd, loff_t to, > static bool spinand_isbad(struct nand_device *nand, const struct nand_pos *pos) > { > struct spinand_device *spinand = nand_to_spinand(nand); > + u8 marker[] = { 0, 0 }; How about u8 marker[2] = { }; ? > struct nand_page_io_req req = { > .pos = *pos, > .ooblen = 2, .ooblen = sizeof(marker), > .ooboffs = 0, > - .oobbuf.in = spinand->oobbuf, > + .oobbuf.in = marker, > .mode = MTD_OPS_RAW, > }; > > - memset(spinand->oobbuf, 0, 2); > spinand_select_target(spinand, pos->target); > spinand_read_page(spinand, &req, false); > - if (spinand->oobbuf[0] != 0xff || spinand->oobbuf[1] != 0xff) > + if (marker[0] != 0xff || marker[1] != 0xff) > return true; > > return false; > @@ -603,11 +603,12 @@ static int spinand_mtd_block_isbad(struct mtd_info *mtd, loff_t offs) > static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos) > { > struct spinand_device *spinand = nand_to_spinand(nand); > + u8 marker[] = { 0, 0 }; > struct nand_page_io_req req = { > .pos = *pos, > .ooboffs = 0, > .ooblen = 2, > - .oobbuf.out = spinand->oobbuf, > + .oobbuf.out = marker, Ditto. > }; > int ret; > > @@ -622,7 +623,6 @@ static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos) > > spinand_erase_op(spinand, pos); > > - memset(spinand->oobbuf, 0, 2); > return spinand_write_page(spinand, &req); > } > With these minor things addressed Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 89f6beefb01c..5d267a67a5f7 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -568,18 +568,18 @@ static int spinand_mtd_write(struct mtd_info *mtd, loff_t to, static bool spinand_isbad(struct nand_device *nand, const struct nand_pos *pos) { struct spinand_device *spinand = nand_to_spinand(nand); + u8 marker[] = { 0, 0 }; struct nand_page_io_req req = { .pos = *pos, .ooblen = 2, .ooboffs = 0, - .oobbuf.in = spinand->oobbuf, + .oobbuf.in = marker, .mode = MTD_OPS_RAW, }; - memset(spinand->oobbuf, 0, 2); spinand_select_target(spinand, pos->target); spinand_read_page(spinand, &req, false); - if (spinand->oobbuf[0] != 0xff || spinand->oobbuf[1] != 0xff) + if (marker[0] != 0xff || marker[1] != 0xff) return true; return false; @@ -603,11 +603,12 @@ static int spinand_mtd_block_isbad(struct mtd_info *mtd, loff_t offs) static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos) { struct spinand_device *spinand = nand_to_spinand(nand); + u8 marker[] = { 0, 0 }; struct nand_page_io_req req = { .pos = *pos, .ooboffs = 0, .ooblen = 2, - .oobbuf.out = spinand->oobbuf, + .oobbuf.out = marker, }; int ret; @@ -622,7 +623,6 @@ static int spinand_markbad(struct nand_device *nand, const struct nand_pos *pos) spinand_erase_op(spinand, pos); - memset(spinand->oobbuf, 0, 2); return spinand_write_page(spinand, &req); }