[PATCH V2] drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize

[PATCH V2] drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize

[wang wei]

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(+)

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);

-- 
2.55.0.windows.3

Re: [PATCH V2] drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize

[sashiko-bot]

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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001124733.43559-1-a929244872@163.com?part=1

Re:Re: [PATCH V2] drivers/mtd: rawnand: nandsim: Fix stale NAND_ROW_ADDR_3 with overridesize

[wang wei]

>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);