| Message ID | 20261001103359.20216-1-a929244872@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize | expand |
Hi Wang, On 01/10/2026 at 18:33:59 +08, wang wei <a929244872@163.com> wrote: > 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. Fine until here. > 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. The decision thus always saw the "The decision"? > 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. The > commit that introduced the regression is: > > https://git.kernel.org/torvalds/c/14157f861437ebe2d624b0a845b91bbdf8ca9a2d This is not needed, you mention it above and below already. > 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") Cc: stable > Signed-off-by: wang wei <a929244872@163.com> > --- > drivers/mtd/nand/raw/nandsim.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c > index fe96803..176db89 100644 > --- a/drivers/mtd/nand/raw/nandsim.c > +++ b/drivers/mtd/nand/raw/nandsim.c > @@ -2359,6 +2359,18 @@ 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; > + > + /* > + * NAND_ROW_ADDR_3 was set by nand_scan_ident() from the > + * geometry decoded out of the ID bytes, which the size > + * override has just made stale. Re-evaluate it against > + * the new geometry, otherwise the core emits one more (or > + * one less) row address byte than the simulator expects. > + */ Everything above this line can just be deleted. Tell your LLM that this is obvious enough and does not require any particular comment. > + 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); Instead of repeating the operation in nandsim, why not calling nand_scan() after over writing the size? (inverting the two blocks) Thanks, Miquèl
>>> + 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); >> >>Instead of repeating the operation in nandsim, why not calling >>nand_scan() after over writing the size? (inverting the two blocks) > > The override cannot be moved before nand_scan(): In nand_detect(), > chip ->chipushift will be set to the actual flash size. In addition, > In addition, expressed in erase blocks (new_size = nsmtd->erasesize << overridesize) > and erasesize is what nand_scan_ident() decodes from the ID bytes, so > the override has a hard data dependency on the scan. Ok, fine then.
>Hi Wang, > >On 01/10/2026 at 18:33:59 +08, wang wei <a929244872@163.com> wrote: > >> 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. > >Fine until here. Okay > >> 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. The decision thus always saw the > >"The decision"? What I mean is the semantics of this code > >> 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. The >> commit that introduced the regression is: >> >> https://git.kernel.org/torvalds/c/14157f861437ebe2d624b0a845b91bbdf8ca9a2d > >This is not needed, you mention it above and below already. Okay > >> 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") > >Cc: stable > >> Signed-off-by: wang wei <a929244872@163.com> >> --- >> drivers/mtd/nand/raw/nandsim.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c >> index fe96803..176db89 100644 >> --- a/drivers/mtd/nand/raw/nandsim.c >> +++ b/drivers/mtd/nand/raw/nandsim.c >> @@ -2359,6 +2359,18 @@ 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; >> + >> + /* >> + * NAND_ROW_ADDR_3 was set by nand_scan_ident() from the >> + * geometry decoded out of the ID bytes, which the size >> + * override has just made stale. Re-evaluate it against >> + * the new geometry, otherwise the core emits one more (or >> + * one less) row address byte than the simulator expects. >> + */ > >Everything above this line can just be deleted. Tell your LLM that this >is obvious enough and does not require any particular comment. Okay, will be modified in V2 > >> + 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); > >Instead of repeating the operation in nandsim, why not calling >nand_scan() after over writing the size? (inverting the two blocks) The override cannot be moved before nand_scan(): In nand_detect(), chip ->chipushift will be set to the actual flash size. In addition, In addition, expressed in erase blocks (new_size = nsmtd->erasesize << overridesize) and erasesize is what nand_scan_ident() decodes from the ID bytes, so the override has a hard data dependency on the scan. > >Thanks, >Miquèl
diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c index fe96803..176db89 100644 --- a/drivers/mtd/nand/raw/nandsim.c +++ b/drivers/mtd/nand/raw/nandsim.c @@ -2359,6 +2359,18 @@ 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; + + /* + * NAND_ROW_ADDR_3 was set by nand_scan_ident() from the + * geometry decoded out of the ID bytes, which the size + * override has just made stale. Re-evaluate it against + * the new geometry, otherwise the core emits one more (or + * one less) row address byte than the simulator expects. + */ + 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. The decision 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. The commit that introduced the regression is: https://git.kernel.org/torvalds/c/14157f861437ebe2d624b0a845b91bbdf8ca9a2d 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 | 12 ++++++++++++ 1 file changed, 12 insertions(+)