[PATCH 1/2] dev/flash: Add API for Flash devices
Gedare Bloom
gedare at rtems.org
Fri Mar 17 22:16:11 UTC 2023
Some comments below. However, I think it will be best to first re-send
just the flashdev.h file so that we can iterate on the API before you
put too much more effort into implementing it.
On Wed, Mar 15, 2023 at 8:56 PM <aaron.nyholm at unfoldedeffective.com> wrote:
>
> From: Aaron Nyholm <aaron.nyholm at southerninnovation.com>
>
> Updates #4896
I don't see a ticket with this number.
> ---
> cpukit/dev/flash/flashdev.c | 353 ++++++++++++++++++++++++++++
> cpukit/include/dev/flash/flashdev.h | 95 ++++++++
> spec/build/cpukit/librtemscpu.yml | 4 +
> 3 files changed, 452 insertions(+)
> create mode 100644 cpukit/dev/flash/flashdev.c
> create mode 100644 cpukit/include/dev/flash/flashdev.h
>
> diff --git a/cpukit/dev/flash/flashdev.c b/cpukit/dev/flash/flashdev.c
> new file mode 100644
> index 0000000000..a37d0e6c3d
> --- /dev/null
> +++ b/cpukit/dev/flash/flashdev.c
> @@ -0,0 +1,353 @@
> +/*
> + * Copyright (C) 2023, 2023 Aaron Nyholm
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +
only 1 blank horizontal line at a time
> +#include <dev/flash/flashdev.h>
> +
> +#include <rtems/imfs.h>
> +#include <rtems/score/assert.h>
> +
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +int flashdev_do_init(
> + flashdev *flash,
> + void (*destroy)(flashdev *flash)
> +);
> +
> +void flashdev_destroy_and_free(flashdev *flash);
> +
> +void flashdev_destroy(flashdev *flash);
> +
Global function declarations should be in header files. If these are
only needed here, declare static.
> +static void flashdev_obtain(flashdev *flash)
{
linebreak {
> + rtems_recursive_mutex_lock(&flash->mutex);
> +}
> +
> +static void flashdev_release(flashdev *flash) {
> + rtems_recursive_mutex_unlock(&flash->mutex);
> +}
> +
> +static ssize_t flashdev_read(
> + rtems_libio_t *iop,
> + void *buffer,
> + size_t count
> + )
delete leading spaces
> +{
> + flashdev *flash = IMFS_generic_get_context_by_iop(iop);
> +
> + // Check reading from valid region
no // comments
> + off_t new_offset = iop->offset + count;
> +
> + if (iop->data1 != NULL && new_offset > ((flashdev_region*)iop->data1)->length) {
> + rtems_set_errno_and_return_minus_one(EINVAL);
> + }
> +
> + // Read flash
> + off_t addr;
decls at start of function
> + if (iop->data1 == NULL) {
> + addr = iop->offset;
> + } else {
> + addr = (iop->offset + ((flashdev_region*)iop->data1)->base);
> + }
> + flashdev_obtain(flash);
> + int status = (*flash->read)(flash->driver, addr, count, buffer);
> + flashdev_release(flash);
> +
> + // Update offset and return
> + if (status == 0) {
> + iop->offset = new_offset;
> + return count;
> + } else {
> + rtems_set_errno_and_return_minus_one(-status);
why -status. error conditions of this driver framework are not defined.
> + }
> +}
> +
> +static ssize_t flashdev_write(
> + rtems_libio_t *iop,
> + const void *buffer,
> + size_t count
> + )
> +{
> + flashdev *flash = IMFS_generic_get_context_by_iop(iop);
> +
> + // Check writing to valid region
> + off_t new_offset = iop->offset + count;
> +
> + if (iop->data1 != NULL && new_offset > ((flashdev_region*)iop->data1)->length) {
> + rtems_set_errno_and_return_minus_one(EINVAL);
> + }
> +
> + // Write to flash
> + off_t addr;
> + if (iop->data1 == NULL) {
> + addr = iop->offset;
> + } else {
> + addr = (iop->offset + ((flashdev_region*)iop->data1)->base);
> + }
this code identical to above, consider a helper function anytime you
find yourself copy-pasting code.
> + flashdev_obtain(flash);
> + int status = (*flash->write)(flash->driver, addr, count, buffer);
> + flashdev_release(flash);
> +
> + // Update offset and return
> + if (status == 0) {
> + iop->offset = new_offset;
> + return count;
> + } else {
> + rtems_set_errno_and_return_minus_one(-status);
> + }
ditto
> +}
> +
> +static int flashdev_ioctl(
> + rtems_libio_t *iop,
> + ioctl_command_t command,
> + void *arg
> + )
> +{
> + flashdev *flash = IMFS_generic_get_context_by_iop(iop);
> + int err = 0;
> +
> + flashdev_obtain(flash);
> +
> + switch (command) {
> + case FLASHDEV_IOCTL_OBTAIN:
> + flashdev_obtain(flash);
> + err = 0;
> + break;
> + case FLASHDEV_IOCTL_RELEASE:
> + flashdev_release(flash);
> + err = 0;
> + break;
> + case FLASHDEV_IOCTL_READID:
> + uint32_t *readID_out = arg;
decl at start of function, or just...
*((uint32_t*)arg) = (*flash->read_id)(flash);
simplify.
> + *readID_out = (*flash->read_id)(flash);
> + err = 0;
> + break;
> + case FLASHDEV_IOCTL_ERASE:
> + erase_args *erase_args_1 = (erase_args*)arg;
> + // Check erasing valid region
> + off_t check_offset = erase_args_1->offset + erase_args_1->count;
> + if ((iop->data1 != NULL) &&
> + (check_offset > ((flashdev_region*)iop->data1)->length ||
> + (erase_args_1->offset < ((flashdev_region*)iop->data1)->base))
> + ) {
> + rtems_set_errno_and_return_minus_one(EINVAL);
> + }
> + // Erasing
> + off_t addr;
> + if (iop->data1 == NULL) {
> + addr = erase_args_1->offset;
> + } else {
> + addr = (erase_args_1->offset + ((flashdev_region*)iop->data1)->base);
> + }
> + (*flash->erase)(flash->driver, addr, erase_args_1->count);
Write a helper function when your case exceeds a few lines. It is hard
to understand nested conditional logic inside of a switch statement.
Ignoring return value from flash->erase.
> + err = 0;
> + break;
> + case FLASHDEV_IOCTL_REGION_SET:
> + err = 0;
> + flashdev_region *region_in = (flashdev_region*)arg;
> + if (region_in == NULL || region_in->base < 0 || region_in->length < 0) {
> + rtems_set_errno_and_return_minus_one(EINVAL);
> + }
> + flashdev_region *region_save = malloc(sizeof(flashdev_region));
avoid malloc if possible, document its use where necessary. I don't
see why malloc is necessary here.
> + if (region_save == NULL) {
> + rtems_set_errno_and_return_minus_one(ENOMEM);
> + }
> + iop->data1 = region_save;
> + ((flashdev_region*)iop->data1)->base = region_in->base;
> + ((flashdev_region*)iop->data1)->length = region_in->length;
> + iop->offset = region_in->base;
write a function for this case.
> + break;
> + case FLASHDEV_IOCTL_REGION_UNSET:
> + err = 0;
> + free((flashdev_region*)iop->data1);
> + iop->data1 = NULL;
> + break;
> + case FLASHDEV_IOCTL_TYPE:
> + int *type_out = arg;
> + *type_out = (*flash->flash_type)(flash);
> + err = 0;
> + break;
> + }
> +
> + flashdev_release(flash);
> +
> + if (err == 0) {
> + return 0;
> + } else {
> + rtems_set_errno_and_return_minus_one(err);
> + }
> +}
> +
> +
too many blank lines
> +static off_t flashdev_lseek(
> + rtems_libio_t *iop,
> + off_t offset,
> + int whence
> +)
> +{
> + off_t tmp_offset;
> +
> + if (offset < 0) {
> + rtems_set_errno_and_return_minus_one( EINVAL );
> + }
> +
> + switch (whence) {
> + case SEEK_CUR:
> + tmp_offset = iop->offset + offset;
> + break;
> + case SEEK_SET:
> + tmp_offset = offset;
> + break;
> + case SEEK_END:
> + default:
> + rtems_set_errno_and_return_minus_one( EINVAL );
> + }
> +
> + if (iop->data1 != NULL &&
> + (tmp_offset > ((flashdev_region*)iop->data1)->length)) {
> + rtems_set_errno_and_return_minus_one( EINVAL );
> + }
> +
> + iop->offset = tmp_offset;
> + return iop->offset;
> +}
> +
> +static int flashdev_close(rtems_libio_t *iop) {
> + free((flashdev_region*)iop->data1);
Need to confirm iop is open before you close. Should check something
to avoid double-free or other bad free. I don't see that open()
mallocs, so I don't know what you're freeing. at least, you should
check if iop->data1 != NULL before you free, and then set it to NULL
after you free it. This is good defense programming practice.
> + return rtems_filesystem_default_close(iop);
> +}
> +
> +static int flashdev_open(rtems_libio_t *iop, const char *path, int oflag, mode_t mode) {
> + int ret = rtems_filesystem_default_open(iop, path, oflag, mode);
> + iop->data1 = NULL;
> + return ret;
> +}
> +
> +
> +static const rtems_filesystem_file_handlers_r flashdev_handler = {
> + .open_h = flashdev_open,
> + .close_h = flashdev_close,
> + .read_h = flashdev_read,
> + .write_h = flashdev_write,
> + .ioctl_h = flashdev_ioctl,
> + .lseek_h = flashdev_lseek,
> + .fstat_h = IMFS_stat,
> + .ftruncate_h = rtems_filesystem_default_ftruncate,
> + .fsync_h = rtems_filesystem_default_fsync_or_fdatasync,
> + .fdatasync_h = rtems_filesystem_default_fsync_or_fdatasync,
> + .fcntl_h = rtems_filesystem_default_fcntl,
> + .kqfilter_h = rtems_filesystem_default_kqfilter,
> + .mmap_h = rtems_filesystem_default_mmap,
> + .poll_h = rtems_filesystem_default_poll,
> + .readv_h = rtems_filesystem_default_readv,
> + .writev_h = rtems_filesystem_default_writev
> +};
> +
> +static void flashdev_node_destroy(IMFS_jnode_t *node) {
> + flashdev *flash;
> +
> + flash = IMFS_generic_get_context_by_node(node);
> +
error handling?
> + (*flash->destroy)(flash);
> +
> + IMFS_node_destroy_default(node);
> +
> +}
> +
> +static const IMFS_node_control flashdev_node_control = IMFS_GENERIC_INITIALIZER(
> + &flashdev_handler,
> + IMFS_node_initialize_generic,
> + flashdev_node_destroy
> +);
> +
> +int flashdev_register(
> + flashdev *flash,
> + const char *flash_path
> +)
{
linebreak
> + int rv;
> +
> + rv = IMFS_make_generic_node(
> + flash_path,
> + S_IFCHR | S_IRWXU | S_IRWXG | S_IRWXO,
> + &flashdev_node_control,
> + flash
> + );
> + if (rv != 0) {
> + (*flash->destroy)(flash);
> + }
> +
> + return rv;
> +}
> +
> +int flashdev_do_init(
static
> + flashdev *flash,
> + void (*destroy)(flashdev *flash)
> +) {
> + rtems_recursive_mutex_init(&flash->mutex, "FLASHDEV Flash");
Is it possible to have multiple flash drivers in one system? If so,
this name should not be globally unique.
> + flash->destroy = destroy;
> + return 0;
> +}
> +
> +void flashdev_destroy(flashdev *flash) {
> + rtems_recursive_mutex_destroy(&flash->mutex);
> +}
> +
> +void flashdev_destroy_and_free(flashdev *flash) {
> + rtems_recursive_mutex_destroy(&flash->mutex);
> + free(flash);
> +}
> +
> +int flashdev_init(flashdev *flash) {
> + memset(flash, 0, sizeof(*flash));
> +
> + return flashdev_do_init(flash, flashdev_destroy);
> +}
> +
> +flashdev* flashdev_alloc_and_init(size_t size) {
> +
> + flashdev *flash = NULL;
> +
> + if (size >= sizeof(*flash)) {
> + flash = calloc(1, size);
> + if (flash != NULL) {
> + int rv;
> +
> + rv = flashdev_do_init(flash, flashdev_destroy_and_free);
> + if (rv != 0) {
> + free(flash);
> + return NULL;
> + }
> + }
> + }
> +
> + return flash;
> +
> +}
> diff --git a/cpukit/include/dev/flash/flashdev.h b/cpukit/include/dev/flash/flashdev.h
> new file mode 100644
> index 0000000000..b6869629c4
> --- /dev/null
> +++ b/cpukit/include/dev/flash/flashdev.h
> @@ -0,0 +1,95 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup FLASHDEV_Flash
> + *
> + * @brief FLASHDEV flashdev API
This doxygen needs improvement and made a proper group with good descriptions.
> + */
> +
> +/*
> + * Copyright (C) 2023, 2023 Aaron Nyholm
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _DEV_FLASHDEV_H
> +#define _DEV_FLASHDEV_H
> +
> +#include <rtems/thread.h>
> +#include <sys/types.h>
> +#include <stdbool.h>
> +
Please add doxygen documentation for this header file suitable for API usage.
> +typedef struct flashdev flashdev;
> +
Since RTEMS is monolithically compiled, it is important to avoid
polluting the global namespace. For an RTEMS-specific API, it would be
good to scope the function names of the API better. Maybe with
`rtems_flashdev...` or similar. You might mimic for now the API
functions/names on the Zephyr framework as a guide. The i2c/spi driver
frameworks in cpukit/dev are not prepended with rtems because they are
ported from elsewhere, so their interfaces need to match the upstream.
> +typedef struct erase_args {
> + uint32_t offset;
> + uint32_t count;
> +} erase_args;
All the globally-visible symbols need to be consistently named. so,
rtems_flashdev_erase_args might be better here. Although, whether one
really needs a struct for this is unclear to me, and this struct is
not used in the global interface so I'm not sure it needs to be
defined here. Think about encapsulation as you design APIs.
> +
> +typedef struct flashdev_region {
> + off_t base;
> + off_t length;
> +} flashdev_region;
ditto.
> +
> +struct flashdev {
should be rtems_flashdev.
> +
> + int (*read)(void *driver, uint32_t offset, uint32_t count, void *buffer);
If you know the type for *driver, please use it. Typically, the first
arg in a driver framework would be struct flashdev*. Then the function
would get the driver out of the context structure (flashdev->driver).
> +
> + int (*write)(void *driver, uint32_t offset, uint32_t count, const void *buffer);
> +
> + int (*erase)(void *driver, uint32_t offset, uint32_t count);
> +
> + uint32_t (*read_id)(flashdev *flashdev);
> +
> + int (*flash_type)(flashdev *flashdev);
> +
> + void (*destroy)(flashdev *flashdev);
> +
> + void *driver;
> +
> + rtems_recursive_mutex mutex;
> +
> +};
> +
> +#define FLASHDEV_IOCTL_OBTAIN 0
RTEMS_FLASHDEV_...
> +#define FLASHDEV_IOCTL_RELEASE 1
> +#define FLASHDEV_IOCTL_READID 2
> +#define FLASHDEV_IOCTL_ERASE 3
> +#define FLASHDEV_IOCTL_REGION_SET 4
> +#define FLASHDEV_IOCTL_REGION_UNSET 5
> +#define FLASHDEV_IOCTL_TYPE 6
> +
> +#define FLASHDEV_TYPE_NOR 0
> +#define FLASHDEV_TYPE_NAND 1
> +
> +flashdev* flashdev_alloc_and_init(size_t size);
Avoid mixing the API function declarations with macros. Keep the
structure of the file clean. You might consolidate all the macros in
one place.
> +
> +int flashdev_init(flashdev *flashdev);
> +
> +int flashdev_register(
> + flashdev *flashdev,
> + const char *flashdev_path
> +);
> +
> +#endif /* _DEV_FLASHDEV_H */
> diff --git a/spec/build/cpukit/librtemscpu.yml b/spec/build/cpukit/librtemscpu.yml
> index 2c969aa5f6..86c5c8ff2b 100644
> --- a/spec/build/cpukit/librtemscpu.yml
> +++ b/spec/build/cpukit/librtemscpu.yml
> @@ -52,6 +52,9 @@ install:
> - destination: ${BSP_INCLUDEDIR}/dev/spi
> source:
> - cpukit/include/dev/spi/spi.h
> +- destination: ${BSP_INCLUDEDIR}/dev/flash
> + source:
> + - cpukit/include/dev/flash/flashdev.h
> - destination: ${BSP_INCLUDEDIR}/dev/can
> source:
> - cpukit/include/dev/can/can.h
> @@ -522,6 +525,7 @@ links:
> - role: build-dependency
> uid: vckey
> source:
> +- cpukit/dev/flash/flashdev.c
> - cpukit/dev/i2c/eeprom.c
> - cpukit/dev/i2c/fpga-i2c-slave.c
> - cpukit/dev/i2c/gpio-nxp-pca9535.c
> --
> 2.25.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list