API k_delayed_work_submit_to_queue() make a delayed_work unusable

[noodlefighter]

Calling k_delayed_work_submit_to_queue() repeatly would broke the delayed_work object.

test case:

K_THREAD_STACK_DEFINE(stack_area, 1024);
struct k_work_q workq;
volatile struct k_delayed_work dwork;

void dwork_handler (struct k_work *work) {
    printk("work done!!!!!!!!!!!!!!!!!!!!!\n");
}

void submit (void) {
    int ret = k_delayed_work_submit_to_queue(&workq, &dwork, 1);
    if (ret < 0) {
        printk("submit fail, ret=%d, flag=0x%02x\n", ret, dwork.work.flags[0]);
    } else {
        printk("submit ok, flag=0x%02x\n", dwork.work.flags[0]);
    }
}

int main (void) {
    k_work_q_start(&workq, stack_area, K_THREAD_STACK_SIZEOF(stack_area), 0);

    k_delayed_work_init(&dwork, dwork_handler);

    printk("\ndestroy it.............\n");
    for (int i = 0; i < 100; i++) {
        submit();
        while (!k_work_pending(&dwork.work));
        submit();
        k_sleep(10);
    }

    k_sleep(1000);
    printk("\nbroken..............\n");
    submit();
    k_sleep(1000);
    submit();
    k_sleep(1000);
    submit();
    k_sleep(100);
}

run:

[QEMU] CPU: cortex-m3
***** BOOTING ZEPHYR OS v1.10.0- - BUILD: Feb  2 2018 05:50:13 *****

destroy it.............
submit ok, flag=0x00
submit ok, flag=0x01
submit fail, ret=-22, flag=0x01
submit fail, ret=-22, flag=0x01
...
submit fail, ret=-22, flag=0x01
submit fail, ret=-22, flag=0x01

broken..............
submit fail, ret=-22, flag=0x01
submit fail, ret=-22, flag=0x01
submit fail, ret=-22, flag=0x01

-22 is "-EINVAL", let's check the docs:

-EINVAL Work item is being processed or has completed its work.

Is it a bug or just some wrong with my operation?

[andyross]

The test case seems a little weird. It looks like this line has a boolean inversion:

    while (!k_work_pending(&dwork.work));

My guess is that this was intended to busy-wait until the handler executes (itself a fragile idea -- that will only work if the work_q has a higher priority than the main thread). But instead it's waiting until the work item is pending. And since it was just submitted with a non-zero timeout, it's guaranteed to be pending here and this while() will only run the test once.

So what happens is that your code falls straight through to a second submission of the same item, which succeeds as a noop per docs. But then, indeed, there's a bug. The handler never actually executes after that second submission, and in fact the third submission then fails.

[andyross]

Never mind, the inversion was mine, not yours: "pending" in this context means "the timeout is finished and we're in the main work queue" not "pending on timeout". I see what you're doing now, and indeed it failed to try to resubmit a pending object, despite the fact that the code was trying to do exactly that.

There's a fix for this specific case up at #6189

Note that careful review shows that this particular usage can't ever be safe, though. The work queue itself does no synchronization, so it's not legal to call this function to "restart the timeout" without external synchronization in the general case. Cleaned up the docs to reflect that. But it seems to make your particular case work. Be very careful.