Skip to content

edf_schedule: don't assert if there is no extra task to run #4874

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

Merged
lgirdwood merged 1 commit into from
Nov 30, 2021
Merged

edf_schedule: don't assert if there is no extra task to run #4874

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
7 changes: 3 additions & 4 deletions src/schedule/edf_schedule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,9 @@ static void edf_scheduler_run(void *data)

irq_local_enable(flags);

/* having next task is mandatory */
assert(task_next);

schedule_edf_task_running(data, task_next);
Copy link
Collaborator

@ranj063 ranj063 Oct 14, 2021

Choose a reason for hiding this comment

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

this sounds fishy @keyonjie. If the last one in the list is completed, the main audio task would be in WFI isnt it? How will we ever get here?

Copy link
Contributor Author

@keyonjie keyonjie Oct 14, 2021

Choose a reason for hiding this comment

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

I have no explanation why the task_next could become NULL as well. But from the scheduler design POV, we should not depends on or make assumption that the main task is always in the list.

Copy link
Member

@lgirdwood lgirdwood Oct 14, 2021

Choose a reason for hiding this comment

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

How is this bug presented today ?

Copy link
Contributor Author

@keyonjie keyonjie Oct 14, 2021

Choose a reason for hiding this comment

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

@lgirdwood here: #4538

Copy link
Member

@lgirdwood lgirdwood Oct 18, 2021

Choose a reason for hiding this comment

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

ok, thanks @keyonjie - I think the correct fix is to make sure we disable all schedulers, timers, IRQs, notifiers prior to shutdown assembly code executing. This should also include cache WB/INV too.

Copy link
Contributor Author

@keyonjie keyonjie Oct 19, 2021

Choose a reason for hiding this comment

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

yes, just draft some commits wrt cleanup on secondary cores shutting down here (not tested yet):
#4718

Copy link
Member

@lgirdwood lgirdwood Oct 19, 2021

Choose a reason for hiding this comment

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

So this should be folded into the power down flow, but I'm curious why the edf scheduler is being run here (if all the task have been removed or the EDF has been shutdown for this core). This could just be a race in the shutdown path and if so we should add this to the comment and explain that no task may be present in this flow.

/* schedule next pending task */
if (task_next)
schedule_edf_task_running(data, task_next);
}

static int schedule_edf_task(void *data, struct task *task, uint64_t start,
Expand Down