| Message ID | 20240516131320.579822-3-miquel.raynal@bootlin.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: rawnand: NAND early identification fixes | expand |
On Thu, May 16, 2024 at 03:13:20PM +0200, Miquel Raynal wrote: > Early during NAND identification, mtd_info fields have not yet been > initialized (namely, writesize and oobsize) and thus cannot be used for > sanity checks yet. Of course if there is a misuse of > nand_change_read_column_op() so early we won't be warned, but there is > anyway no actual check to perform at this stage as we do not yet know > the NAND geometry. > > So, if the fields are empty, especially mtd->writesize which is *always* > set quite rapidly after identification, let's skip the sanity checks. > > nand_change_read_column_op() is subject to be used early for ONFI/JEDEC > identification in the very unlikely case of: > - bitflips appearing in the parameter page, > - the controller driver not supporting simple DATA_IN cycles. > > As nand_change_read_column_op() uses nand_fill_column_cycles() the logic > explaind above also applies in this secondary helper. > > Fixes: c27842e7e11f ("mtd: rawnand: onfi: Adapt the parameter page read to constraint controllers") > Fixes: daca31765e8b ("mtd: rawnand: jedec: Adapt the parameter page read to constraint controllers") > Cc: stable@vger.kernel.org > Reported-by: Alexander Dahl <ada@thorsis.com> > Closes: https://lore.kernel.org/linux-mtd/20240306-shaky-bunion-d28b65ea97d7@thorsis.com/ > Reported-by: Steven Seeger <steven.seeger@flightsystems.net> > Closes: https://lore.kernel.org/linux-mtd/DM6PR05MB4506554457CF95191A670BDEF7062@DM6PR05MB4506.namprd05.prod.outlook.com/ > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> With the attached debug patch applied I can confirm that I can now read all three ONFI parameter pages successfully using nand_change_read_column_op(), so: Tested-by: Sascha Hauer <s.hauer@pengutronix.de> Sascha -----------------------------------8<-------------------------------------- diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c index 861975e44b552..ca6b4bf426750 100644 --- a/drivers/mtd/nand/raw/nand_onfi.c +++ b/drivers/mtd/nand/raw/nand_onfi.c @@ -180,6 +180,9 @@ int nand_onfi_detect(struct nand_chip *chip) ret = nand_change_read_column_op(chip, sizeof(*pbuf) * i, &pbuf[i], sizeof(*pbuf), true); + + print_hex_dump(KERN_INFO, "onfi: ", DUMP_PREFIX_OFFSET, 16, 1, &pbuf[i], sizeof(*pbuf), true); + if (ret) { ret = 0; goto free_onfi_param_page; @@ -188,7 +191,6 @@ int nand_onfi_detect(struct nand_chip *chip) crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 254); if (crc == le16_to_cpu(pbuf[i].crc)) { p = &pbuf[i]; - break; } }
Hi Sascha, s.hauer@pengutronix.de wrote on Thu, 16 May 2024 15:51:49 +0200: > On Thu, May 16, 2024 at 03:13:20PM +0200, Miquel Raynal wrote: > > Early during NAND identification, mtd_info fields have not yet been > > initialized (namely, writesize and oobsize) and thus cannot be used for > > sanity checks yet. Of course if there is a misuse of > > nand_change_read_column_op() so early we won't be warned, but there is > > anyway no actual check to perform at this stage as we do not yet know > > the NAND geometry. > > > > So, if the fields are empty, especially mtd->writesize which is *always* > > set quite rapidly after identification, let's skip the sanity checks. > > > > nand_change_read_column_op() is subject to be used early for ONFI/JEDEC > > identification in the very unlikely case of: > > - bitflips appearing in the parameter page, > > - the controller driver not supporting simple DATA_IN cycles. > > > > As nand_change_read_column_op() uses nand_fill_column_cycles() the logic > > explaind above also applies in this secondary helper. > > > > Fixes: c27842e7e11f ("mtd: rawnand: onfi: Adapt the parameter page read to constraint controllers") > > Fixes: daca31765e8b ("mtd: rawnand: jedec: Adapt the parameter page read to constraint controllers") > > Cc: stable@vger.kernel.org > > Reported-by: Alexander Dahl <ada@thorsis.com> > > Closes: https://lore.kernel.org/linux-mtd/20240306-shaky-bunion-d28b65ea97d7@thorsis.com/ > > Reported-by: Steven Seeger <steven.seeger@flightsystems.net> > > Closes: https://lore.kernel.org/linux-mtd/DM6PR05MB4506554457CF95191A670BDEF7062@DM6PR05MB4506.namprd05.prod.outlook.com/ > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > With the attached debug patch applied I can confirm that I can now read > all three ONFI parameter pages successfully using > nand_change_read_column_op(), so: > > Tested-by: Sascha Hauer <s.hauer@pengutronix.de> Excellent! Thanks, Miquèl
On Thu, 2024-05-16 at 13:13:20 UTC, Miquel Raynal wrote: > Early during NAND identification, mtd_info fields have not yet been > initialized (namely, writesize and oobsize) and thus cannot be used for > sanity checks yet. Of course if there is a misuse of > nand_change_read_column_op() so early we won't be warned, but there is > anyway no actual check to perform at this stage as we do not yet know > the NAND geometry. > > So, if the fields are empty, especially mtd->writesize which is *always* > set quite rapidly after identification, let's skip the sanity checks. > > nand_change_read_column_op() is subject to be used early for ONFI/JEDEC > identification in the very unlikely case of: > - bitflips appearing in the parameter page, > - the controller driver not supporting simple DATA_IN cycles. > > As nand_change_read_column_op() uses nand_fill_column_cycles() the logic > explaind above also applies in this secondary helper. > > Fixes: c27842e7e11f ("mtd: rawnand: onfi: Adapt the parameter page read to constraint controllers") > Fixes: daca31765e8b ("mtd: rawnand: jedec: Adapt the parameter page read to constraint controllers") > Cc: stable@vger.kernel.org > Reported-by: Alexander Dahl <ada@thorsis.com> > Closes: https://lore.kernel.org/linux-mtd/20240306-shaky-bunion-d28b65ea97d7@thorsis.com/ > Reported-by: Steven Seeger <steven.seeger@flightsystems.net> > Closes: https://lore.kernel.org/linux-mtd/DM6PR05MB4506554457CF95191A670BDEF7062@DM6PR05MB4506.namprd05.prod.outlook.com/ > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Tested-by: Sascha Hauer <s.hauer@pengutronix.de> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes. Miquel
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 248e654ecefd..53e16d39af4b 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -1093,28 +1093,32 @@ static int nand_fill_column_cycles(struct nand_chip *chip, u8 *addrs, unsigned int offset_in_page) { struct mtd_info *mtd = nand_to_mtd(chip); + bool ident_stage = !mtd->writesize; - /* Make sure the offset is less than the actual page size. */ - if (offset_in_page > mtd->writesize + mtd->oobsize) - return -EINVAL; - - /* - * On small page NANDs, there's a dedicated command to access the OOB - * area, and the column address is relative to the start of the OOB - * area, not the start of the page. Asjust the address accordingly. - */ - if (mtd->writesize <= 512 && offset_in_page >= mtd->writesize) - offset_in_page -= mtd->writesize; - - /* - * The offset in page is expressed in bytes, if the NAND bus is 16-bit - * wide, then it must be divided by 2. - */ - if (chip->options & NAND_BUSWIDTH_16) { - if (WARN_ON(offset_in_page % 2)) + /* Bypass all checks during NAND identification */ + if (likely(!ident_stage)) { + /* Make sure the offset is less than the actual page size. */ + if (offset_in_page > mtd->writesize + mtd->oobsize) return -EINVAL; - offset_in_page /= 2; + /* + * On small page NANDs, there's a dedicated command to access the OOB + * area, and the column address is relative to the start of the OOB + * area, not the start of the page. Asjust the address accordingly. + */ + if (mtd->writesize <= 512 && offset_in_page >= mtd->writesize) + offset_in_page -= mtd->writesize; + + /* + * The offset in page is expressed in bytes, if the NAND bus is 16-bit + * wide, then it must be divided by 2. + */ + if (chip->options & NAND_BUSWIDTH_16) { + if (WARN_ON(offset_in_page % 2)) + return -EINVAL; + + offset_in_page /= 2; + } } addrs[0] = offset_in_page; @@ -1123,7 +1127,7 @@ static int nand_fill_column_cycles(struct nand_chip *chip, u8 *addrs, * Small page NANDs use 1 cycle for the columns, while large page NANDs * need 2 */ - if (mtd->writesize <= 512) + if (!ident_stage && mtd->writesize <= 512) return 1; addrs[1] = offset_in_page >> 8; @@ -1436,16 +1440,19 @@ int nand_change_read_column_op(struct nand_chip *chip, unsigned int len, bool force_8bit) { struct mtd_info *mtd = nand_to_mtd(chip); + bool ident_stage = !mtd->writesize; if (len && !buf) return -EINVAL; - if (offset_in_page + len > mtd->writesize + mtd->oobsize) - return -EINVAL; + if (!ident_stage) { + if (offset_in_page + len > mtd->writesize + mtd->oobsize) + return -EINVAL; - /* Small page NANDs do not support column change. */ - if (mtd->writesize <= 512) - return -ENOTSUPP; + /* Small page NANDs do not support column change. */ + if (mtd->writesize <= 512) + return -ENOTSUPP; + } if (nand_has_exec_op(chip)) { const struct nand_interface_config *conf =
Early during NAND identification, mtd_info fields have not yet been initialized (namely, writesize and oobsize) and thus cannot be used for sanity checks yet. Of course if there is a misuse of nand_change_read_column_op() so early we won't be warned, but there is anyway no actual check to perform at this stage as we do not yet know the NAND geometry. So, if the fields are empty, especially mtd->writesize which is *always* set quite rapidly after identification, let's skip the sanity checks. nand_change_read_column_op() is subject to be used early for ONFI/JEDEC identification in the very unlikely case of: - bitflips appearing in the parameter page, - the controller driver not supporting simple DATA_IN cycles. As nand_change_read_column_op() uses nand_fill_column_cycles() the logic explaind above also applies in this secondary helper. Fixes: c27842e7e11f ("mtd: rawnand: onfi: Adapt the parameter page read to constraint controllers") Fixes: daca31765e8b ("mtd: rawnand: jedec: Adapt the parameter page read to constraint controllers") Cc: stable@vger.kernel.org Reported-by: Alexander Dahl <ada@thorsis.com> Closes: https://lore.kernel.org/linux-mtd/20240306-shaky-bunion-d28b65ea97d7@thorsis.com/ Reported-by: Steven Seeger <steven.seeger@flightsystems.net> Closes: https://lore.kernel.org/linux-mtd/DM6PR05MB4506554457CF95191A670BDEF7062@DM6PR05MB4506.namprd05.prod.outlook.com/ Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- Changes in v2: * Dropped the double (( )) * Fixed nand_fill_column_cycles() as well. --- drivers/mtd/nand/raw/nand_base.c | 57 ++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 25 deletions(-)