ipc: rpmsg_multi_instance: Rework instance tracking

This patch is the first step to make the rpmsg_multi_instance usable in
a multi-core scenario.

The current driver is using a local driver variable (instance) to track
the number of allocated instances. This counter is practically used to
allocate to the instance the correct portion of the shared memory.

This is fundamentally wrong because this is assuming that it does exist
only one single shared memory region to split amongs all the allocated
instances.  When the platform has more than one core this is obviously
not the case since each couple of cores are communicating using a
different memory region.

To solve this issue we introduce a new struct rpmsg_mi_ctx_shm_cfg that
is doing two things: (1) it's carrying the information about the shared
memory and (2) it's carrying an internal variable used to track the
instances allocated in that region. The same struct should be used every
time a new instance is allocated in the same shared memory region.

We also fix a problem with the current code where there is a race
between threads when accessing the instance variable, so this patch is
adding a serializing mutex.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>

include/ipc/rpmsg_multi_instance.h

@@ -117,18 +117,23 @@ struct rpmsg_mi_ctx {
 	sys_slist_t endpoints;
 };
 
+struct rpmsg_mi_ctx_shm_cfg {
+	/** Physical address shared memory region. */
+	uintptr_t addr;
+
+	/** Size shared memory region. */
+	size_t size;
+
+	/** Internal counter. */
+	unsigned int instance;
+};
+
 /** @brief Configuration of the RPMsg instance. */
 struct rpmsg_mi_ctx_cfg {
 
 	/** Name of instance. */
 	const char *name;
 
-	/** Physical address shared memory region. */
-	uintptr_t shm_addr;
-
-	/** Size shared memory region. */
-	size_t shm_size;
-
 	/** Stack area for k_work_q. */
 	k_thread_stack_t *ipm_stack_area;
 
@@ -149,6 +154,9 @@ struct rpmsg_mi_ctx_cfg {
 
 	/** IPM message identifier. */
 	unsigned int ipm_tx_id;
+
+	/** SHM struct. */
+	struct rpmsg_mi_ctx_shm_cfg *shm;
 };
 
 /** @brief Initialization of RPMsg instance.

samples/subsys/ipc/rpmsg_multi_instance/remote/src/main.c

@@ -66,6 +66,11 @@ static void received2_cb(const void *data, size_t len, void *priv)
 	k_sem_give(&data_rx2_sem);
 }
 
+static struct rpmsg_mi_ctx_shm_cfg shm = {
+	.addr	= SHM_START_ADDR,
+	.size	= SHM_SIZE,
+};
+
 static const struct rpmsg_mi_ctx_cfg cfg_1 = {
 	.name               = "instance 1",
 	.ipm_stack_area     = ipm_stack_area_1,
@@ -75,8 +80,7 @@ static const struct rpmsg_mi_ctx_cfg cfg_1 = {
 	.ipm_tx_name        = CONFIG_RPMSG_MULTI_INSTANCE_0_IPM_TX_NAME,
 	.ipm_rx_name        = CONFIG_RPMSG_MULTI_INSTANCE_0_IPM_RX_NAME,
 	.ipm_tx_id          = IPM_MSG_ID,
-	.shm_addr           = SHM_START_ADDR,
+	.shm                = &shm,
-	.shm_size           = SHM_SIZE,
 };
 
 static const struct rpmsg_mi_ctx_cfg cfg_2 = {
@@ -88,8 +92,7 @@ static const struct rpmsg_mi_ctx_cfg cfg_2 = {
 	.ipm_tx_name        = CONFIG_RPMSG_MULTI_INSTANCE_1_IPM_TX_NAME,
 	.ipm_rx_name        = CONFIG_RPMSG_MULTI_INSTANCE_1_IPM_RX_NAME,
 	.ipm_tx_id          = IPM_MSG_ID,
-	.shm_addr           = SHM_START_ADDR,
+	.shm                = &shm,
-	.shm_size           = SHM_SIZE,
 };
 
 static struct rpmsg_mi_cb cb_1 = {

samples/subsys/ipc/rpmsg_multi_instance/src/main.c

@@ -66,6 +66,11 @@ static void received2_cb(const void *data, size_t len, void *priv)
 	k_sem_give(&data_rx2_sem);
 }
 
+static struct rpmsg_mi_ctx_shm_cfg shm = {
+	.addr	= SHM_START_ADDR,
+	.size	= SHM_SIZE,
+};
+
 static const struct rpmsg_mi_ctx_cfg cfg_1 = {
 	.name               = "instance 1",
 	.ipm_stack_area     = ipm_stack_area_1,
@@ -75,8 +80,7 @@ static const struct rpmsg_mi_ctx_cfg cfg_1 = {
 	.ipm_tx_name        = CONFIG_RPMSG_MULTI_INSTANCE_0_IPM_TX_NAME,
 	.ipm_rx_name        = CONFIG_RPMSG_MULTI_INSTANCE_0_IPM_RX_NAME,
 	.ipm_tx_id          = IPM_MSG_ID,
-	.shm_addr           = SHM_START_ADDR,
+	.shm                = &shm,
-	.shm_size           = SHM_SIZE,
 };
 
 static const struct rpmsg_mi_ctx_cfg cfg_2 = {
@@ -88,8 +92,7 @@ static const struct rpmsg_mi_ctx_cfg cfg_2 = {
 	.ipm_tx_name        = CONFIG_RPMSG_MULTI_INSTANCE_1_IPM_TX_NAME,
 	.ipm_rx_name        = CONFIG_RPMSG_MULTI_INSTANCE_1_IPM_RX_NAME,
 	.ipm_tx_id          = IPM_MSG_ID,
-	.shm_addr           = SHM_START_ADDR,
+	.shm                = &shm,
-	.shm_size           = SHM_SIZE,
 };
 
 static struct rpmsg_mi_cb cb_1 = {

subsys/ipc/ipc_service/backends/ipc_rpmsg_multi_instance.c

@@ -56,6 +56,11 @@ struct ipc_rpmsg_mi_instances {
 
 static struct ipc_rpmsg_mi_instances instances[NUM_INSTANCES];
 
+static struct rpmsg_mi_ctx_shm_cfg shm = {
+	.addr   = SHM_START_ADDR,
+	.size   = SHM_SIZE,
+};
+
 static void common_bound_cb(void *priv)
 {
 	struct ipc_ept *ept = (struct ipc_ept *)priv;
@@ -139,8 +144,7 @@ static int register_ept(struct ipc_ept **ept, const struct ipc_ept_cfg *cfg)
 		ctx_cfg.ipm_tx_name = ipm_tx_name[i_idx];
 		ctx_cfg.ipm_tx_id = IPM_MSG_ID;
 
-		ctx_cfg.shm_addr = SHM_START_ADDR;
+		ctx_cfg.shm = &shm;
-		ctx_cfg.shm_size = SHM_SIZE;
 
 		if (rpmsg_mi_ctx_init(&instances[i_idx].ctx, &ctx_cfg) < 0) {
 			LOG_ERR("Instance initialization failed");

subsys/ipc/rpmsg_multi_instance/rpmsg_multi_instance.c

@@ -17,7 +17,7 @@
 
 LOG_MODULE_REGISTER(rpmsg_multi_instance, CONFIG_RPMSG_MULTI_INSTANCE_LOG_LEVEL);
 
-static int instance;
+K_MUTEX_DEFINE(shm_mutex);
 
 static void rpmsg_service_unbind(struct rpmsg_endpoint *p_ep)
 {
@@ -101,11 +101,11 @@ static void ipm_callback(const struct device *dev, void *context, uint32_t id, v
 
 int rpmsg_mi_configure_shm(struct rpmsg_mi_ctx *ctx, const struct rpmsg_mi_ctx_cfg *cfg)
 {
-	uint8_t vring_size = VRING_SIZE_GET(cfg->shm_size);
+	uint8_t vring_size = VRING_SIZE_GET(cfg->shm->size);
-	uint32_t shm_addr = SHMEM_INST_ADDR_AUTOALLOC_GET(cfg->shm_addr,
+	uint32_t shm_addr = SHMEM_INST_ADDR_AUTOALLOC_GET(cfg->shm->addr,
-							  cfg->shm_size,
+							  cfg->shm->size,
-							  instance);
+							  cfg->shm->instance);
-	uint32_t shm_size = SHMEM_INST_SIZE_AUTOALLOC_GET(cfg->shm_size);
+	uint32_t shm_size = SHMEM_INST_SIZE_AUTOALLOC_GET(cfg->shm->size);
 
 	uint32_t shm_local_start_addr = shm_addr + VDEV_STATUS_SIZE;
 	uint32_t shm_local_size = shm_size - VDEV_STATUS_SIZE;
@@ -196,7 +196,7 @@ static void ns_bind_cb(struct rpmsg_device *rdev, const char *name, uint32_t des
 
 static bool rpmsg_mi_config_verify(const struct rpmsg_mi_ctx_cfg *cfg)
 {
-	if (SHMEM_INST_SIZE_AUTOALLOC_GET(cfg->shm_size) * IPC_INSTANCE_COUNT > cfg->shm_size) {
+	if (SHMEM_INST_SIZE_AUTOALLOC_GET(cfg->shm->size) * IPC_INSTANCE_COUNT > cfg->shm->size) {
 		LOG_ERR("Not enough memory");
 		return false;
 	}
@@ -207,9 +207,9 @@ static bool rpmsg_mi_config_verify(const struct rpmsg_mi_ctx_cfg *cfg)
 int rpmsg_mi_ctx_init(struct rpmsg_mi_ctx *ctx, const struct rpmsg_mi_ctx_cfg *cfg)
 {
 	struct metal_init_params metal_params = METAL_INIT_DEFAULTS;
-	uint8_t vring_size = VRING_SIZE_GET(cfg->shm_size);
+	uint8_t vring_size = VRING_SIZE_GET(cfg->shm->size);
 	struct metal_device *device;
-	int err;
+	int err = 0;
 
 	if (!ctx || !cfg) {
 		return -EINVAL;
@@ -221,6 +221,8 @@ int rpmsg_mi_ctx_init(struct rpmsg_mi_ctx *ctx, const struct rpmsg_mi_ctx_cfg *c
 
 	LOG_DBG("RPMsg multiple instance initialization");
 
+	k_mutex_lock(&shm_mutex, K_FOREVER);
+
 	/* Start IPM workqueue */
 	k_work_queue_start(&ctx->ipm_work_q, cfg->ipm_stack_area,
 			   cfg->ipm_stack_size, cfg->ipm_work_q_prio, NULL);
@@ -234,39 +236,41 @@ int rpmsg_mi_ctx_init(struct rpmsg_mi_ctx *ctx, const struct rpmsg_mi_ctx_cfg *c
 	err = rpmsg_mi_configure_shm(ctx, cfg);
 	if (err) {
 		LOG_ERR("shmem configuration: failed - error code %d", err);
-		return err;
+		goto out;
 	}
 
 	/* Libmetal setup */
 	err = metal_init(&metal_params);
 	if (err) {
 		LOG_ERR("metal_init: failed - error code %d", err);
-		return err;
+		goto out;
 	}
 
 	err = metal_register_generic_device(&ctx->shm_device);
 	if (err) {
 		LOG_ERR("Could not register shared memory device: %d", err);
-		return err;
+		goto out;
 	}
 
 	err = metal_device_open("generic", SHM_DEVICE_NAME, &device);
 	if (err) {
 		LOG_ERR("metal_device_open failed: %d", err);
-		return err;
+		goto out;
 	}
 
 	ctx->shm_io = metal_device_io_region(device, 0);
 	if (!ctx->shm_io) {
 		LOG_ERR("metal_device_io_region failed to get region");
-		return err;
+		err = -ENODEV;
+		goto out;
 	}
 
 	 /* IPM setup. */
 	ctx->ipm_tx_handle = device_get_binding(cfg->ipm_tx_name);
 	if (!ctx->ipm_tx_handle) {
 		LOG_ERR("Could not get TX IPM device handle");
-		return -ENODEV;
+		err = -ENODEV;
+		goto out;
 	}
 
 	ctx->ipm_tx_id = cfg->ipm_tx_id;
@@ -274,7 +278,8 @@ int rpmsg_mi_ctx_init(struct rpmsg_mi_ctx *ctx, const struct rpmsg_mi_ctx_cfg *c
 	ctx->ipm_rx_handle = device_get_binding(cfg->ipm_rx_name);
 	if (!ctx->ipm_rx_handle) {
 		LOG_ERR("Could not get RX IPM device handle");
-		return -ENODEV;
+		err = -ENODEV;
+		goto out;
 	}
 
 	/* Register IPM callback. This cb executes when msg has come. */
@@ -289,13 +294,15 @@ int rpmsg_mi_ctx_init(struct rpmsg_mi_ctx *ctx, const struct rpmsg_mi_ctx_cfg *c
 	ctx->vq[RPMSG_VQ_0] = virtqueue_allocate(vring_size);
 	if (!ctx->vq[RPMSG_VQ_0]) {
 		LOG_ERR("virtqueue_allocate failed to alloc vq[RPMSG_VQ_0]");
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto out;
 	}
 
 	ctx->vq[RPMSG_VQ_1] = virtqueue_allocate(vring_size);
 	if (!ctx->vq[RPMSG_VQ_1]) {
 		LOG_ERR("virtqueue_allocate failed to alloc vq[RPMSG_VQ_1]");
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto out;
 	}
 
 	ctx->rvrings[RPMSG_VQ_0].io = ctx->shm_io;
@@ -333,17 +340,20 @@ int rpmsg_mi_ctx_init(struct rpmsg_mi_ctx *ctx, const struct rpmsg_mi_ctx_cfg *c
 
 	if (err) {
 		LOG_ERR("RPMSG vdev initialization failed %d", err);
-		return err;
+		goto out;
 	}
 
 	/* Get RPMsg device from RPMsg VirtIO device. */
 	ctx->rdev = rpmsg_virtio_get_rpmsg_device(&ctx->rvdev);
 
-	instance++;
+	cfg->shm->instance++;
 
 	LOG_DBG("RPMsg multiple instance initialization done");
 
-	return 0;
+out:
+	k_mutex_unlock(&shm_mutex);
+
+	return err;
 }
 
 int rpmsg_mi_ept_register(struct rpmsg_mi_ctx *ctx, struct rpmsg_mi_ept *ept,