<meta http-equiv="Content-Type" content="text/html; charset=utf-8"><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 4, 2024 at 2:44 PM Bernd Moessner <<a href="mailto:berndmoessner80@gmail.com">berndmoessner80@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>
<div>
On 04.01.2024 20:40, Kinsey Moore wrote:<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Jan 4, 2024 at
12:34 PM <<a href="mailto:berndmoessner80@gmail.com" target="_blank">berndmoessner80@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From:
Bernd Moessner <<a href="mailto:berndmoessner80@gmail.com" target="_blank">berndmoessner80@gmail.com</a>><br>
<br>
Dear all,<br>
<br>
as outlined in <a href="https://devel.rtems.org/ticket/4981" rel="noreferrer" target="_blank">https://devel.rtems.org/ticket/4981</a>
I`d like to<br>
overwork the flashdev API. The provided patch series should
at<br>
least serve as a fundament for the discussion.<br>
<br>
It includes more, but h:<br>
<br>
1) Bugfixes<br>
<br>
1.1) missing c++ include guards<br>
<br>
1.2) missing default cases<br>
<br>
1.3) There are some false positive test cases. Ie. they
fail,<br>
are expected to fail, but fail for a different reason.<br>
The test cases are not taking into account that fputs works<br>
on buffered IO per default. The region mechanism is there to<br>
protect reads / writes to an area outside the<br>
currently active region. This part works fine, but due to<br>
buffering newlib tries to read even more bytes than the user<br>
requested - which in turn triggers the region mechanism and<br>
causes the read to fail. In addtion to that, theres a bug in<br>
the _update_and_return function due to which a wrong address<br>
(the absolute address and not the relative address of the<br>
region) is returned to newlib.<br>
<br>
2) Refactoring / Style Changes:<br>
<br>
2.1) Not sure if there are naming conventions for IOCTL
defines<br>
and functions, but I think it makes sense if an IOCTL is
used<br>
to "get" a value, that "get" is reflected by the macro and<br>
function names.<br>
<br>
2.2) Flashdev used the term "regions", but from my point of<br>
view "partions" fits better<br>
<br>
2.3) I'd like to call the minimum write block size simply<br>
minimum write size. The point is that the term block is
already<br>
predefined for flash devices (Bytes => Pages =>
Sectors => Block<br>
=> Die => Chip). We should should also make sure that
the<br>
writes are aligned to the min write size and are carried out<br>
with this size. I've added alignment checks, but the user<br>
must set the buffering size for newlib using setvbuf.<br>
<br>
3) API Extension<br>
<br>
3.1) The API is missing a function to get the erase size.<br>
Partions / Regions need to be aligned to the erase size of<br>
the flash memory.<br>
<br>
3.2) The region / partition mechnism works differently now.<br>
There are IOCTL the create, delete, activate, deactivate<br>
partitions, and one to get idx of the currently active<br>
partion. I think the internal implementation becomes<br>
much simpler if we reduce the number of allowed partitions<br>
from 32 to 16.<br>
<br>
The patch set addresses all issues I have found. I have<br>
updated and extended the test cases. I must admit that I am<br>
a bit insecure wrt. to the RTEMS style guide - I`d be very<br>
happy if one could have a close look at that. Aaron Nyholm
has<br>
added the flashdev API last year. The changes within this<br>
patch set are too large to contribute without his agreement<br>
</blockquote>
<div><br>
</div>
<div>Thanks for all the work that went into this!</div>
<div><br>
</div>
<div>
<div>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.</div>
<div><br>
</div>
<div>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.<br>
</div>
<div><br>
</div>
<div>Kinsey</div>
</div>
</div>
</div>
</blockquote>
<br>
<p>Thanks a lot for going through all the commits and commenting on
them!</p>
<p>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? <br></p></div></blockquote><div>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.</div><div><br></div><div>Kinsey<br></div></div></div>