zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.12k stars 6.21k forks source link

nvs failed to init on stm32f4 flash layout #19895

Open erwango opened 4 years ago

erwango commented 4 years ago

Describe the bug [00:00:00.006,000] <err> fs_nvs: Invalid sector size on nucleo_f429zi + x_nucleo_idb05a1

STM32F4 parts have a flash layout composed of several blocks of variable size. For instance on F429ZI

static const struct flash_pages_layout stm32f4_flash_layout[] = {
    {.pages_count = 4, .pages_size = KB(16)},
    {.pages_count = 1, .pages_size = KB(64)},
    {.pages_count = 7, .pages_size = KB(128)},
    {.pages_count = 4, .pages_size = KB(16)},
    {.pages_count = 1, .pages_size = KB(64)},
    {.pages_count = 7, .pages_size = KB(128)},
};

Storage flash partition is defined as follows:

        /* storage: 64KB for settings */
        storage_partition: partition@10000 {
            label = "storage";
            reg = <0x00010000 0x00010000>;
        };

For some reason, flash driver doesn't manage to retrieve valid sector_size in this set-up.

To Reproduce Steps to reproduce the behavior:

  1. cd samples/bluetooth/peripheral
  2. west build -b nucleo_f429zi -- -DSHIELD=x_nucleo_idb05a1
  3. west flash

Expected behavior NCS could be set up sucessfully

Impact Can't use settings on stm32f4 parts

Screenshots or console output

Bluetooth init failed (err -22)
[00:00:00.006,000] <err> fs_nvs: Invalid sector size
[00:00:00.006,000] <err> bt_settings: settings_subsys_init failed (err -22)

Environment (please complete the following information): Requires https://github.com/zephyrproject-rtos/zephyr/pull/19858

Additional context Add any other context about the problem here.

erwango commented 4 years ago

Issue is fs->sector_size = 0 in nvs_init, which is due to:

    nvs_sector_size = CONFIG_SETTINGS_NVS_SECTOR_SIZE_MULT *
              hw_flash_sector.fs_size;

With CONFIG_SETTINGS_NVS_SECTOR_SIZE_MULT set to 1. Then I'm loosing track.

erwango commented 4 years ago

Thanks to @Laczen help, we've identified that nucleo_f429zi assigns NVS storage partition on one sector. Though NVS requires minimum 2 sectors (so atht one can be rased when full wi/o loss of information, documented here: https://docs.zephyrproject.org/latest/reference/storage/nvs/nvs.html).

Board partition should be reviewed. Meanwhile, following storage configuration is working:

        /* storage: 32KB for settings */
        storage_partition: partition@100000 {
            label = "storage";
            reg = <0x100000 0x8000>;
        };
erwango commented 4 years ago

I've tried to reorganized nucleo_f429zi storage to fit in first 1MB, I'm using following description:

        /* storage: 32KB for settings (2x16Kb sectors) */
        storage_partition: partition@8000 {
            label = "storage";
            reg = <0x00008000 0x00008000>;
        };

This is failing while it actually respects NVS stated constraints (2 sectors, total < 64Kb). Error happens in should_bail, which sets status to ENOMEM when inspecting last sector:

(gdb) p *data
$14 = {area_idx = 0, area_off = 32768, area_len = 32768, ret = 0x20002258 <z_main_stack+904>, ret_idx = 1, ret_len = 1, status = 0}
erwango commented 4 years ago

Summary, based on the flash sectors I'm trying to use

    {.pages_count = 4, .pages_size = KB(16)},   > 1&2: ko, 3&4: ko, 1&2&3: ko
    {.pages_count = 1, .pages_size = KB(64)},
    {.pages_count = 7, .pages_size = KB(128)},
    {.pages_count = 4, .pages_size = KB(16)},  >  1&2: ok, 3&4: ok
    {.pages_count = 1, .pages_size = KB(64)},
    {.pages_count = 7, .pages_size = KB(128)},

For some reason i can't use 4 first slots of the flash.

erwango commented 4 years ago

zephyr,code-partition is no-op when mcuboot is not used. So binary is linked at start of flash and defined partition doesn't actually apply in tree ...

erwango commented 4 years ago

Removing 'bug' as this actually not a bug. Requires an enhancement: #20070

akofoed commented 4 years ago

Is this modulo operation not the wrong way around (nvs.c line 722):

    if (!fs->sector_size || fs->sector_size % info.size) {
        LOG_ERR("Invalid sector size");
        return -EINVAL;
    }

I guess that info.size is the flash total size and sector_size is the multiples that comprises the total flash size. That is: if (!fs->sector_size || info.size % fs->sector_size) { My code is working with this change (nvs.c line 722). Can anyone confirm this?

Laczen commented 4 years ago

@akofoed, no info.size is the flash page size and fs->sector_size should be a multiple of this.

akofoed commented 4 years ago

In my case I have a info.size=65536 and a sector_size=4096 (MX25R64 spi flash) Therefore: if (!fs->sector_size || fs->sector_size % info.size) { is expanded into: if (!65536 || 4096 % 65536) { which evaluates to true because 4096 % 65536 == 4096

As there are multiple sectors in a page/block/info-size, the modulo expression must test if "a page is comprised of an exact multiple of sectors. Not if "...a sector size is a (=) multiple of a (*) flash page..."

So should it not be like this?: if (!65536 || 65536 % 4096) {

Laczen commented 4 years ago

In my case I have a info.size=65536 and a sector_size=4096 (MX25R64 spi flash) Therefore: if (!fs->sector_size || fs->sector_size % info.size) { is expanded into: if (!65536 || 4096 % 65536) { which evaluates to true because 4096 % 65536 == 4096

As there are multiple sectors in a page/block/info-size, the modulo expression must test if "a page is comprised of an exact multiple of sectors. Not if "...a sector size is a (=) multiple of a (*) flash page..."

So should it not be like this?: if (!65536 || 65536 % 4096) {

No it should not be like that. A sector needs to be at least one flash page. The flash page is what is erased in one go. If you make the sector smaller and you need to do garbage collecting you will erase the page (since this is the smallest region that can be erased) and all data that was on previous sectors will be erased. If the flash page is correctly reporting 64kB then you will need to make your sectors at least 64kB.

!!! But this won't work since the maximum sector size is 32kB !!!

GeorgePriestner-Eaton commented 1 year ago

I'm still not able to use nvs even with the modified storage partition size. I placed

/* storage: 32KB for settings (2x16Kb sectors) */
    storage_partition: partition@10000 {
        label = "storage";
        reg = <0x00010000 0x0008000>;
};

in my project .overlay file, however...

fs.offset = FLASH_AREA_OFFSET(storage);
rc = flash_get_page_info_by_offs(flash_dev, fs.offset, &info);

info.size is still reporting as 0x10000 (64kB) incorrectly!

I checked the zephyr.dts file in the build directory and it matches the new storage size set in my overlay file. Any ideas?

Laczen commented 1 year ago

I'm still not able to use nvs even with the modified storage partition size. I placed

/* storage: 32KB for settings (2x16Kb sectors) */
    storage_partition: partition@10000 {
      label = "storage";
      reg = <0x00010000 0x0008000>;
};

in my project .overlay file, however...

fs.offset = FLASH_AREA_OFFSET(storage);
rc = flash_get_page_info_by_offs(flash_dev, fs.offset, &info);

info.size is still reporting as 0x10000 (64kB) incorrectly!

I checked the zephyr.dts file in the build directory and it matches the new storage size set in my overlay file. Any ideas?

Isn't the flash at 0x10000 a block of 64kB ?

If you would like to use the 16kB sectors you need to use a location below 0x10000.

GeorgePriestner-Eaton commented 1 year ago

Thanks to @Laczen help, we've identified that nucleo_f429zi assigns NVS storage partition on one sector. Though NVS requires minimum 2 sectors (so atht one can be rased when full wi/o loss of information, documented here: https://docs.zephyrproject.org/latest/reference/storage/nvs/nvs.html).

Board partition should be reviewed. Meanwhile, following storage configuration is working:

      /* storage: 32KB for settings */
      storage_partition: partition@100000 {
          label = "storage";
          reg = <0x100000 0x8000>;
      };

It is a block of 64kB flash yes, but thought I'd try the suggestion above anyway. I tried using sectors 2&3 (offset 0x00008000) but no luck with them either. I defined my own region "xdl-storage" but nvs_init fails with this.

xdl_storage_partition: partition@8000 {
    label = "xdl-storage";
    reg = < 0x8000 0x8000 >;
};

I set

fs.sector_size = info.size;
fs.sector_count = 2U;

info.size gets correctly set to 0x4000.

Laczen commented 1 year ago

@GeorgePriestner-Eaton, are you using a bootloader that is larger than 32kB? Then nvs init should fail because the flash area contains other data. The working example at the high flash range is working as this area is not covered by a bootloader.

GeorgePriestner-Eaton commented 1 year ago

@Laczen No I'm not using any bootloader so nothing should be stored in that section. I even tried re-allocating the mcuboot partition to a smaller size. With this nvs_init does not crash the system but returns -45.

        boot_partition: partition@0 {
            label = "mcuboot";
            reg = <0x00000000 0x00004000>;
            read-only;
        };

    /* storage: 32KB for settings (2x16Kb sectors) */
    xdl_storage_partition: partition@4000 {
        label = "xdl-storage";
        reg = <0x00004000 0x0008000>;
    };

    flash_dev = FLASH_AREA_DEVICE(xdl_storage);

    if (!device_is_ready(flash_dev)) 
    {
        printk("Flash device %s is not ready\n", flash_dev->name);
        return;
    }
    fs.offset = FLASH_AREA_OFFSET(xdl_storage);
    rc = flash_get_page_info_by_offs(flash_dev, fs.offset, &info);

    if (rc) 
    {
        printk("Unable to get page info\n");
        return;
    }
    fs.sector_size = info.size;
    fs.sector_count = 2U;

    rc = nvs_init(&fs, flash_dev->name);
    if (rc) 
    {
        printk("Flash Init failed\n");
        return;
    }
Laczen commented 1 year ago

@GeorgePriestner-Eaton, are you sure nothing is on the flash. To protect against accidental data removal nvs_init fails on a sector that contains some info that is not nvs. As there is nothing on areas please try to erase them first.

zephyrbot commented 5 months ago

Hi @erwango,

This issue, marked as an Enhancement, was opened a while ago and did not get any traction. Please confirm the issue is correctly assigned and re-assign it otherwise.

Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason.

Thanks!