Skip to content

tests/drivers/ipm: Support ipm test case in SMP #14619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

tests/drivers/ipm: Support ipm test case in SMP #14619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 17 additions & 28 deletions drivers/console/ipm_console_receiver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,41 +15,49 @@
#include <ipm.h>
#include <console/ipm_console.h>
#include <misc/__assert.h>
#include <atomic.h>

static void ipm_console_thread(void *arg1, void *arg2, void *arg3)
{
u8_t size32;
u16_t type;
int ret, key;
int ret;
k_spinlock_key_t key;
struct device *d;
const struct ipm_console_receiver_config_info *config_info;
struct ipm_console_receiver_runtime_data *driver_data;
int pos;
int pos, end;
atomic_t *target;

d = (struct device *)arg1;
driver_data = d->driver_data;
config_info = d->config->config_info;
ARG_UNUSED(arg2);
size32 = 0U;
pos = 0;
target = config_info->print_num;

while (1) {
k_sem_take(&driver_data->sem, K_FOREVER);

key = k_spin_lock(&driver_data->rb_spinlock);
ret = ring_buf_item_get(&driver_data->rb, &type,
(u8_t *)&config_info->line_buf[pos],
NULL, &size32);
k_spin_unlock(&driver_data->rb_spinlock, key);
if (ret) {
/* Shouldn't ever happen... */
printk("ipm console ring buffer error: %d\n", ret);
size32 = 0U;
continue;
}

end = 0;
if (config_info->line_buf[pos] == '\n' ||
pos == config_info->lb_size - 2) {
if (pos != config_info->lb_size - 2) {
config_info->line_buf[pos] = '\0';
end = 1;
} else {
config_info->line_buf[pos + 1] = '\0';
}
Expand All @@ -61,26 +69,14 @@ static void ipm_console_thread(void *arg1, void *arg2, void *arg3)
printf("%s: '%s'\n", d->config->name,
config_info->line_buf);
}
if (end == 1) {
atomic_dec(target);
}
pos = 0;
} else {
++pos;
}

/* ISR may have disabled the channel due to full buffer at
* some point. If that happened and there is now room,
* re-enable it.
Copy link
Contributor

@andyross andyross Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why it's OK to remove this. That condition (an interrupt delivering data that fills the buffer) seems like it can still happen.

Copy link
Contributor Author

@wentongwu wentongwu Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to use the flags(enabled and channel_disable), there is possibility that the flags are accessed by ISR and receiver thread at the same time if SMP enabled.

*
* Lock interrupts to avoid pathological scenario where
* the buffer fills up in between enabling the channel and
* clearing the channel_disabled flag.
*/
if (driver_data->channel_disabled &&
ring_buf_space_get(&driver_data->rb)) {
key = irq_lock();
ipm_set_enabled(driver_data->ipm_device, 1);
driver_data->channel_disabled = 0;
irq_unlock(key);
}
}
}

Expand All @@ -90,27 +86,20 @@ static void ipm_console_receive_callback(void *context, u32_t id,
struct device *d;
struct ipm_console_receiver_runtime_data *driver_data;
int ret;
k_spinlock_key_t key;

ARG_UNUSED(data);
d = context;
driver_data = d->driver_data;

key = k_spin_lock(&driver_data->rb_spinlock);
/* Should always be at least one free buffer slot */
ret = ring_buf_item_put(&driver_data->rb, 0, id, NULL, 0);
k_spin_unlock(&driver_data->rb_spinlock, key);
__ASSERT(ret == 0, "Failed to insert data into ring buffer");
k_sem_give(&driver_data->sem);

/* If the buffer is now full, disable future interrupts for this channel
* until the thread has a chance to consume characters.
*
Copy link
Contributor

@andyross andyross Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as before: why is it OK to remove this? The condition and the comment explaining it (a RX interrupt is out of space in the buffer) makes sense, and I don't see anything in the patch that addresses it.

Copy link
Contributor Author

@wentongwu wentongwu Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sender thread will wait until there is space in ring buffer to do next send.

  •   while (ring_buf_space_get(&driver_data->rb) == 0) {

  •   }

* This works without losing data if the sending side tries to send
* more characters because the sending side is making an ipm_send()
* call with the wait flag enabled. It blocks until the receiver side
* re-enables the channel and consumes the data.
*/
if (ring_buf_space_get(&driver_data->rb) == 0) {
ipm_set_enabled(driver_data->ipm_device, 0);
driver_data->channel_disabled = 1;
while (ring_buf_space_get(&driver_data->rb) == 0) {
}
}

Expand Down
9 changes: 9 additions & 0 deletions include/drivers/console/ipm_console.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include <kernel.h>
#include <device.h>
#include <ring_buffer.h>
#include <spinlock.h>
#include <atomic.h>

#ifdef __cplusplus
extern "C" {
Expand Down Expand Up @@ -61,6 +63,8 @@ struct ipm_console_receiver_config_info {
* IPM_CONSOLE_STDOUT or IPM_CONSOLE_PRINTK
*/
unsigned int flags;

atomic_t *print_num;
};

struct ipm_console_receiver_runtime_data {
Expand All @@ -80,6 +84,11 @@ struct ipm_console_receiver_runtime_data {

/** Receiver worker thread */
struct k_thread rx_thread;

/**
* Ring buffer spinlock
*/
struct k_spinlock rb_spinlock;
};

struct ipm_console_sender_config_info {
Expand Down
6 changes: 3 additions & 3 deletions include/ring_buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ extern "C" {
* @brief A structure to represent a ring buffer
*/
struct ring_buf {
u32_t head; /**< Index in buf for the head element */
u32_t tail; /**< Index in buf for the tail element */
volatile u32_t head; /**< Index in buf for the head element */
volatile u32_t tail; /**< Index in buf for the tail element */
Copy link
Contributor

@andyross andyross Mar 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong spot. There are plenty of uses for a ring buffer that don't involve contending with SMP CPUs. Instead, I think you probably want to tag the pointer to the struct volatile in the app instead.

Copy link
Contributor Author

@wentongwu wentongwu Mar 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Thanks

Copy link

@ghost ghost Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hate to stick my nose in here, but is volatile even the right thing here? If the issue is that other CPUs might change the value, then I'm not convinced that we don't also need explicit storage fences. Probably not a problem on x86, but on other arches.

Copy link
Contributor

@andyross andyross Apr 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On architectures where it matters, the fence will be provided by the synchronization primitive where we are using one, which we aren't here. It's true that IPM is sort of a special case, as it was intended for a asymmetric multiprocessing architecture (Quark SE -- one x86 and one ARC on the same memory bus) with no cache. But that's not really something we can fix with this patch.

union ring_buf_misc {
struct ring_buf_misc_item_mode {
u32_t dropped_put_count; /**< Running tally of the
Expand All @@ -39,7 +39,7 @@ struct ring_buf {
u32_t tmp_head;
} byte_mode;
} misc;
u32_t size; /**< Size of buf in 32-bit chunks */
volatile u32_t size; /**< Size of buf in 32-bit chunks */

union ring_buf_buffer {
u32_t *buf32; /**< Memory region for stored entries */
Expand Down
7 changes: 1 addition & 6 deletions tests/drivers/ipm/src/ipm_dummy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,8 @@ static int ipm_dummy_send(struct device *d, int wait, u32_t id,
driver_data->regs.id = id;
driver_data->regs.busy = 1U;

irq_offload(ipm_dummy_isr, d);
ipm_dummy_isr(d);

if (wait) {
while (driver_data->regs.busy) {
/* busy-wait */
Copy link
Contributor

@andyross andyross Apr 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed? It sounds like the original test expects the busy flag to be cleared by an ISR before returning, and now it's going to return synchronously before the interrupt (or whatever) reports success.

Copy link
Contributor Author

@wentongwu wentongwu Apr 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above, sender thread will wait until there is at least one free buffer for the ring buffer.

}
}
return 0;
}

Expand Down
11 changes: 10 additions & 1 deletion tests/drivers/ipm/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <device.h>
#include <init.h>
#include <stdio.h>
#include <atomic.h>

#include <tc_util.h>
#include "ipm_dummy.h"
Expand Down Expand Up @@ -54,6 +55,7 @@ DEVICE_INIT(ipm_console_send0, "ipm_send0", ipm_console_sender_init,
static u32_t ring_buf_data[RING_BUF_SIZE32];
static K_THREAD_STACK_DEFINE(thread_stack, IPM_CONSOLE_STACK_SIZE);
static char line_buf[LINE_BUF_SIZE];
static atomic_t target;

/* Dump incoming messages to printk() */
static struct ipm_console_receiver_config_info receiver_config = {
Expand All @@ -63,7 +65,8 @@ static struct ipm_console_receiver_config_info receiver_config = {
.rb_size32 = RING_BUF_SIZE32,
.line_buf = line_buf,
.lb_size = LINE_BUF_SIZE,
.flags = DEST
.flags = DEST,
.print_num = &target
};

struct ipm_console_receiver_runtime_data receiver_data;
Expand All @@ -78,16 +81,19 @@ void main(void)
int rv, i;
struct device *ipm;

target = 0;
TC_START("Test IPM");
ipm = device_get_binding("ipm_dummy0");

/* Try sending a raw string to the IPM device to show that the
* receiver works
*/
atomic_inc(&target);
for (i = 0; i < strlen(thestr); i++) {
ipm_send(ipm, 1, thestr[i], NULL, 0);
}

atomic_inc(&target);
/* Now do this through printf() to exercise the sender */
printf("Lorem ipsum dolor sit amet, consectetur adipiscing elit, "
"sed do eiusmod tempor incididunt ut labore et dolore magna "
Expand All @@ -102,6 +108,9 @@ void main(void)
* automation purposes?
*/

while (atomic_get(&target) != 0) {
}

rv = TC_PASS;
TC_END_RESULT(rv);
TC_END_REPORT(rv);
Expand Down
1 change: 0 additions & 1 deletion tests/drivers/ipm/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,4 @@ tests:
peripheral.mailbox:
filter: not CONFIG_SOC_QUARK_SE_C1000_SS
arch_exclude: posix xtensa
platform_exclude: qemu_x86_64 # see issue #12478
tags: drivers ipc