From d469ec076bfebe65068553d7e805d796c745efee Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 17 Jan 2024 23:05:56 +0200 Subject: [PATCH 1/3] idc: zephyr: add a timeout to blocking IDC send Replace infinite wait with a time-limited wait. In case a blocking IDC message is not handled within IDC_TIMEOUT, return an error instead of waiting. This allows to debug stuck core-to-core communication easier. Signed-off-by: Kai Vehmanen --- src/idc/zephyr_idc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/idc/zephyr_idc.c b/src/idc/zephyr_idc.c index 887cff0eb375..d165ef9f19b2 100644 --- a/src/idc/zephyr_idc.c +++ b/src/idc/zephyr_idc.c @@ -151,7 +151,7 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) switch (mode) { case IDC_BLOCKING: - ret = k_p4wq_wait(work, K_FOREVER); + ret = k_p4wq_wait(work, K_USEC(IDC_TIMEOUT)); if (!ret) /* message was sent and executed successfully, get status code */ ret = idc_msg_status_get(msg->core); From 72e31f3bcb52d3538522462e6d002a010094680b Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 17 Jan 2024 23:08:23 +0200 Subject: [PATCH 2/3] idc: zephyr: remove unnecessary cache invd/flush calls The IDC message objects are stored to static "idc_work", so the cache operations are unnecessary when pointers to these messages are passed around. Signed-off-by: Kai Vehmanen --- src/idc/zephyr_idc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/idc/zephyr_idc.c b/src/idc/zephyr_idc.c index d165ef9f19b2..22db8636c401 100644 --- a/src/idc/zephyr_idc.c +++ b/src/idc/zephyr_idc.c @@ -73,8 +73,7 @@ static void idc_handler(struct k_p4wq_work *work) int payload = -1; k_spinlock_key_t key; - /* A message is received from another core, invalidate local cache */ - sys_cache_data_invd_range(msg, sizeof(*msg)); + __ASSERT_NO_MSG(!is_cached(msg)); if (msg->size == sizeof(int)) { const int idc_handler_memcpy_err __unused = @@ -145,8 +144,8 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) /* Temporarily store sender core ID */ msg_cp->core = cpu_get_id(); - /* Sending a message to another core, write back local message cache */ - sys_cache_data_flush_range(msg_cp, sizeof(*msg_cp)); + __ASSERT_NO_MSG(!is_cached(msg_cp)); + k_p4wq_submit(q_zephyr_idc + target_cpu, work); switch (mode) { From 342553cf91801ebe751c7613d247104155936eae Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 17 Jan 2024 22:51:09 +0200 Subject: [PATCH 3/3] idc: zephyr: fix race between IDC p4wq worker and new msg submission A race exists between submitting a new IDC message and completion of a previous message. The p4wq interface mandates k_p4wq_wait() must be called to claim back ownership of the work object. The SOF IDC code does not do this, but relies on the caller not to send multiple IDC messages to the same destination core. Add appropriate calls to k_p4wq_wait() to handle this issue. If caller wants to send an IDC in blocking mode, also call k_p4wq_wait() in blocking mode. If non-blocking, return -EBUSY immediately. The check for CPU status is moved earlier in the function. No pointing in waiting for p4wq semaphore if the target core is already powered down, so do the power state check as the first step. Signed-off-by: Kai Vehmanen --- src/idc/zephyr_idc.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/idc/zephyr_idc.c b/src/idc/zephyr_idc.c index 22db8636c401..88a0ef40c449 100644 --- a/src/idc/zephyr_idc.c +++ b/src/idc/zephyr_idc.c @@ -120,6 +120,32 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) int ret; int idc_send_memcpy_err __unused; + if (!cpu_is_core_enabled(target_cpu)) { + tr_err(&zephyr_idc_tr, "Core %u is down, cannot sent IDC message", target_cpu); + return -EACCES; + } + + /* + * Handler is NULL when work object has never been submitted. + * In all other cases, we must use k_p4wq_wait() before reuse + * of the object. + */ + if (work->handler) { + /* + * If new request is in blocking mode, we must call + * k_p4wq in blocking mode. This is workaround for + * k_p4wq_wait() interface. + */ + work->sync = (mode == IDC_BLOCKING); + + ret = k_p4wq_wait(work, K_USEC(IDC_TIMEOUT)); + if (ret < 0) { + tr_err(&zephyr_idc_tr, "idc_send_msg error %d, target core %u", + ret, target_cpu); + return ret; + } + } + idc_send_memcpy_err = memcpy_s(msg_cp, sizeof(*msg_cp), msg, sizeof(*msg)); assert(!idc_send_memcpy_err); /* Same priority as the IPC thread which is an EDF task and under Zephyr */ @@ -128,10 +154,6 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) work->handler = idc_handler; work->sync = mode == IDC_BLOCKING; - if (!cpu_is_core_enabled(target_cpu)) { - tr_err(&zephyr_idc_tr, "Core %u is down, cannot sent IDC message", target_cpu); - return -EACCES; - } if (msg->payload) { idc_send_memcpy_err = memcpy_s(payload->data, sizeof(payload->data), msg->payload, msg->size);