Notice: We have migrated to GitLab launching 2024-05-01 see here: https://gitlab.rtems.org/

#4981 assigned defect

Overwork Flashdev

Reported by: Bernd Moessner Owned by: Bernd Moessner
Priority: normal Milestone:
Component: admin Version: 6
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

Hi, Aaron Nyholm has added a nice flashdev API this year. I wanted to put LittleFS on top of this API and encountered some difficulties / found some bugs / have some open questions / would like to make some breaking API changes. Therefore, I hope that Aaron can comment.

  1. C++ include guards are missing in flashdev.h
  1. The switch case in flashdev.c => rtems_flashdev_ioctl is missing a default case
  1. Is there a guideline for naming IOCTLs? There are several defines and functions which, I feel, could have a more descriptive name. For example, there is the define RTEMS_FLASHDEV_IOCTL_PAGE_COUNT which is used to get the number of pages within the flash. Could we have it called RTEMS_FLASHDEV_IOCTL_GET_PAGE_COUNT so that it is immediately clear what is good for? Basically, I would like to add "_GET" / "_get" at all appropriate places (i.e. refactor the corresponding function names, too). I guess there will be an issue with the 80 char per line limit, but I'd provide a proposal for handling them at a final stage when all other issues have been addressed.
  1. I need to be able to create more than one flashdev. Therefore, the mutex initializer in rtems_flashdev_do_init is a bit of a roadblocker as it would use the same name for the mutex inside each flashdev instance. From my point of view, the easiest way to deal with this is to have the mutex name as an argument to the initializer. However, I'd prefer to
    1. shorten the name - like FDEV_MTX_${Number}
    2. loop over ${Number} until we can create the mutex Is there an rtems_get_id_by_name? I've only found a function to get the name of an id.
  1. Some thoughts on the "regions". Regions behave much like paritions, except that not every region / partition gets its own file handle, right?
    1. Why not calling the regions partitions?
    2. Do we really need to set them up "dynamically", or could we pass a partition table to the flashdev initialisation?
    3. There are the IOCTLs RTEMS_FLASHDEV_IOCTL_REGION_SET and RTEMS_FLASHDEV_IOCTL_REGION_UNSET. Looking at the implementation I feel they do more than I'd expect. For example, RTEMS_FLASHDEV_IOCTL_REGION_SET creates the region, or even redefines a region if one has not called unset, and then it activates the region. I'd like to refactor this into four IOCTLs:
  • RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _CREATE
  • RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _DELETE
  • RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _ACTIVATE
  • RTEMS_FLASHDEV_IOCTL_ REGION or PARTITION _DEACTIVATE

For my use case, setting up a static partition table would be good enough. We could spare the _CREATE, _DELETE, check the partitions more thoroughly during initalisation (for example that they do not overlap), and would not have to provide a mechanism to be able to resize them during runtime.

Flashdev shall work for NAND flashes, too. I am not an expert when it comes to NAND flashes. Therefore, I would like to outline my basic understanding and hope someone tells me if I am completely off.

Some parts I find often in designs:

NOR - MT25Q https://www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_01g_bbb_0.pdf

NOR - S25FL512
https://www.infineon.com/dgdl/Infineon-S25FL512S_512_Mb_(64_MB)_3.0_V_SPI_Flash_Memory-DataSheet-v20_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ed046ae4b53

NAND - W25N01 https://www.mouser.de/datasheet/2/949/w25n01gv_revl_050918_unsecured-1489588.pdf


Memory layout: bytes form pages, pages form sectors and sectors form blocks. There is always some confusion as some vendors omit the term sector

Read operations: can start at every address and have arbitrary length

Write / Program operations: work on single pages. Therefore, at max. PAGE_SIZE Bytes can be written in one go. There is a wrap around in case the configured address was not byte 0 in the current page. Usually, the minimum write size is 1 byte. However, setting a minimum write size to a different value might allow using additional features (such as ECC for the s25fl when using 16 Byte writes to 16 byte aligned addresses).

Erase operations: work on sectors and they always erase the complete sector. If an address is passed which points right in the middle of the sector, also the sector memory before the address is erased.

  1. Wrt. the write operations. I think RTEMS_FLASHDEV_IOCTL_WRITE_BLOCK_SIZE is misleading as the term "block" is already defined in the context of flash memories. The implementation / comments / unit tests show that we are looking at the minimum write size.
    1. Could we refactor this to RTEMS_FLASHDEV_IOCTL_GET_MIN_WRITE_SIZE?
    2. Especially regarding the ECC features of some flashes. The addresses need to be aligned to RTEMS_FLASHDEV_IOCTL_GET_MIN_WRITE_SIZE. I think we should test the adress alignment on writes.
  1. Wrt. the erase operations. The erase operations work on sectors!
    1. I need an API extension to get the erase size
    2. We should test that the address passed to the erase function is aligned to the erase size.
    3. The erase size puts constraints on the regions / partions. Their addresses need to be aligned with the erase size and their length must be a multiple of the erase size
  1. Some unit tests (testsuite flashdev01) fail as expected, but not due to expected reason. Take for example line 148 in testsuites/libtests/flashdev01/init.c and change

read_data = fgets(buff, 2048, file);

to

read_data = fgets(buff, 1, file);

Actually, the test case is expected to fail as the sector size is only 512 bytes and we try to read 2048 bytes. Unfortunatelly, reading a single byte doesnt work either. This is due to the fact that by default buffered IO is used. On my side, each fgets translates to one or more read operations of 1024 bytes length. I.e. this testcase will always succeed as we expect the test to fail.

  1. I like to propose that we initialise flashdev to operate in buffered mode using RTEMS_FLASHDEV_IOCTL_GET_MIN_WRITE_SIZE.
  2. I think we should add a raw_read and raw_write to be able to transfer larger junks of data.

I can work on this and start providing patches for review.



Change History (5)

comment:1 Changed on 12/28/23 at 16:03:10 by Bernd Moessner

Owner: set to Bernd Moessner
Status: newassigned

comment:2 Changed on 12/28/23 at 16:11:14 by Bernd Moessner

6b / 8a) Of course I thought of using the return values of RTEMS_FLASHDEV_IOCTL_GET_MIN_WRITE_SIZE for alignment / size checks

comment:3 Changed on 01/16/24 at 16:58:26 by Bernd Moessner <berndmoessner80@…>

In [changeset:"a43163d058bd8d9da901e7a7e48591e2ff6aaabe/rtems" a43163d0/rtems]:

flashdev.h: Add missing C++ include guards

Updates #4981

comment:4 Changed on 01/16/24 at 16:58:29 by Bernd Moessner <berndmoessner80@…>

In [changeset:"bd898b503fd0d250b11cbffddc041e1e29e3fc7e/rtems" bd898b50/rtems]:

flashdev: Add missing default case

Updates #4981

comment:5 Changed on 01/16/24 at 16:58:32 by Bernd Moessner <berndmoessner80@…>

In [changeset:"a73b52d6a469c330d21e02ea8f82a05edce9fb73/rtems" a73b52d/rtems]:

flashdev.c: return error if both buffers are NULL

Updates #4981

Note: See TracTickets for help on using tickets.