On 04.01.2024 20:40, Kinsey Moore wrote:
On Thu, Jan 4, 2024 at 12:34 PM <berndmoessne...@gmail.com> wrote:
From: Bernd Moessner <berndmoessne...@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?
Bernd
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
