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

[Kinsey Moore]

On Thu, Jan 4, 2024 at 2:44 PM Bernd Moessner <berndmoessner80 at gmail.com>
wrote:

> On 04.01.2024 20:40, Kinsey Moore wrote:
>
> 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
>
>
>
> Thanks a lot for going through all the commits and commenting on them!
>
> Yes, you've chosen the correct patch set. I`ll work in your comments and
> resubmit the patch set after we've clarified if it is "min_write_size",
> "min_write_block_size" or something entirely new ;). Therefore, the patch
> set will become smaller as two patches fix bugs I've introduced. Do I
> resubmit it using "[PATCH rtems6 - v1 00/16] Overwork flashdev - V1" or as
> V2?
>
The easiest way to do it is just to specify the -v2 flag on the git
send-email line. There's no need to specify v2 again in the 00 subject line
since it should already be there inside [PATCH v2 00/16]. Joel will likely
also commit some of the pieces, so be ready to omit those by rebasing.

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