[RFC] Device: generalizing and improving control features (sync, lock and timeout)

include/device.h

@@ -81,6 +81,28 @@ extern "C" {
 	DEVICE_AND_API_INIT(dev_name, drv_name, init_fn,		\
 			    data, cfg_info, level, prio, NULL)
 
+/* Initialize structure device_context */
+#if defined(CONFIG_DEVICE_CONCURRENT_ACCESS)
+#define DEVICE_CONTEXT_INITIALIZE(_obj)				\
+	{							\
+		.sync = Z_SEM_INITIALIZER(_obj.sync, 0, 1),	\
+		.lock = Z_SEM_INITIALIZER(_obj.lock, 1, 1),	\
+	}
+#else
+#define DEVICE_CONTEXT_INITIALIZE(_obj)				\
+	{							\
+		.sync = Z_SEM_INITIALIZER(_obj.sync, 0, 1)	\
+	}
+#endif
+
+#define DEVICE_CONTEXT_DEFINE(dev_name, level, prio)			\
+	static Z_DECL_ALIGN(struct device_context)			\
+		_CONCAT(__device_context_, dev_name) __used		\
+	__attribute__(							\
+		(__section__(".device_context_" #level STRINGIFY(prio)))) = \
+		DEVICE_CONTEXT_INITIALIZE(_CONCAT(__device_context_,	\
+						  dev_name))		\
+
 
 /**
  * @def DEVICE_AND_API_INIT
@@ -97,6 +119,7 @@ extern "C" {
 #ifndef CONFIG_DEVICE_POWER_MANAGEMENT
 #define DEVICE_AND_API_INIT(dev_name, drv_name, init_fn, data, cfg_info, \
 			    level, prio, api)				\
+	DEVICE_CONTEXT_DEFINE(dev_name, level, prio);			\
 	static Z_DECL_ALIGN(struct device)				\
 		_CONCAT(__device_, dev_name) __used			\
 	__attribute__((__section__(".device_" #level STRINGIFY(prio)))) = { \
@@ -151,6 +174,7 @@ extern "C" {
 			K_POLL_MODE_NOTIFY_ONLY,			\
 			&_CONCAT(__pm_, dev_name).signal),		\
 	};								\
+	DEVICE_CONTEXT_DEFINE(dev_name, level, prio);			\
 	static Z_DECL_ALIGN(struct device)				\
 		_CONCAT(__device_, dev_name) __used			\
 	__attribute__((__section__(".device_" #level STRINGIFY(prio)))) = { \
@@ -255,13 +279,84 @@ struct device {
 	const void *config_info;
 	const void *driver_api;
 	void * const driver_data;
+#ifdef CONFIG_DEVICE_CALL_TIMEOUT
+	void (*cancel)(struct device *dev);
+#endif
 #ifdef CONFIG_DEVICE_POWER_MANAGEMENT
 	int (*device_pm_control)(struct device *device, u32_t command,
 				 void *context, device_pm_cb cb, void *arg);
 	struct device_pm * const pm;
 #endif
 };
 
+/**
+ * @brief Runtime device context structure per-driver instance
+ *
+ * @param sync A semaphore used for API call synchronization
+ * @param call_status The status to return from the call
+ * @param lock A semaphore used to protect against concurrent access
+ * @param timeout A timeout for the on-going call
+ */
+struct device_context {
+	struct k_sem sync;
+	int call_status;
+#ifdef CONFIG_DEVICE_CONCURRENT_ACCESS
+	struct k_sem lock;
+#endif
+#ifdef CONFIG_DEVICE_CALL_TIMEOUT
+	k_timeout_t timeout;
+#endif
+};
+
+#if CONFIG_DEVICE_CALL_TIMEOUT_VALUE == -1
+#define DEVICE_CALL_TIMEOUT K_FOREVER
+#else
+#define DEVICE_CALL_TIMEOUT K_MSEC(CONFIG_DEVICE_CALL_TIMEOUT_VALUE)
+#endif
+
+/**
+ * @brief Lock a device structure to avoid concurrent access
+ *
+ * @param dev A valid pointer on a struct device instance
+ * @param timeout Waiting period to take the semaphore,
+ *                or one of the special values K_NO_WAIT and K_FOREVER.
+ *
+ * @return 0 if device got locked, a negative errno otherwise.
+ */
+int device_lock_timeout(struct device *dev, k_timeout_t timeout);
+
+/**
+ * @brief Lock an interrupt based device structure to avoid concurrent access.
+ *
+ * @param sync A semaphore used for API call synchronization
+ * @param call_status The status to return from the call
+ * @param dev A valid pointer on a struct device instance
+ *
+ * @return 0 if device got locked, a negative errno otherwise.
+ */
+static inline int device_lock(struct device *dev)
+{
+	return device_lock_timeout(dev, DEVICE_CALL_TIMEOUT);
+}
+
+/**
+ * @brief Notify when a call is finished
+ *
+ * @param dev A valid pointer on a struct device instance
+ * @param status The status of the device, 0 on success or a negative errno
+ *               otherwise. This function is ISR ready.
+ */
+void device_call_complete(struct device *dev, int status);
+
+/**
+ * @brief Release a previously locked device
+ *
+ * @param dev A valid pointer on a struct device instance.
+ *
+ * @return status of the device
+ */
+int device_release(struct device *dev);
+
 /**
  * @brief Retrieve the device structure for a driver by name
  *

include/linker/linker-defs.h

@@ -88,6 +88,16 @@
 		KEEP(*(SORT(.init_[_A-Z0-9]*)))	\
 
 
+#define DEVICE_CONTEXT_SECTIONS()				\
+		__device_context_start = .;			\
+		CREATE_OBJ_LEVEL(device_context, PRE_KERNEL_1)	\
+		CREATE_OBJ_LEVEL(device_context, PRE_KERNEL_2)	\
+		CREATE_OBJ_LEVEL(device_context, POST_KERNEL)	\
+		CREATE_OBJ_LEVEL(device_context, APPLICATION)	\
+		CREATE_OBJ_LEVEL(device_context, SMP)		\
+		__device_context_end = .;
+
+
 /*
  * link in devices objects, which are tied to the init ones;
  * the objects are thus sorted the same way as their init object parent
@@ -101,7 +111,8 @@
 		CREATE_OBJ_LEVEL(device, APPLICATION)	\
 		CREATE_OBJ_LEVEL(device, SMP)		\
 		__device_end = .;			\
-		DEVICE_BUSY_BITFIELD()			\
+		DEVICE_BUSY_BITFIELD();			\
+		DEVICE_CONTEXT_SECTIONS()		\
 
 
 /*

kernel/Kconfig

@@ -785,6 +785,8 @@ config TICKLESS_KERNEL
 	  This option enables a fully event driven kernel. Periodic system
 	  clock interrupt generation would be stopped at all times.
 
+source "kernel/Kconfig.device"
+
 source "kernel/Kconfig.power_mgmt"
 
 endmenu

kernel/Kconfig.device

@@ -0,0 +1,39 @@
+# Device configuration options
+
+# Copyright (c) 2020 Intel Corporation
+# SPDX-License-Identifier: Apache-2.0
+
+menu "Device driver generic options"
+
+config DEVICE_CONCURRENT_ACCESS
+	bool "Enable concurrent access"
+	default y
+	help
+	  If disabled, devices will not be protected over concurrent access.
+	  This can be a valid possibility if you know your devices will not be
+	  accessed by 2+ threads, and thus will reduce the memory footprint.
+
+config DEVICE_CALL_TIMEOUT
+	bool "Have a timeout on device calls"
+	help
+	  If enabled, this will insert a timeout on device calls. Either
+	  a generic one via DEVICE_CALL_TIMEOUT_VALUE or a dedicated one.
+	  This is disabled by defautl to follow the legacy behavior.
+
+config DEVICE_NO_LOCK_TIMEOUT
+	bool "No locking timeout"
+	depends on DEVICE_CONCURRENT_ACCESS && DEVICE_CALL_TIMEOUT
+	default y
+	help
+	  If enabled, no timeout will ever be applied on locking a device.
+	  This is the default to follow the legacy behavior.
+
+config DEVICE_CALL_TIMEOUT_VALUE
+	int "The default device call timeout"
+	default -1
+	help
+	  This will set the generic timeout for device locking and syncing.
+	  By default -1 is set (K_FOREVER), any positive value is valid.
+	  Make sure to select a suitable value for all the devices.
+
+endmenu

kernel/device.c

@@ -23,6 +23,8 @@ extern const struct init_entry __init_SMP_start[];
 extern struct device __device_start[];
 extern struct device __device_end[];
 
+extern struct device_context __device_context_start[];
+
 #ifdef CONFIG_DEVICE_POWER_MANAGEMENT
 extern u32_t __device_busy_start[];
 extern u32_t __device_busy_end[];
@@ -177,3 +179,116 @@ void device_busy_clear(struct device *busy_dev)
 	ARG_UNUSED(busy_dev);
 #endif
 }
+
+#ifdef CONFIG_DEVICE_CALL_TIMEOUT
+static void device_call_set_timeout(struct device *dev, k_timeout_t timeout)
+{
+	struct device_context *dc =
+		(struct device_context *)__device_context_start +
+		(dev - __device_start);
+
+	dc->timeout = timeout;
+}
+
+static void device_call_cancel(struct device *dev)
+{
+	if (dev->cancel != NULL) {
+		dev->cancel(dev);
+	}
+}
+#else
+#define device_call_set_timeout(...)
+#define device_call_cancel(...)
+#endif /* CONFIG_DEVICE_CALL_TIMEOUT */
+
+int device_lock_timeout(struct device *dev, k_timeout_t timeout)
+{
+#ifdef CONFIG_DEVICE_CONCURRENT_ACCESS
+	struct device_context *dc =
+		(struct device_context *)__device_context_start +
+		(dev - __device_start);
+	k_timeout_t lock_timeout = timeout;
+
+	if (k_is_in_isr()) {
+		return 0;
+	}
+
+
+#ifdef CONFIG_DEVICE_CALL_TIMEOUT
+#ifdef CONFIG_DEVICE_NO_LOCK_TIMEOUT
+	lock_timeout = K_FOREVER;
+#else
+	u32_t time_spent;
+
+	if (!K_TIMEOUT_EQ(timeout, K_FOREVER)) {
+		time_spent = k_uptime_get_32();
+ 	}
+#endif /* CONFIG_DEVICE_NO_LOCK_TIMEOUT */
+#endif /* CONFIG_DEVICE_CALL_TIMEOUT */
+
+	if (k_sem_take(&dc->lock, lock_timeout) != 0) {
+		return -EAGAIN;
+	}
+
+#if defined(CONFIG_DEVICE_CALL_TIMEOUT) &&	\
+	!defined(CONFIG_DEVICE_NO_LOCK_TIMEOUT)
+
+	if (!K_TIMEOUT_EQ(timeout, K_FOREVER)) {
+		if (k_uptime_get_32() < time_spent) {
+			time_spent = k_uptime_get_32() +
+				(UINT32_MAX - time_spent);
+		} else {
+			time_spent = k_uptime_get_32() + time_spent;
+		}
+
+		timeout = K_MSEC(k_ticks_to_ms_floor32(tiemout) - time_spent);
+	}
+#endif /* CONFIG_DEVICE_CALL_TIMEOUT */
+
+#endif /* CONFIG_DEVICE_CONCURRENT_ACCESS */
+
+	device_call_set_timeout(dev, timeout);
+
+	return 0;
+}
+
+void device_call_complete(struct device *dev, int status)
+{
+	struct device_context *dc =
+		(struct device_context *)__device_context_start +
+		(dev - __device_start);
+
+	dc->call_status = status;
+
+	if (!k_is_in_isr()) {
+		k_sem_give(&dc->sync);
+	}
+}
+
+int device_release(struct device *dev)
+{
+	struct device_context *dc =
+		(struct device_context *)__device_context_start +
+		(dev - __device_start);
+	u32_t status;
+
+	if (!k_is_in_isr()) {
+#ifdef CONFIG_DEVICE_CALL_TIMEOUT
+		if (k_sem_take(&dc->sync, dc->timeout) == -EAGAIN) {
+			device_call_cancel(dev);
+			status = -ECANCELED;
+		}
+#else
+		k_sem_take(&dc->sync, K_FOREVER);
+#endif /* CONFIG_DEVICE_CALL_TIMEOUT */
+	}
+
+	status = dc->call_status;
+
+#ifdef CONFIG_DEVICE_CONCURRENT_ACCESS
+	if (!k_is_in_isr()) {
+		k_sem_give(&dc->lock);
+	}
+#endif
+	return status;
+}

tests/kernel/device/control/CMakeLists.txt

@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.13.1)
+find_package(Zephyr HINTS $ENV{ZEPHYR_BASE})
+project(device_control)
+
+FILE(GLOB app_sources src/*.c)
+target_sources(app PRIVATE ${app_sources})

tests/kernel/device/control/prj.conf

@@ -0,0 +1 @@
+CONFIG_ZTEST=y

tests/kernel/device/control/src/fake_api.h

@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2020 Intel Corporation
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include <device.h>
+
+#define FAKE_DRV_NAME "FAKE_DRV"
+
+struct fake_api {
+	int (*sync_int_call)(struct device *dev);
+	int (*sync_poll_call)(struct device *dev);
+	int (*lock_int_call)(struct device *dev, int *val);
+	int (*lock_poll_call)(struct device *dev, int *val);
+	int (*no_to_call)(struct device *dev, int *val);
+
+};
+
+static inline int fake_sync_int_call(struct device *dev)
+{
+	const struct fake_api *api = (const struct fake_api *)dev->driver_api;
+
+	return api->sync_int_call(dev);
+}
+
+static inline int fake_sync_poll_call(struct device *dev)
+{
+	const struct fake_api *api = (const struct fake_api *)dev->driver_api;
+
+	return api->sync_poll_call(dev);
+}
+
+static inline int fake_lock_int_call(struct device *dev, int *val)
+{
+	const struct fake_api *api = (const struct fake_api *)dev->driver_api;
+
+	return api->lock_int_call(dev, val);
+}
+
+static inline int fake_lock_poll_call(struct device *dev, int *val)
+{
+	const struct fake_api *api = (const struct fake_api *)dev->driver_api;
+
+	return api->lock_poll_call(dev, val);
+}
+
+static inline int fake_no_timeout_call(struct device *dev, int *val)
+{
+	const struct fake_api *api = (const struct fake_api *)dev->driver_api;
+
+	return api->no_to_call(dev, val);
+}