Repository navigation
Correctness and style fixes - #4856
Conversation
|
@lyakh can you split this PR into smaller chunks, it looks like one patch may need to rework to pass the CI (or CI may need updated as your fixes look correct). |
If the last task in the timer domain is completed, the ll_scheduler tries to unregister the task from the domain. The timer domain driver currently tries to unregister the interruptin its .domain_unregister handler, which then leads to a crash. Avoid unregistering the handler from itself. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
If the last task completes in a domain, don't re-enable it. It will be re-enabled when a new task is submitted. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
atomic_read() returns values of type uint32_t, no need to cast to it again. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
@lgirdwood #4868 and #4869 |
|
SOFCI TEST |
|
@lyakh Looks like one IPC timeout for KWD, you may to to further bisect the PR to help root cause the commit |
|
SOFCI TEST |
| * and we cannot unregister the timer interrupt handler from the handler | ||
| * itself. Keep the interrupt registered but disabled. This assumes that | ||
| * the handler is at least not unloaded. | ||
| */ |
There was a problem hiding this comment.
@lgirdwood @lyakh the finding here is good, but this is behavior change for the timer_domain_unregister().
Better just remove the unregister call from the interrupt handler itself, while keep this timer_domain_unregister() unchanged:
diff --git a/src/schedule/ll_schedule.c b/src/schedule/ll_schedule.c
index c63d34a20a70..59309af3b8cd 100644
--- a/src/schedule/ll_schedule.c
+++ b/src/schedule/ll_schedule.c
@@ -107,11 +107,6 @@ static void schedule_ll_task_update_start(struct ll_schedule_data *sch,
static void schedule_ll_task_done(struct ll_schedule_data *sch,
struct task *task)
{
- /* Remove from the task list, schedule_task_cancel() won't handle it again */
- list_item_del(&task->list);
-
- domain_unregister(sch->domain, task, atomic_sub(&sch->num_tasks, 1) - 1);
-
tr_info(&ll_tr, "task complete %p %pU", task, task->uid);
tr_info(&ll_tr, "num_tasks %d total_num_tasks %d",
atomic_read(&sch->num_tasks),
There was a problem hiding this comment.
@lgirdwood I don't know. I'd prefer to know exactly what that would fix. Also I'm not 100% certain we always cancel all tasks. I actually think we don't. I think it's valid to just return COMPLETED from a task and not cancel it at all.
There was a problem hiding this comment.
@lgirdwood I don't know. I'd prefer to know exactly what that would fix. Also I'm not 100% certain we always cancel all tasks. I actually think we don't. I think it's valid to just return
COMPLETEDfrom a task and not cancel it at all.
@lyakh I think yes, as the task_done itself can't unregister the handler from itself, we need to call .schedule_task_cancel() for all tasks added via .schedule_task(), if any exception found, we should fix it.
The .schedule_task_cancel() is something like finishing a task formally, it changes the task state to _CANCEL, delete it from the scheduler task list and decrease scheduler task count, and eventually unregister task from domain. No matter if the task is already done or not, from running POV, we need this formal symmetric ops.
At the moment, we follow that for all pipelines, for SA task, for DMIC ramping task, for IDC comp copy task, for DMA trace task and probe task.
Addressing several outstanding issues, e.g. #4746, #4796