[PATCH rtems6 - v1 00/16] Overwork flashdev - V1

[Kinsey Moore]

On Thu, Jan 4, 2024 at 12:34 PM <berndmoessner80 at gmail.com> wrote:

> From: Bernd Moessner <berndmoessner80 at gmail.com>
>
> Dear all,
>
> as outlined in https://devel.rtems.org/ticket/4981 I`d like to
> overwork the flashdev API. The provided patch series should at
> least serve as a fundament for the discussion.
>
> It includes more, but h:
>
> 1) Bugfixes
>
> 1.1) missing c++ include guards
>
> 1.2) missing default cases
>
> 1.3) There are some false positive test cases. Ie. they fail,
> are expected to fail, but fail for a different reason.
> The test cases are not taking into account that fputs works
> on buffered IO per default. The region mechanism is there to
> protect reads / writes to an area outside the
> currently active region. This part works fine, but due to
> buffering newlib tries to read even more bytes than the user
> requested - which in turn triggers the region mechanism and
> causes the read to fail. In addtion to that, theres a bug in
> the _update_and_return function due to which a wrong address
> (the absolute address and not the relative address of the
> region) is returned to newlib.
>
> 2) Refactoring / Style Changes:
>
> 2.1) Not sure if there are naming conventions for IOCTL defines
> and functions, but I think it makes sense if an IOCTL is used
> to "get" a value, that "get" is reflected by the macro and
> function names.
>
> 2.2) Flashdev used the term "regions", but from my point of
> view "partions" fits better
>
> 2.3) I'd like to call the minimum write block size simply
> minimum write size. The point is that the term block is already
> predefined for flash devices (Bytes => Pages => Sectors => Block
> => Die => Chip). We should should also make sure that the
> writes are aligned to the min write size and are carried out
> with this size. I've added alignment checks, but the user
> must set the buffering size for newlib using setvbuf.
>
> 3) API Extension
>
> 3.1) The API is missing a function to get the erase size.
> Partions / Regions need to be aligned to the erase size of
> the flash memory.
>
> 3.2) The region / partition mechnism works differently now.
> There are IOCTL the create, delete, activate, deactivate
> partitions, and one to get idx of the currently active
> partion. I think the internal implementation becomes
> much simpler if we reduce the number of allowed partitions
> from 32 to 16.
>
> The patch set addresses all issues I have found. I have
> updated and extended the test cases. I must admit that I am
> a bit insecure wrt. to the RTEMS style guide - I`d be very
> happy if one could have a close look at that. Aaron Nyholm has
> added the flashdev API last year. The changes within this
> patch set are too large to contribute without his agreement
>

Thanks for all the work that went into this!

I see that there are 2 versions of this patch set on the list. It seems
that one doesn't have a version identifier and one has a v1 identifier.
Either would be fine, but I wouldn't recommend using both as it can get
confusing as to which is the latest version of the patch set.

Hopefully, I've chosen the correct set to review (16 patches).
Unfortunately, it was made harder to distinguish because the mail server
was down for a few days and they both got delivered within a few tens of
minutes of each other.

Kinsey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20240104/bd04c3c9/attachment-0001.htm>