Re: [PATCH v3 4/4] firmware: stratix10-svc: Add for SDM mailbox doorbell interrupt

Hi,

See my reply inline.

On 13/8/2026 2:51 pm, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] devm_request_any_context_irq() positive success values are incorrectly rejected by strict `ret == 0` check.
> - [High] Dropping trx_list_lock before callback execution allows client stack popping, leading to UAF/stack corruption.
> - [Medium] Improper handling of -EPROBE_DEFER from of_irq_get().
> --
> 
> --- Patch [4]: [PATCH v3 4/4] firmware: stratix10-svc: Add for SDM mailbox doorbell interrupt ---
> commit 0c0971a9e6fe55c0bfa458e38981b259f988e3ae
> Author: Mahesh Rao <[email protected]>
> 
> firmware: stratix10-svc: Add for SDM mailbox doorbell interrupt
> 
> Add support for SDM mailbox doorbell interrupt for async transactions. On
> interrupt, a workqueue is triggered which polls the ATF for pending responses
> and retrieves the bitmap of all retrieved and unprocessed transaction ids of
> mailbox responses from SDM. It then triggers the corresponding registered
> callbacks.
> 
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
>> index f8c2da207cb4e..8c86441eae58b 100644
>> --- a/drivers/firmware/stratix10-svc.c
>> +++ b/drivers/firmware/stratix10-svc.c
> 
> [ ... ]
> 
>> @@ -1659,14 +1666,96 @@ static inline void stratix10_smc_1_2(struct stratix10_async_ctrl *actrl,
>>   	arm_smccc_1_2_smc(args, res);
>>   }
>>   
> [ ... ]
>> +static void stratix10_async_workqueue_handler(struct work_struct *work)
>> +{
> [ ... ]
>> +		do {
>> +			transaction_id = find_next_bit(pend_on_irq,
>> +						       TOTAL_TRANSACTION_IDS,
>> +						       transaction_id);
>> +			if (transaction_id >= TOTAL_TRANSACTION_IDS)
>> +				break;
>> +
>> +			cb = NULL;
>> +			cb_arg = NULL;
>> +			spin_lock(&actrl->trx_list_lock);
>> +			hash_for_each_possible(actrl->trx_list, handler,
>> +					       next, transaction_id) {
>> +				if (handler->transaction_id == transaction_id) {
>> +					cb = handler->cb;
>> +					cb_arg = handler->cb_arg;
>> +					break;
>> +				}
>> +			}
>> +			spin_unlock(&actrl->trx_list_lock);
>> +			if (cb)
>> +				cb(cb_arg);
> 
> [Severity: High]
> Is there a risk of stack corruption or a use-after-free here?
> 
> By executing the callback after dropping trx_list_lock, a racing client
> timeout might execute stratix10_svc_async_done() and delete the handler.
> 
> This could result in popping the client stack before the workqueue executes
> the callback on the now-invalid stack address cb_arg. Should the lock be held
> during the callback or is there another way to ensure cb_arg remains valid?

Agreed. We copy cb/cb_arg under the lock (so the handler itself is 
safe), but a client that times out and calls stratix10_svc_async_done() 
on another CPU can still free cb_arg before the callback runs.

Since callbacks run in BH context (system_bh_wq) and are already atomic, 
we can simply invoke the callback under the lock so async_done() waits 
for it to finish:

scoped_guard(spinlock, &actrl->trx_list_lock) {
	hash_for_each_possible(actrl->trx_list, handler,
		next, transaction_id) {
	if (handler->transaction_id == transaction_id) {
		if (handler->cb)
			handler->cb(handler->cb_arg);
		break;
		}
	}
}

The only constraint is that a callback must not call back into 
async_send()/async_done() (same lock). Does this approach look good to you?

> 
>> +
>> +			transaction_id++;
>> +		} while (transaction_id < TOTAL_TRANSACTION_IDS);
>> +	}
>> +	enable_irq(actrl->irq);
>> +}
> 
> [ ... ]
> 
>> @@ -1727,6 +1816,22 @@ static int stratix10_svc_async_init(struct stratix10_svc_controller *controller)
>>   	hash_init(actrl->trx_list);
>>   	atomic_set(&actrl->common_achan_refcount, 0);
>>   
>> +	irq = of_irq_get(dev_of_node(dev), 0);
>> +	if (irq <= 0) {
> 
> [Severity: Medium]
> Does this improperly swallow -EPROBE_DEFER?
> 
> Because of_irq_get() can return -EPROBE_DEFER as a negative value,
> this check will convert the probe deferral into a permanent fallback
> to polling mode. Should this propagate -EPROBE_DEFER instead?
> 

Agreed. The parent here is the GIC (initialized early), so it shouldn't 
defer in practice, but propagating it is more correct:

irq = of_irq_get(dev_of_node(dev), 0);
if (irq == -EPROBE_DEFER) {
	ida_destroy(&actrl->async_id_pool);
	actrl->invoke_fn = NULL;
	return -EPROBE_DEFER;
}
if (irq <= 0) {
	dev_warn(dev, "Failed to get IRQ, falling back to polling mode\n");
} else {
	...
}

Since the IRQ is optional (polling is a valid fallback), do you want us 
to propagate -EPROBE_DEFER, or just keep the polling fallback on any 
failure?

>> +		dev_warn(dev, "Failed to get IRQ, falling back to polling mode\n");
>> +	} else {
>> +		ret = devm_request_any_context_irq(dev, irq, stratix10_svc_async_irq_handler,
>> +						   IRQF_NO_AUTOEN, "stratix10_svc", controller);
>> +		if (ret == 0) {
> 
> [Severity: High]
> Will this correctly handle nested IRQs?
> 
> devm_request_any_context_irq() returns IRQC_IS_NESTED (which is 1) on success
> for nested IRQs.
> 
> By strictly checking for ret == 0, successful nested IRQ assignments will
> be rejected. This skips INIT_WORK and enable_irq(), leaving the registered IRQ
> permanently disabled and causing asynchronous transactions to time out.
> Should this check for ret >= 0 instead?
> 

devm_request_any_context_irq() returns 0 (hard IRQ) or 1 (nested) on 
success. On our devices the SDM doorbell is a normal GIC SPI (hard IRQ), 
so ret == 0 is always correct here and never nested. ret >= 0 would be 
the more idiomatic check.

Do you prefer we keep ret == 0, or switch to ret >= 0?

Thanks,
Tze Yee

>> +			dev_info(dev,
>> +				 "Registered IRQ %d for sip async operations\n",
>> +				 irq);
>> +			actrl->irq = irq;
>> +			INIT_WORK(&actrl->async_work, stratix10_async_workqueue_handler);
>> +			enable_irq(actrl->irq);
>> +		}
>> +	}
>