LittleFS_QPINAND::getMediaName() broken

[h4yn0nnym0u5e]

Despite a valid part being fitted, when this function is called the chip ID is not read correctly, resulting in a NULL info pointer, which is not checked and is dereferenced, causing a Data Access Violation. The part fitted is a W25N01GVZEIG. Example code:

#include <LittleFS.h>

const char* szDiskMem = "QSPI_NAND";
LittleFS_QPINAND myfs;

void setup()
{
  while (!Serial)  // wait for serial to connect
    ;
  if (CrashReport)
    Serial.println(CrashReport);
    
  Serial.println("Start LittleFS...");
  if (!myfs.begin())
  {
    Serial.printf("Error starting %s\n", szDiskMem);
    while (1)
      ;
  }
  const char* mn = myfs.getMediaName();
  Serial.printf("Chip is %s\n",mn?mn:"unknown - nullptr");
  Serial.println("setup() complete");
}

void loop()
{

}

 

[mjs513]

Despite a valid part being fitted, when this function is called the chip ID is not read correctly, resulting in a NULL info pointer, which is not checked and is dereferenced, causing a Data Access Violation. The part fitted is a W25N01GVZEIG. Example code:

Finally got my test board out and can confirm something is wrong - I am seeing the same issue. Without calling getMediaName failing. Its been a while since I worked on this but will see I can remember.

 

[mjs513]

Just a follow up - looks like the cmd 8 needs to be initialialized, so the new function should read:

const char * LittleFS_QPINAND::getMediaName(){
  uint8_t buf[5] = {0, 0, 0, 0, 0};
    // cmd index 8 = read ID bytes
    FLEXSPI2_LUT32 = LUT0(CMD_SDR, PINS1, 0x9F) | LUT1(READ_SDR, PINS1, 1);
    FLEXSPI2_LUT33 = 0;
  flexspi2_ip_read(8, 0x00800000, buf, 4);
  const struct chipinfo *info = chip_lookup(buf+1);

  return info->pn;
}

when you add that then:

Start LittleFS...
Chip is W25N01GVZEIG
setup() complete

Before I shoot Paul a PR want to do some more testing but getting late here now but will do in the morning.

 

[jmarsh]

Doesn't the LUT need to be unlocked to modify commands? Or is it just left unlocked forever after startup (which kind of defeats the purpose of it being lockable...)?

 

[mjs513]

@PaulStoffregen
Updated existing PR for this fix. Did run @defragsters QPINAND data integrity sketch and all appears working.

Did notice that there are a few outstading PRs against LittleFS that were never incorporated. Probably should be updated.

FYI: used my old test board:

 

[h4yn0nnym0u5e]

That’s great, thanks @mjs513. I tried to figure it out, but the complexity of FLEXSPI and general lack of comments left me totally at sea…

One suggestion … it would be a good plan to check for a nullptr return from chip_lookup() and return a nullptr if that happens, otherwise there’s still the opportunity for a crash within the library code if there’s no QSPI NAND fitted, or it’s not on the recognised list.

 

[KurtE]

That’s great, thanks @mjs513. I tried to figure it out, but the complexity of FLEXSPI and general lack of comments left me totally at sea…

One suggestion … it would be a good plan to check for a nullptr return from chip_lookup() and return a nullptr if that happens, otherwise there’s still the opportunity for a crash within the library code if there’s no QSPI NAND fitted, or it’s not on the recognised list.

void setup() { while (!Serial) // wait for serial to connect ; if (CrashReport) Serial.println(CrashReport); Serial.println("Start LittleFS..."); if (!myfs.begin()) { Serial.printf("Error starting %s\n", szDiskMem); while (1) ; } const char* mn = myfs.getMediaName(); Serial.printf("Chip is %s\n",mn?mn:"unknown - nullptr"); Serial.println("setup() complete"); }

Interesting, I did a quote of this section of the post and the forum code jumbled it up.

@mjs513, @PaulStoffregen - in principle for robustness I somewhat agree. However in this case as well as most if not all of our
examples, this code will never run, as the code properly checks the return value for begin : if (!myfs.begin())
and loops...

But not sure in this case if it would be better to return a nullptr or if it should return an empty string: ""

Edit: And I would not hold up a release of Teensyduino for this.