On Thu, Jan 4, 2024 at 2:44 PM Bernd Moessner <berndmoessne...@gmail.com> wrote:
> 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? > 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
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel