Fritzbox memory chip misdetection since 0bc794a6 (master and 22.03)

[hnyman]

cc @andyboeh and @chunkeey

Based on forum discussion, the commit 0bc794a "kernel: add support for Toshiba TC58NVG0S3HTA00 NAND flash" causes flash memory chip misdetection for some other Fritzbox devices, as the commit only defines a 4-byte flash memory chip ID that matches several chips used in the devices.

See discussion from this onward
https://forum.openwrt.org/t/openwrt-22-03-0-rc1-first-release-candidate/126045/182

OpenWrt 22.03.0-rc2 and rc3 are causing on a Fritzbox 7412 bootloops due to a misdetected flash chip.

Yup, that patch is missing the 5th ID byte entirely - both chips share the same first 4;

TC58NVG0S3HTA00 = 0x98 0xf1 0x80 0x15 0x72 (digikey datasheet, page 35)
TC58BVG0S3HTA00 = 0x98 0xf1 0x80 0x15 0xf2 (digikey datasheet, page 28)

The commit has also been backported to openwrt-22.03 after rc1, so both rc2 and rc3 suffer from this bug.

[chunkeey]

From the thread:

https://forum.openwrt.org/t/openwrt-22-03-0-rc1-first-release-candidate/126045/194

have you updated the id_len field in the second row of the https://elixir.bootlin.com/linux/latest/source/include/linux/mtd/rawnand.h#L1416 1 to 5 as well? (otherwise it would just match the first four bytes which would cause something like this to happen.)

ie:

 {"TC58NVG0S3HTA00 1G 3.3V 8-bit",
         { .id = {0x98, 0xf1, 0x80, 0x15, 0x72, } },
           SZ_2K, SZ_128, SZ_128K, 0, 5, 128, NAND_ECC_INFO(8, SZ_512), }, 

https://forum.openwrt.org/t/openwrt-22-03-0-rc1-first-release-candidate/126045/198

That's the missing bit [...]

What I can confirm without that patch 22.03.0-rc3 works just fine. Next trial tomorrow with your changed line...

a ready to merge patch would be greatly appreciated. (Ideally also sent to upstream linux.

Ideally with the following tags:

Reported-by: Peter_vdL? (if he wants)
Tested-by: Peter_vdL (")
Suggested-by: neg2led (if he wants)
Suggested-by: hnyman (if he wants)

Fixes: 3380557fc7e2 ("mtd: rawnand: add support for Toshiba TC58NVG0S3HTA00 NAND flash")
BugLink: #9962

anything else?

(EDIT: oh yea. Does anybody know how long that ID is? Other chips have 8 bytes. Are 5 enough, or is there
potential for 6, 7 or 8 bytes too?)

[edrikk]

#9905

Related

[andyboeh]

fyi: I checked on the FRITZ!7530 this patch was introduced for: The chip really only reports a four-byte ID. Changing it to five-bytes results in the device not booting.

I also checked the length of the ID in nand_base.c by printing chip->id.len with printk(): Length is 4. You can test this on your chip by temporarily editing the file nand_base.c in: build_dir/target-arm_cortex-a7+neon-vfpv4_musl_eabi/linux-ipq40xx_generic/linux-5.10.113/drivers/mtd/nand/raw. Insert line 4745 (right after chip->id.len = nand_id_len(id_data, ARRAY_SIZE(chip->id.data));):

printk(KERN_WARNING "Chip ID Length is: %d\n", chip->id.len);

I also found out that AVM matches just the four-byte ID in its GPL source code.

Is the order important? I.e. could you match the five-byte ID for your chip and still have the four-byte ID as fallback for my chip?

[Peter-vdL]

I built a version with @andyboeh's patch modified like this:

--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -29,6 +29,12 @@ struct nand_flash_dev nand_flash_ids[] =
        {"TC58NVG0S3E 1G 3.3V 8-bit",
                { .id = {0x98, 0xd1, 0x90, 0x15, 0x76, 0x14, 0x01, 0x00} },
                  SZ_2K, SZ_128, SZ_128K, 0, 8, 64, NAND_ECC_INFO(1, SZ_512), },
+       {"TC58NVG0S3HTA00 1G 3.3V 8-bit",
+               { .id = {0x98, 0xf1, 0x80, 0x15, 0x72} },
+                 SZ_2K, SZ_128, SZ_128K, 0, 5, 128, NAND_ECC_INFO(8, SZ_512), },
+       {"TC58BVG0S3H 1G 3.3V 8-bit",
+               { .id = {0x98, 0xf1, 0x80, 0x15, 0xf2} },
+                 SZ_2K, SZ_128, SZ_128K, 0, 5, 64, NAND_ECC_INFO(8, SZ_512), },
        {"TC58NVG2S0F 4G 3.3V 8-bit",
                { .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08} },
                  SZ_4K, SZ_512, SZ_256K, 0, 8, 224, NAND_ECC_INFO(4, SZ_512) },

--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4742,6 +4742,8 @@
        }
 
        chip->id.len = nand_id_len(id_data, ARRAY_SIZE(chip->id.data));
+       printk(KERN_WARNING "Chip ID Length is: %d\n", chip->id.len);
+       printk(KERN_WARNING "Chip ID Data is: %d\n", chip->id.data);
 
        /* Try to identify manufacturer */
        manufacturer_desc = nand_get_manufacturer_desc(maf_id);

That gives me following output of the command logread | grep -A2 -B2 'nand: device found':

Sat May 21 18:52:47 2022 kern.warn kernel: [    8.505150] Chip ID Length is: 7
Sat May 21 18:52:47 2022 kern.warn kernel: [    8.506960] Chip ID Data is: -2124053616
Sat May 21 18:52:47 2022 kern.info kernel: [    8.510872] nand: device found, Manufacturer ID: 0x98, Chip ID: 0xf1
Sat May 21 18:52:47 2022 kern.info kernel: [    8.517225] nand: Toshiba TC58BVG0S3H 1G 3.3V 8-bit
Sat May 21 18:52:47 2022 kern.info kernel: [    8.522110] nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64

(Would been better to leave the 2nd printk on hex, noticed it too late - a dec-hex-converter gives unusual numbers)
So in my case it recognizes even more than the five bits that're needed to fully identify that chip, beside it's booting fine.

I had in the meantime problems building OpenWrt even with a repo cleaned with make distclean, it broke every time at the same point even with a vanilla .config, only a plain reinstall worked. So to rule that possibility out would you @andyboeh try this too, or if you trust me I have built a version for the 7430 too, just booting the initramfs-version is enough to test it, a reset and all my bad vibes are gone... Where to put that file and how if you wanna have it?

One thing I noticed looking around inside the directory where the files are laying: There is a file that tests for exactly that kind of chips, nand_toshiba.c. Is it possible that this file for some odd reason isn't called or don't get all it's needed information? In it there are hints that the in the datasheet undocumented 6th field codes the chips structure widthness. Let's 've had a change there we're or better that file is not aware of... Or an other possibility that comes to my mind is that we've found a device with a not fully compilant flash chip...

[andyboeh]

@Peter-vdL I tested this just yesterday and the result is a 4-byte ID - I don't get what you would like to rule out?
The next possibility for doing any tests from my side is in a few days, sorry, I don't have regular access to this device.
nand_toshiba.c is called and works fine - but not on the TC58NVG0S3HTA00. That's the reason for the entry in nand_ids.c, to avoid going through the auto-detection on this particular chip. The auto-detection might just work fine on yours, but IMHO that makes matters more complicated.

Could you try if your chip works with a 5-byte entry for your device and a more "generic" 4-byte entry for mine? The 4-byte entry needs to be after the 5-byte one.

Edit: Could you try this patch? NB: Hand-edited, untested.

--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -29,6 +29,12 @@ struct nand_flash_dev nand_flash_ids[] =
        {"TC58NVG0S3E 1G 3.3V 8-bit",
                { .id = {0x98, 0xd1, 0x90, 0x15, 0x76, 0x14, 0x01, 0x00} },
                  SZ_2K, SZ_128, SZ_128K, 0, 8, 64, NAND_ECC_INFO(1, SZ_512), },
+       {"TC58BVG0S3H 1G 3.3V 8-bit",
+               { .id = {0x98, 0xf1, 0x80, 0x15, 0xf2} },
+                 SZ_2K, SZ_128, SZ_128K, 0, 5, 64, NAND_ECC_INFO(8, SZ_512), },
+       {"TC58NVG0S3HTA00 1G 3.3V 8-bit",
+               { .id = {0x98, 0xf1, 0x80, 0x15} },
+                 SZ_2K, SZ_128, SZ_128K, 0, 4, 128, NAND_ECC_INFO(8, SZ_512), },
        {"TC58NVG2S0F 4G 3.3V 8-bit",
                { .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08} },
                  SZ_4K, SZ_512, SZ_256K, 0, 8, 224, NAND_ECC_INFO(4, SZ_512) },

--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4742,6 +4742,8 @@
        }
 
        chip->id.len = nand_id_len(id_data, ARRAY_SIZE(chip->id.data));
+       printk(KERN_WARNING "Chip ID Length is: %d\n", chip->id.len);
+       printk(KERN_WARNING "Chip ID Data is: %x\n", chip->id.data);
 
        /* Try to identify manufacturer */
        manufacturer_desc = nand_get_manufacturer_desc(maf_id);

If this works on both devices, we should quickly send it upstream. A comment should IMHO also be added to point out that the N-version needs to go last.

[Peter-vdL]

With that changed patch it works, but TBH I'm not happy with it...

Te more I think on that issue the more I like the idea of a probably fraud nand chip you've in your device. According to the datasheet there have to be at least 5 ID fields 'cause otherwise there is no chance to detect the chip correctly by the ID - for what it exist. To verify this the only chance we have is to test it against an as identical OpenWrt-build as possible, otherwise it opens up other possibilities. Verification by exclusion, 'cause I can't imagine that AVM or Toshiba 're tellng us the truth if they even talk to us if we gently ask, we have to deliver facts to them to break the silence.

But that opens another much bigger can. That means we have to put in code to detect fraud flash chips in the kernel prior to using 'em no matter where the flash is, and that's not limited to flash, we have to test absolute everything prior to using it and can't trust the manufacturers anymore. I think that'd be barely feasible. I hate if somebody gets away with such things, that's why I want to rule that possibility out.

Anyway, if we change your patch to

--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -29,6 +29,12 @@ struct nand_flash_dev nand_flash_ids[] =
        {"TC58NVG0S3E 1G 3.3V 8-bit",
                { .id = {0x98, 0xd1, 0x90, 0x15, 0x76, 0x14, 0x01, 0x00} },
                  SZ_2K, SZ_128, SZ_128K, 0, 8, 64, NAND_ECC_INFO(1, SZ_512), },
+       {"TC58BVG0S3H 1G 3.3V 8-bit",
+               { .id = {0x98, 0xf1, 0x80, 0x15, 0xf2} },
+                 SZ_2K, SZ_128, SZ_128K, 0, 5, 64, NAND_ECC_INFO(8, SZ_512), },
+       {"TC58NVG0S3HTA00 1G 3.3V 8-bit",
+               { .id = {0x98, 0xf1, 0x80, 0x15} },
+                 SZ_2K, SZ_128, SZ_128K, 0, 4, 128, NAND_ECC_INFO(8, SZ_512), },
        {"TC58NVG2S0F 4G 3.3V 8-bit",
                { .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08} },
                  SZ_4K, SZ_512, SZ_256K, 0, 8, 224, NAND_ECC_INFO(4, SZ_512) },

the system should boot flawlessly on both devices and probably others.

[andyboeh]

Te more I think on that issue the more I like the idea of a probably fraud nand chip you've in your device.

I had the very same thought the moment the datasheets with the correct IDs were posted. Could be that AVM had problems sourcing the flash chips and used some not-so-trusty supplier.

But that opens another much bigger can.

It does. It means that every legit ID has to be put in there even if the auto-detection would work perfectly fine for this chip family.

I'm unsure how to proceed, especially upstream. If this is really a fake chip, then IMHO the upstream patch should be reverted and we should stick with this patch as an OpenWrt-specific hack.

[Peter-vdL]

I have an idea: It seems out of the behavior of your device that the B-model with auto-ECC is the default or at least it falls back to this. What if we change this and define the N-model without ECC as the default and go completely via autodetect? Then it won't matter that there are chips out there with missing fifth ID bit - as long as they aren't ones with ECC built in. The only thing we have to get on is if it's a toshiba chip - for that the first four ID bits are sufficient. What calls nand_toshiba.c and how to tell it what defaults to use if it can't find any better suited values?

If a vendor knows that on this particular hardware are only chips of one model he can go with specially patched firmware, even like in our case when the hardware is never meant to carry an other firmware than that of the manufacturer. The only thing he has to ensure is that the hardware works - with his firmware. Written that I can imagine that AVM could go with slightly out-of-spec chips - eg. when in-spec chips are out of stock or something like that. We'll never know.

[andyboeh]

ECC is not the problem. It's the size of the OOB area that's incorrectly detected: The chip has 128 bytes OOB while the auto-detection thinks it's 64 bytes. nand_toshiba.c actually ignores my chip since the ID is too short. On my chip, it falls back to oobsize = memorg / 32 = 64 bytes.

AFAICT, the detection is here: https://github.com/torvalds/linux/blob/8171acb8bc9b33f3ed827f0615b24f7a06495cd0/drivers/mtd/nand/raw/nand_toshiba.c#L146

The detection routine is in nand_base.c (https://github.com/torvalds/linux/blob/8171acb8bc9b33f3ed827f0615b24f7a06495cd0/drivers/mtd/nand/raw/nand_base.c#L4989): It first reads the ID, decodes manufacturer and model data and tries to match it against nand_ids.c. If that doesn't work, it tries the manufacturer-specific detection methods. And if that doesn't work either, it falls back to legacy, hard-coded defaults - this is what happens with my NAND.

[chunkeey]

Sorry, I can only visit on the weekend. But oh boy, yea. This is a can of worms.

@andyboeh just for fun, could you add the following: (hope it has no errors, I wrote this but haven't tested it)

print_hex_dump_bytes("NANDID:", DUMP_PREFIX_NONE, id_data, sizeof(ARRAY_SIZE(chip->id.data)));

before the nand_id_len thiny? This is because the printk(KERN_WARNING "Chip ID Data is: %x\n", chip->id.data);
prints just 4 bytes of the pointer to data... and not the data itself. you would need something like PRIx64 / "%lx" for the format and *((u64 *)chip->id.data) for the printout

If the nand_id_len = 4 (due to the period 4) hold true, it would be expected to look like:

0x98 0xf1 0x80 0x15 0x98 0xf1 0x80 0x15

[chunkeey]

Found something:

https://linux-mtd.infradead.narkive.com/8DRxas2M/patch-mtd-nand-detect-oob-size-for-toshiba-24nm-raw-slc

@computersforpeace do you still have this hardware with the "TC58NVG0S3HTA00" ?

I will provide the 8-byte ID strings from the two 24nm Toshiba samples I
have; their first 6 bytes match the documentation I received from
Toshiba:

24nm SLC 1Gbit TC58NVG0S3HTA00
0x98 0xf1 0x80 0x15 0x72 0x16 0x08 0x00

I'm asking because there seem to be counterfeit TC58NVG0S3HTA00 going around.

@andyboeh, @Peter-vdL

this is kinda stupid WA that I came up with. If the ID of @andyboeh is what I think it is in my previous post.
This following, "dodgy" entry in the nand id could be "good enough" to correctly identify Andreas chip and not cause havoc for others?

+       {"TC58NVG0S3HTA00 1G 3.3V 8-bit",   /* possibly counterfeit chip */
+               { .id = {0x98, 0xf1, 0x80, 0x15, 0x98, 0xf1, 0x80, 0x15 } },
+                 SZ_2K, SZ_128, SZ_128K, 0, 8, 128, NAND_ECC_INFO(8, SZ_512), }

(The reason why this could work is that "find_full_id_nand()" takes the id_len from the nand table and not the chip.
So if the ID repeats this entry would match for just this chip.

[andyboeh]

@chunkeey
No worries, I can do testing with the hardware only at the weekend as well.

I changed your patch to print_hex_dump(KERN_WARNING, "NANDID:", DUMP_PREFIX_NONE, 16, 1, id_data, sizeof(ARRAY_SIZE(chip->id.data)), true); to see the output in the log. The output is:

NANDID:98 f1 80 15 ....

Then I tried your hack with the repeated ID, unfortunately, it does not match. It reverts back to the default detection method, giving an OOB size of 64 bytes.

IMHO the current way to go is:

• Revert patch upstream
• Add an OpenWrt hack:
  • Add the B-version ID first
  • Add the "generic" ID as fallback for the possibly counterfeit devices

What do you think?

[Peter-vdL]

Is it possible to detect the bit flips in a safe manner and if they occur test the chip with an other OOB size whether the bit flips disappear? That would make it possible if all the detection fails and nand_base.c would fall back to legacy to have one last "brute force" trial to get the chip right without the need of an entry in the nand_ids.c file - and without the danger of breaking good findings of non-malicious chips.

[edrikk]

While all of this is figured out, can someone revert the OpenWRT back ports in master and 22.03?

Leaving a patch known to brick existing supported devices this long is not the right approach. Especially since it’s in 22.03 RC builds.