| Message ID | 20261001124733.43559-1-a929244872@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] mtd: nandsim: address cycle mismatch for 256 MiB / 4096-byte page devices -- commit d11867536ecbb0d0243c052069d60f5f1a2c4afe Author: wang wei <a929244872@163.com> drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize The commit fixes a stale NAND_ROW_ADDR_3 flag by re-evaluating it after changing the size of the simulated device when using the overridesize parameter. It updates the flag based on the core logic: chip_shift - page_shift > 16. > diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c > index fe968037f75a2..ac82dd7e18a93 100644 > --- a/drivers/mtd/nand/raw/nandsim.c > +++ b/drivers/mtd/nand/raw/nandsim.c > @@ -2359,6 +2359,11 @@ static int __init ns_init_module(void) > targetsize = nanddev_target_size(&chip->base); > chip->chip_shift = ffs(nsmtd->erasesize) + overridesize - 1; > chip->pagemask = (targetsize >> chip->page_shift) - 1; > + > + if (chip->chip_shift - chip->page_shift > 16) > + chip->options |= NAND_ROW_ADDR_3; > + else > + chip->options &= ~NAND_ROW_ADDR_3; [Severity: Medium] Does this logic conflict with the address byte expectation in ns_init()? For a 256 MiB device with 4096-byte pages, chip_shift - page_shift evaluates to 28 - 12 = 16. This causes the code here to clear NAND_ROW_ADDR_3, so the core emits 4 address bytes. However, ns_init() has hardcoded logic expecting 5 address bytes for devices larger than 128 MiB: drivers/mtd/nand/raw/nandsim.c:ns_init() { ... if (ns->geom.totsz <= (128 << 20)) { ns->geom.pgaddrbytes = 4; ns->geom.secaddrbytes = 2; } else { ns->geom.pgaddrbytes = 5; ns->geom.secaddrbytes = 3; } ... } Will this mismatch stall the nandsim state machine and fail operations due to an address cycle mismatch? Prior to this change, shrinking a 512 MiB device to 256 MiB left the stale flag set, which serendipitously matched nandsim's expectation of 5 bytes. > } > > ret = ns_setup_wear_reporting(nsmtd);
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: >- [Medium] mtd: nandsim: address cycle mismatch for 256 MiB / 4096-byte page devices >-- > >commit d11867536ecbb0d0243c052069d60f5f1a2c4afe >Author: wang wei <a929244872@163.com> > >drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize > >The commit fixes a stale NAND_ROW_ADDR_3 flag by re-evaluating it after >changing the size of the simulated device when using the overridesize >parameter. It updates the flag based on the core logic: chip_shift - >page_shift > 16. > >> diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c >> index fe968037f75a2..ac82dd7e18a93 100644 >> --- a/drivers/mtd/nand/raw/nandsim.c >> +++ b/drivers/mtd/nand/raw/nandsim.c >> @@ -2359,6 +2359,11 @@ static int __init ns_init_module(void) >> targetsize = nanddev_target_size(&chip->base); >> chip->chip_shift = ffs(nsmtd->erasesize) + overridesize - 1; >> chip->pagemask = (targetsize >> chip->page_shift) - 1; >> + >> + if (chip->chip_shift - chip->page_shift > 16) >> + chip->options |= NAND_ROW_ADDR_3; >> + else >> + chip->options &= ~NAND_ROW_ADDR_3; > >[Severity: Medium] >Does this logic conflict with the address byte expectation in ns_init()? > >For a 256 MiB device with 4096-byte pages, chip_shift - page_shift evaluates >to 28 - 12 = 16. This causes the code here to clear NAND_ROW_ADDR_3, so the >core emits 4 address bytes. > >However, ns_init() has hardcoded logic expecting 5 address bytes for devices >larger than 128 MiB: > >drivers/mtd/nand/raw/nandsim.c:ns_init() { > ... > if (ns->geom.totsz <= (128 << 20)) { > ns->geom.pgaddrbytes = 4; > ns->geom.secaddrbytes = 2; > } else { > ns->geom.pgaddrbytes = 5; > ns->geom.secaddrbytes = 3; > } > ... >} > >Will this mismatch stall the nandsim state machine and fail operations due to >an address cycle mismatch? > >Prior to this change, shrinking a 512 MiB device to 256 MiB left the stale >flag set, which serendipitously matched nandsim's expectation of 5 bytes. You are right, thanks for catching this. Re-evaluating the flag alone would regress 4 KiB-page devices overridden into the 128-256 MiB range: the row address still fits in 16 bits there (pgnum <= 65536), so the core would correctly drop the third byte while ns_init() keeps expecting it -- the stale flag used to match ns_init()'s expectation serendipitously. The inconsistent side is actually ns_init(): the hardcoded 128 MiB threshold encodes a 2 KiB-page assumption, while the core (and ONFI) keys the third row cycle on the row address being wider than 16 bits, ie. more than 65536 pages, regardless of the page size. The small-page branch (32 MiB of 512-byte pages = exactly 65536 pages) already matches that rule. v3 derives ns_init()'s expectation from the page count as well, so both sides agree on every geometry: - if (ns->geom.totsz <= (128 << 20)) { + if (ns->geom.pgnum <= (1 << 16)) { > >> } >> >> ret = ns_setup_wear_reporting(nsmtd);
diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c index fe96803..ac82dd7 100644 --- a/drivers/mtd/nand/raw/nandsim.c +++ b/drivers/mtd/nand/raw/nandsim.c @@ -2359,6 +2359,11 @@ static int __init ns_init_module(void) targetsize = nanddev_target_size(&chip->base); chip->chip_shift = ffs(nsmtd->erasesize) + overridesize - 1; chip->pagemask = (targetsize >> chip->page_shift) - 1; + + if (chip->chip_shift - chip->page_shift > 16) + chip->options |= NAND_ROW_ADDR_3; + else + chip->options &= ~NAND_ROW_ADDR_3; } ret = ns_setup_wear_reporting(nsmtd);
The overridesize module parameter changes the size of the simulated device after nand_scan() has completed. It updates nsmtd->size, memorg->eraseblocks_per_lun, chip->chip_shift and chip->pagemask to match the new geometry, but leaves the NAND_ROW_ADDR_3 option untouched, even though nand_scan_ident() set it from the geometry decoded out of the ID bytes. When the ID bytes describe a device larger than 128 MiB and overridesize shrinks the simulation to 128 MiB or less, the stale option makes the core emit one extra row address byte (page >> 16, always zero given the reduced page count) in every read, program and erase operation, while the simulator state machine expects one byte less. All accesses then fail with: nandsim: error: write_byte: address (0x0) isn't expected, expected state is STATE_CMD_READSTART, switch to STATE_READY The opposite direction is equally broken: growing a small device past 128 MiB keeps NAND_ROW_ADDR_3 cleared, so the third row address byte is dropped and the wrong pages are silently addressed. This used to work before commit 14157f861437 ("mtd: nand: introduce NAND_ROW_ADDR_3 flag"). Back then nandsim kept chip->chipsize in sync with the override, and nand_command_lp() decided at run time, per command, whether the third row address cycle was needed by testing chip->chipsize against 128 MiB. This code thus always saw the overridden size. The above commit moved the decision to nand_scan_ident(), which encodes it once into NAND_ROW_ADDR_3 at scan time -- before nandsim applies the override -- and the overridesize path was never taught to re-evaluate the flag. Re-evaluate NAND_ROW_ADDR_3 right after overriding chip_shift, using the same test as nand_scan_ident(), so that the address width emitted by the core always matches ns->geom.pgaddrbytes, which ns_init() derives from the overridden total size. Fixes: 14157f861437 ("mtd: nand: introduce NAND_ROW_ADDR_3 flag") Signed-off-by: wang wei <a929244872@163.com> --- drivers/mtd/nand/raw/nandsim.c | 5 +++++ 1 file changed, 5 insertions(+)