From c7ee7c8d4b60fe46d4861b1200bc1c7ab657960a Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Fri, 23 Feb 2024 12:55:43 +0100 Subject: mtd: rawnand: Fix and simplify again the continuous read derivations We need to avoid the first page if we don't read it entirely. We need to avoid the last page if we don't read it entirely. While rather simple, this logic has been failed in the previous fix. This time I wrote about 30 unit tests locally to check each possible condition, hopefully I covered them all. Reported-by: Christophe Kerello Closes: https://lore.kernel.org/linux-mtd/20240221175327.42f7076d@xps-13/T/#m399bacb10db8f58f6b1f0149a1df867ec086bb0a Suggested-by: Christophe Kerello Fixes: 828f6df1bcba ("mtd: rawnand: Clarify conditions to enable continuous reads") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal Tested-by: Christophe Kerello Link: https://lore.kernel.org/linux-mtd/20240223115545.354541-2-miquel.raynal@bootlin.com --- drivers/mtd/nand/raw/nand_base.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) (limited to 'drivers/mtd/nand/raw/nand_base.c') diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 3b3ce2926f5d..bcfd99a1699f 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -3466,30 +3466,36 @@ static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page, u32 readlen, int col) { struct mtd_info *mtd = nand_to_mtd(chip); - unsigned int end_page, end_col; + unsigned int first_page, last_page; chip->cont_read.ongoing = false; if (!chip->controller->supported_op.cont_read) return; - end_page = DIV_ROUND_UP(col + readlen, mtd->writesize); - end_col = (col + readlen) % mtd->writesize; + /* + * Don't bother making any calculations if the length is too small. + * Side effect: avoids possible integer underflows below. + */ + if (readlen < (2 * mtd->writesize)) + return; + /* Derive the page where continuous read should start (the first full page read) */ + first_page = page; if (col) - page++; - - if (end_col && end_page) - end_page--; + first_page++; - if (page + 1 > end_page) - return; - - chip->cont_read.first_page = page; - chip->cont_read.last_page = end_page; - chip->cont_read.ongoing = true; + /* Derive the page where continuous read should stop (the last full page read) */ + last_page = page + ((col + readlen) / mtd->writesize) - 1; - rawnand_cap_cont_reads(chip); + /* Configure and enable continuous read when suitable */ + if (first_page < last_page) { + chip->cont_read.first_page = first_page; + chip->cont_read.last_page = last_page; + chip->cont_read.ongoing = true; + /* May reset the ongoing flag */ + rawnand_cap_cont_reads(chip); + } } static void rawnand_cont_read_skip_first_page(struct nand_chip *chip, unsigned int page) -- cgit v1.2.3 From df9803bf5a91e3599f12b53c94722f2c4e144a86 Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Fri, 23 Feb 2024 12:55:44 +0100 Subject: mtd: rawnand: Add a helper for calculating a page index For LUN crossing boundaries, it is handy to know what is the index of the last page in a LUN. This helper will soon be reused. At the same time I rename page_per_lun to ppl in the calling function to clarify the lines. Cc: stable@vger.kernel.org # v6.7 Signed-off-by: Miquel Raynal Link: https://lore.kernel.org/linux-mtd/20240223115545.354541-3-miquel.raynal@bootlin.com --- drivers/mtd/nand/raw/nand_base.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'drivers/mtd/nand/raw/nand_base.c') diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index bcfd99a1699f..d6a27e08b112 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -1211,19 +1211,25 @@ static int nand_lp_exec_read_page_op(struct nand_chip *chip, unsigned int page, return nand_exec_op(chip, &op); } +static unsigned int rawnand_last_page_of_lun(unsigned int pages_per_lun, unsigned int lun) +{ + /* lun is expected to be very small */ + return (lun * pages_per_lun) + pages_per_lun - 1; +} + static void rawnand_cap_cont_reads(struct nand_chip *chip) { struct nand_memory_organization *memorg; - unsigned int pages_per_lun, first_lun, last_lun; + unsigned int ppl, first_lun, last_lun; memorg = nanddev_get_memorg(&chip->base); - pages_per_lun = memorg->pages_per_eraseblock * memorg->eraseblocks_per_lun; - first_lun = chip->cont_read.first_page / pages_per_lun; - last_lun = chip->cont_read.last_page / pages_per_lun; + ppl = memorg->pages_per_eraseblock * memorg->eraseblocks_per_lun; + first_lun = chip->cont_read.first_page / ppl; + last_lun = chip->cont_read.last_page / ppl; /* Prevent sequential cache reads across LUN boundaries */ if (first_lun != last_lun) - chip->cont_read.pause_page = first_lun * pages_per_lun + pages_per_lun - 1; + chip->cont_read.pause_page = rawnand_last_page_of_lun(ppl, first_lun); else chip->cont_read.pause_page = chip->cont_read.last_page; } -- cgit v1.2.3 From 6fb075fca63c3486612986eeff84ed4179644038 Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Fri, 23 Feb 2024 12:55:45 +0100 Subject: mtd: rawnand: Ensure all continuous terms are always in sync While crossing a LUN boundary, it is probably safer (and clearer) to keep all members of the continuous read structure aligned, including the pause page (which is the last page of the lun or the last page of the continuous read). Once these members properly in sync, we can use the rawnand_cap_cont_reads() helper everywhere to "prepare" the next continuous read if there is one. Fixes: bbcd80f53a5e ("mtd: rawnand: Prevent crossing LUN boundaries during sequential reads") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal Link: https://lore.kernel.org/linux-mtd/20240223115545.354541-4-miquel.raynal@bootlin.com --- drivers/mtd/nand/raw/nand_base.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'drivers/mtd/nand/raw/nand_base.c') diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index d6a27e08b112..4d5a663e4e05 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -1232,6 +1232,15 @@ static void rawnand_cap_cont_reads(struct nand_chip *chip) chip->cont_read.pause_page = rawnand_last_page_of_lun(ppl, first_lun); else chip->cont_read.pause_page = chip->cont_read.last_page; + + if (chip->cont_read.first_page == chip->cont_read.pause_page) { + chip->cont_read.first_page++; + chip->cont_read.pause_page = min(chip->cont_read.last_page, + rawnand_last_page_of_lun(ppl, first_lun + 1)); + } + + if (chip->cont_read.first_page >= chip->cont_read.last_page) + chip->cont_read.ongoing = false; } static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int page, @@ -1298,12 +1307,11 @@ static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int p if (!chip->cont_read.ongoing) return 0; - if (page == chip->cont_read.pause_page && - page != chip->cont_read.last_page) { - chip->cont_read.first_page = chip->cont_read.pause_page + 1; - rawnand_cap_cont_reads(chip); - } else if (page == chip->cont_read.last_page) { + if (page == chip->cont_read.last_page) { chip->cont_read.ongoing = false; + } else if (page == chip->cont_read.pause_page) { + chip->cont_read.first_page++; + rawnand_cap_cont_reads(chip); } return 0; @@ -3510,10 +3518,7 @@ static void rawnand_cont_read_skip_first_page(struct nand_chip *chip, unsigned i return; chip->cont_read.first_page++; - if (chip->cont_read.first_page == chip->cont_read.pause_page) - chip->cont_read.first_page++; - if (chip->cont_read.first_page >= chip->cont_read.last_page) - chip->cont_read.ongoing = false; + rawnand_cap_cont_reads(chip); } /** -- cgit v1.2.3 From 78ffbefba8d7822b232585570b293de5bc397da6 Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Thu, 7 Mar 2024 12:53:14 +0100 Subject: mtd: rawnand: Constrain even more when continuous reads are enabled As a matter of fact, continuous reads require additional handling at the operation level in order for them to work properly. The core helpers do have this additional logic now, but any time a controller implements its own page helper, this extra logic is "lost". This means we need another level of per-controller driver checks to ensure they can leverage continuous reads. This is for now unsupported, so in order to ensure continuous reads are enabled only when fully using the core page helpers, we need to add more initial checks. Also, as performance is not relevant during raw accesses, we also prevent these from enabling the feature. This should solve the issue seen with controllers such as the STM32 FMC2 when in sequencer mode. In this case, the continuous read feature would be enabled but not leveraged, and most importantly not disabled, leading to further operations to fail. Reported-by: Christophe Kerello Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal Tested-by: Christophe Kerello Link: https://lore.kernel.org/linux-mtd/20240307115315.1942678-1-miquel.raynal@bootlin.com --- drivers/mtd/nand/raw/nand_base.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'drivers/mtd/nand/raw/nand_base.c') diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 4d5a663e4e05..2479fa98f991 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -3594,7 +3594,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from, oob = ops->oobbuf; oob_required = oob ? 1 : 0; - rawnand_enable_cont_reads(chip, page, readlen, col); + if (likely(ops->mode != MTD_OPS_RAW)) + rawnand_enable_cont_reads(chip, page, readlen, col); while (1) { struct mtd_ecc_stats ecc_stats = mtd->ecc_stats; @@ -5212,6 +5213,15 @@ static void rawnand_late_check_supported_ops(struct nand_chip *chip) if (!nand_has_exec_op(chip)) return; + /* + * For now, continuous reads can only be used with the core page helpers. + * This can be extended later. + */ + if (!(chip->ecc.read_page == nand_read_page_hwecc || + chip->ecc.read_page == nand_read_page_syndrome || + chip->ecc.read_page == nand_read_page_swecc)) + return; + rawnand_check_cont_read_support(chip); } -- cgit v1.2.3 From 4120aa0e3961f68f1f8cfe6b4c3c809ffea31fdc Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Thu, 7 Mar 2024 12:53:15 +0100 Subject: mtd: rawnand: Ensure continuous reads are well disabled The cont_read.ongoing flag should only be enabled at the beginning of a read operation, and also disabled at its end, so we never end up triggering nasty side effects outside of this scope. The mtd core being highly serialized, we should not be bothered by parallel accesses anyway. In case we reach the end of a read operation and the boolean was not properly disabled, it's a bug, but it's totally manageable. So warn, and then fix the boolean state. Signed-off-by: Miquel Raynal Link: https://lore.kernel.org/linux-mtd/20240307115315.1942678-2-miquel.raynal@bootlin.com --- drivers/mtd/nand/raw/nand_base.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/mtd/nand/raw/nand_base.c') diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 2479fa98f991..d7dbbd469b89 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -3728,6 +3728,9 @@ read_retry: } nand_deselect_target(chip); + if (WARN_ON_ONCE(chip->cont_read.ongoing)) + chip->cont_read.ongoing = false; + ops->retlen = ops->len - (size_t) readlen; if (oob) ops->oobretlen = ops->ooblen - oobreadlen; -- cgit v1.2.3