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