Re: [PATCH v2 2/3] media: qcom: iris: Wait for threaded IRQ completion during power-off

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 11/11/2026 09:05, Vishnu Reddy wrote:
> +void iris_acquire_core_lock(struct iris_core *core)
> +{
> +	mutex_lock(&core->lock);
> +
> +	while (READ_ONCE(core->irq_disabling)) {
> +		mutex_unlock(&core->lock);
> +		wait_event(core->irq_disable_waitqueue, !READ_ONCE(core->irq_disabling));
> +		mutex_lock(&core->lock);
> +	}
> +}
> +

> +static void iris_vpu_disable_irq(struct iris_core *core)
> +{
> +	WRITE_ONCE(core->irq_disabling, true);
> +	mutex_unlock(&core->lock);
> +	disable_irq(core->irq);
> +	mutex_lock(&core->lock);
> +	WRITE_ONCE(core->irq_disabling, false);
> +	wake_up_all(&core->irq_disable_waitqueue);
> +}
> +

Ouch, this is some mad amount of jumping through hoops.

As General Pierre François Joseph Bosquet said of the Charge of the 
Light Brigade in the Crimean war "C’est magnifique, mais ce n’est pas la 
guerre" - It is magnificent, but it is not war.

Why do we have to hold core->lock over iris_vpu_power_off() at all ?

- What is the data-structure or
- Logical state (the state variable I suspect)
- Or guarantee of atomicity

We are providing holding that lock ?

These are pretty much rhetorical questions - the above locking scheme 
can't be right. It implies we are doing something wrong and not really 
fixing that wrong thing.

Holding the lock across power_off() seems to be that wrong thing.

disable_irq_nosync() seems to be a correct fix - so if our locking is so 
muddled we have to go to the above - there better be a fully reasoned 
and unavoidable reason why.

Perhaps I'm being overly reductive here but - why not "just" refactor 
the code so that the power-off comes after the piece held in the lock ?

i.e. push that into a worker and then do a synchronous wait on the worker ?

The chatbot told me I was wrong about the worker because "it would 
deadlock" and I can't be bothered burning the tokens to tell it "I 
didn't mean hold the lock in the worker"

It did make the point that it thinks the only thing being protected by 
the lock is

intr_status = readl(core->reg_base + WRAPPER_INTR_STATUS);
...
writel(CLEAR_XTENSA2HOST_INTR, core->reg_base + CPU_CS_A2HSOFTINTCLR);

Which venus does like this:

static irqreturn_t venus_isr(struct venus_core *core)       /* 
hfi_venus.c */
{
	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
	...
	writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR);
	return IRQ_WAKE_THREAD;
}

i.e. power off in the IRQ.

Perhaps even simpler - just do the power-off directly outside of the 
locked section... once the IRQ is quiescent ?

Either way I don't think we have a good reason to hold this lock and can 
certainly find a way not to hold it - assuming the only thing we care 
about is kicking that register - do it in the IRQ why not else do a 
worker with a rendezvous or even more straight forward.

void iris_core_deinit(struct iris_core *core)
{
	bool power_off = false;
	int ret;

	ret = pm_runtime_resume_and_get(core->dev);

	mutex_lock(&core->lock);
	if (core->state != IRIS_CORE_DEINIT) {
		iris_fw_unload(core);
		power_off = !ret;
		iris_hfi_queues_deinit(core);
		core->state = IRIS_CORE_DEINIT;
	}
	mutex_unlock(&core->lock);

	if (power_off)
		iris_vpu_power_off(core); <- this ?

	if (!ret)
		pm_runtime_put_sync(core->dev);
}

---
bod