pipe: fix a infinite loop issue in schedule module

[RanderWang]

pipe: fix a infinite loop issue in schedule module

There are two paths to delete a task in shedule list. A task would be
deleted when task is completed or cancelled. So it maybe deleted
twice. there is a circle in the schedule list after a sequence
of append-delete operations. This makes a infinite loop in schedule function

Now all of list items are deleted in one place and cancelled when tasks are complete

Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

[RanderWang]

@xiulipan Hi xiuli, could you help to update the info of delete-append operations you found? Thank you!

[xiulipan]

if we have to list_item a,b and the header as 0
after append a,b and delete b.a.
We will get

0<-0->0
0<-a->0
a<-b->0

And by wrongly another delete of b we will get

a<-0->0
0<-a->0
a<-b->0

/* delete item from the list */
static inline void list_item_del(struct list_item *item)
{
        item->next->prev = item->prev;
        // b       0               = a
        item->prev->next = item->next;
        //  b       a               = 0
}

From now on, if we want to append anything the whole list will show nothing in the list.
EG. append a

a<-0->0
a<-a->0
a<-b->0

/* add new item to the end or tail of the list */
static inline void list_item_append(struct list_item *item,
        struct list_item *list)
{
        struct list_item *tail = list->prev; // a

        tail->next = item; // a.next = a
        item->next = list; // a.next = 0
        item->prev = tail; // a.prev = a
        list->prev = item; // 0.prev =a
}

Here we did not touch anything of 0, and with list_for_item or list_for_item_safe
We will have a empty list

[RanderWang]

sorry, I will update my patch for a error

[RanderWang]

ok, I update it and thanks for review

[xiulipan]

For the double add loopback here is the pattern
add a, b, a

a<-0->a
b<-a->0
a<-b->a

and delete b

a<-0->a
a<-a->a
a<-b->a

Here is a loop as 0->a->a->a......
This will make any list_for_item or list_for_item_safe to loop forever.

[lgirdwood]

Best to remove list_item_del() so that we remove the task list only in one place.

[RanderWang]

@lrgirdwo yes, it is only deleted in this function. But this function is called by pipeline_schedule_cancel or _irq_task

[RanderWang]

So the list_item_del() should be kept here.

[lgirdwood]

@RanderWang we need to do something like this where, we lock task state updates, only delete list item in one place and wait for task to complete when we cancel it. compile tested only.

diff --git a/src/audio/pipeline.c b/src/audio/pipeline.c
index ee8cf680..b0839e0f 100644
--- a/src/audio/pipeline.c
+++ b/src/audio/pipeline.c
@@ -1131,7 +1131,12 @@ void pipeline_schedule_copy_idle(struct pipeline *p)
 
 void pipeline_schedule_cancel(struct pipeline *p)
 {
-	schedule_task_complete(&p->pipe_task);
+	int err;
+
+	/* cancel and wait for pipeline to complete */
+	err = schedule_task_cancel(&p->pipe_task, 1);
+	if (err < 0)
+		trace_pipe_error("pC0");
 }
 
 static void pipeline_task(void *arg)
diff --git a/src/include/sof/schedule.h b/src/include/sof/schedule.h
index 83d83454..45232c47 100644
--- a/src/include/sof/schedule.h
+++ b/src/include/sof/schedule.h
@@ -38,6 +38,7 @@
 #include <sof/lock.h>
 #include <sof/list.h>
 #include <sof/work.h>
+#include <sof/wait.h>
 
 struct sof;
 
@@ -55,6 +56,8 @@ struct sof;
 #define TASK_PRI_MED	0
 #define TASK_PRI_HIGH	-20
 
+/* maximun task time slice in miliseconds */
+#define SCHEDULE_TASK_MAX_TIME_SLICE	50
 
 /* task descriptor */
 struct task {
@@ -72,6 +75,7 @@ struct task {
 
 	/* runtime duration in scheduling clock base */
 	uint64_t max_rtime;		/* max time taken to run */
+	completion_t complete;
 };
 
 void schedule(void);
@@ -80,6 +84,8 @@ void schedule_task(struct task *task, uint64_t start, uint64_t deadline);
 
 void schedule_task_idle(struct task *task, uint64_t deadline);
 
+int schedule_task_cancel(struct task *task, int wait);
+
 void schedule_task_complete(struct task *task);
 
 static inline void schedule_task_init(struct task *task, void (*func)(void *),
diff --git a/src/lib/schedule.c b/src/lib/schedule.c
index b98934d2..e9db69bc 100644
--- a/src/lib/schedule.c
+++ b/src/lib/schedule.c
@@ -174,6 +174,7 @@ static struct task *schedule_edf(void)
 	struct task *task;
 	struct task *future_task = NULL;
 	uint64_t current;
+	uint32_t flags;
 
 	tracev_pipe("edf");
 
@@ -196,7 +197,15 @@ static struct task *schedule_edf(void)
 	} else {
 		/* yes, run current task */
 		task->start = current;
+
+		/* init task for running */
+		wait_init(&task->complete);
+		spin_lock_irq(&sch->lock, flags);
 		task->state = TASK_STATE_RUNNING;
+		list_item_del(&task->list);
+		spin_unlock_irq(&sch->lock, flags);
+
+		/* now run task at correct run level */
 		arch_run_task(task);
 	}
 
@@ -204,33 +213,38 @@ static struct task *schedule_edf(void)
 	return future_task;
 }
 
-#if 0 /* FIXME: is this needed ? */
-/* delete task from scheduler */
-static int schedule_task_del(struct task *task)
+/* cancel and delete task from scheduler - wont stop it if already running */
+int schedule_task_cancel(struct task *task, int wait)
 {
 	uint32_t flags;
 	int ret = 0;
 
 	tracev_pipe("del");
 
-	/* add task to list */
 	spin_lock_irq(&sch->lock, flags);
 
-	/* is task already running ? */
-	if (task->state == TASK_STATE_RUNNING) {
-		ret = -EAGAIN;
-		goto out;
+	/* check current task state */
+	switch (task->state) {
+	case TASK_STATE_QUEUED:
+		/* delete task */
+		list_item_del(&task->list);
+		task->state = TASK_STATE_CANCEL;
+		break;
+	case TASK_STATE_RUNNING:
+		/* already running, nothing we can do about it atm */
+		if (wait) {
+			task->complete.timeout = SCHEDULE_TASK_MAX_TIME_SLICE;
+			ret = wait_for_completion_timeout(&task->complete);
+		} else
+			ret = -EBUSY;
+		break;
+	default:
+		break;
 	}
 
-	list_item_del(&task->list);
-	task->state = TASK_STATE_COMPLETED;
-
-out:
 	spin_unlock_irq(&sch->lock, flags);
 	return ret;
 }
-#endif
-
 
 static int _schedule_task(struct task *task, uint64_t start, uint64_t deadline)
 {
@@ -310,9 +324,11 @@ void schedule_task_complete(struct task *task)
 	tracev_pipe("com");
 
 	spin_lock_irq(&sch->lock, flags);
-	list_item_del(&task->list);
 	task->state = TASK_STATE_COMPLETED;
 	spin_unlock_irq(&sch->lock, flags);
+
+	/* tell any waiter that task has completed */
+	wait_completed(&task->complete);
 }
 
 static void scheduler_run(void *unused)

[RanderWang]

@lgirdwood two changes based on your patch.
(1)
+/* maximun task time slice in miliseconds */
+#define SCHEDULE_TASK_MAX_TIME_SLICE 50

change 50us to 5000us

(2) it is no good for wait_for_completion_timeout protected by spin_lock_irq
move spin_lock_irq to only protect list_item_del in schedule_task_cancel

[lgirdwood]

The comment says milliseconds, but time is in micro seconds.

[RanderWang]

yes, correct. I think 50us is too short, so I change it to 5ms.

[lgirdwood]

We need to keep the lock held over this whole function, but we can release it in if (wait) {}

[RanderWang]

Yes, I will update the PR

[RanderWang]

update PR, thanks for review.