Is this check safe? I mean task is cancelled, where is task set to null? It's initially NULL, so this check will cover DP tasks never started, but how about a DP task that has already ended?

If the task is cancel - its OK, calling cancel again is safe and won't have any effect
If task if freed:

static inline void comp_free(struct comp_dev *dev)
{
	assert(dev->drv->ops.free);

	/* free task if shared component or DP task*/
	if ((dev->is_shared || dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) &&
	    dev->task) {
		schedule_task_free(dev->task);
		rfree(dev->task);
		dev->task = NULL;
	}

	dev->drv->ops.free(dev);
}

free and reset are both called from IPC thread, so it looks safe. Anyway, I can change it to safer:

		volatile task * _task = dev->task;
		dev->task = NULL;
		schedule_task_free(_task);
		rfree(_task);

EDIT:
is it safer? Can't the compiler change order of lines in optimalization? Is it needed?
Free is called when a module is really being destroyed, calling "reset" during or after free operation would be fatal anyway

SOF is relying on IPC serialisation. So if both these functions are only called from within IPCs, they cannot race.

@marcinszkudlinski @lyakh I was actually thinking of the case where "dev->task" is a dangling pointer. I couldn't immediately find where dev->task is zeroed, so not sure if this check can ensure task is a valid task object.

@lyakh you're right with IPC serialization, no need for double-over-protection. @kv2019i - as you see pointer is zeroized when task is freed, No changes required here

minor: some typoes in commit, "s/posiibility/possibility", "s/sollution/solution/",

SOF is relying on IPC serialisation. So if both these functions are only called from within IPCs, they cannot race.

if the task is a lower priority thread, however, this won't switch to running that other task immediately

it won't at this point, it will when k_thread_join() is called

this check will always return true

yes, but only because the first possible jump to err is after thread is initialized. I would leave this "overprotection" as is - cost is just a few bytes of code.

@marcinszkudlinski but before it's initialised it's undefined, so this check would be meaningless too and the compiler would warn you about that.

do I understand it correctly, that previously DP tasks were terminated from comp_free() by calling schedule_task_free()? Now scheduler_dp_task_cancel() will (potentially) be called twice - from here and from schedule_task_free(). Should be safe for now, at least as long as list_del() also initialises the list item.

Yes., it is called twice, nobody guarantees that cancel had been called before free.

as long as list_del() also initialises the list item

it does:

static inline void list_item_del(struct list_item *item)
{
	item->next->prev = item->prev;
	item->prev->next = item->next;
	list_init(item);
}

@marcinszkudlinski yes, sorry, that's exactly what I meant: as long as list_init() is there, then calling it twice is ok, but if we once decide to remove it since technically it isn't needed there, then this (and probably a couple of other places) will break