michallenc commented on code in PR #16642:
URL: https://github.com/apache/nuttx/pull/16642#discussion_r2189189335
##########
drivers/mtd/ftl.c:
##########
@@ -478,6 +467,86 @@ static ssize_t ftl_read(FAR struct inode *inode, unsigned
char *buffer,
*
****************************************************************************/
+static int ftl_alloc_eblock(FAR struct ftl_struct_s *dev)
+{
+ if (dev->eblock == NULL)
+ {
+ /* Allocate one, in-memory erase block buffer */
+
+ dev->eblock = kmm_malloc(dev->geo.erasesize);
+ }
+
+ return dev->eblock != NULL ? OK : -ENOMEM;
+}
+
+/****************************************************************************
+ * Name: ftl_flush_direct
+ *
+ * Description: Write the specified number of sectors without cache
+ *
+ ****************************************************************************/
+
+static ssize_t ftl_flush_direct(FAR struct ftl_struct_s *dev,
+ FAR const uint8_t *buffer,
+ off_t startblock, size_t nblocks)
+{
+ size_t blocksize = dev->geo.blocksize;
+ off_t starteraseblock;
+ off_t offset;
+ ssize_t ret;
+ size_t count;
+
+ while (nblocks)
+ {
+ starteraseblock = startblock / dev->blkper;
+ offset = startblock & (dev->blkper - 1);
+ count = MIN(dev->blkper - offset, nblocks);
+
+ if (offset == 0 && dev->mtd->erase != NULL && !(dev->oflags & O_SYNC))
Review Comment:
This logic has to be more complicated. What if user writes two write pages,
one is the last of the erase page and other is at the beginning of the next
erase page. The offset won't be zero, but we should still erase the next erase
page. The same goes if user performs large write across multiple erase pages. I
had something like
```c
int nblocks = ((offset + write_len) / geometry.erasesize) - (offset /
geometry.erasesize) + 1;
if (offset % geometry.erasesize == 0) {
erase.startblock = offset / geometry.erasesize;
erase.nblocks = nblocks;
} else {
erase.startblock = (offset / geometry.erasesize) + 1;
erase.nblocks = nblocks - 1;
}
```
##########
include/nuttx/mtd/mtd.h:
##########
@@ -298,10 +298,25 @@ FAR struct mtd_dev_s *mtd_rwb_initialize(FAR struct
mtd_dev_s *mtd);
* Input Parameters:
* path - The block device path.
* mtd - The MTD device that supports the FLASH interface.
+ * oflags - oflags passed to the ftl layer. Currently, the ftl is affected
+ * by two oflags:
+ * 1. O_DIRECT when this flag is passed in, ftl internally uses
+ * the direct write strategy and no read cache is used in ftl;
+ * otherwise, each write will be executed with the minimum
+ * granularity of flash erase sector size which means a
+ * "sector read back - erase sector - write sector" operation
+ * is performed by using a read cache buffer in heap.
+ *
+ * 2. O_SYNC, when this flag is passed in, we assume that the
+ * flash has been erased in advance and no erasure operation
Review Comment:
```suggestion
* flash has been erased in advance and no erase operation
```
##########
drivers/mtd/ftl.c:
##########
@@ -914,5 +1010,5 @@ int ftl_initialize(int minor, FAR struct mtd_dev_s *mtd)
/* Do the real work by ftl_initialize_by_path */
snprintf(path, DEV_NAME_MAX, "/dev/mtdblock%d", minor);
- return ftl_initialize_by_path(path, mtd);
+ return ftl_initialize_by_path(path, mtd, O_RDWR);
Review Comment:
If oflags will be set by `ftl_initialize_by_path`, it should be possible to
set them with `ftl_initialize` as well.
##########
drivers/mtd/ftl.c:
##########
@@ -789,7 +883,8 @@ static int ftl_unlink(FAR struct inode *inode)
*
****************************************************************************/
-int ftl_initialize_by_path(FAR const char *path, FAR struct mtd_dev_s *mtd)
+int ftl_initialize_by_path(FAR const char *path, FAR struct mtd_dev_s *mtd,
+ int oflags)
Review Comment:
The issue here is this is called during flash initialization and partition
creation. Therefore, you have to set flags there and can't change it from an
application during `open` call. Could we set the `oflags` later based on how is
the partition opened? This would have to be passed from BCH layer, because user
opens that one, not FTL.
Flags `O_DIRECT` and `O_SYNC` are now basically useless from an application,
they have no effect.
##########
drivers/bch/bchdev_register.c:
##########
@@ -50,17 +50,17 @@
****************************************************************************/
int bchdev_register(FAR const char *blkdev, FAR const char *chardev,
- bool readonly)
+ int oflags)
Review Comment:
This change will have breaking impact on current implementations that call
`bchdev_register` function. Maybe we could say the access is `O_RDWR` if
`oflags` is `0`/`false` to keep backwards compatibility?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
