Re: [PATCH v19 4/7] firmware: arm_rmm: Add support for SRO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm_rmm: SRO cancellation loop prematurely exits, leaving operations suspended
--

commit 95084224252b24a8efc8b9db3fa71d3058e8d228
Author: Steven Price <steven.price@arm.com>

firmware: arm_rmm: Add support for SRO
    
RMM v2.0 introduces the concept of "Stateful RMI Operations" (SRO). This
commit adds support for executing SROs, including donating and reclaiming
memory dynamically as requested by the RMM. It also implements cancellation
support for these operations.

> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> index c9ea964fd9081..035f21d3f26b6 100644
> --- a/drivers/firmware/arm_rmm/rmi.c
> +++ b/drivers/firmware/arm_rmm/rmi.c

[ ... ]

> +		if (ret) {
> +			/*
> +			 * All memory donating SROs must be cancellable. So a
> +			 * failure in memory allocation shouldn't be an issue.
> +			 * However, if we encounter a random failure (e.g.,
> +			 * buggy RMM), don't loop forever, just give up.
> +			 */
> +			if (WARN_ON_ONCE(!can_cancel))
> +				return ret;
> +			/*
> +			 * If we have already cancelled, and came back here due
> +			 * to an error in MEMREQ, then there is no point
> +			 * in going in loops.
> +			 */
> +			if (WARN_ON_ONCE(cancelled))
> +				break;
> +			rmi_op_cancel(sro_handle, regs);
> +			cancelled = true;
> +
> +			if (WARN_ON_ONCE(RMI_RESULT_STATUS(regs->a0) != RMI_INCOMPLETE))
> +				return ret;

[Severity: High]
Does this error path permanently leave the SRO suspended in the RMM?

The commit message notes that after requesting cancellation, the host must
still "continue the operation until it has completed (otherwise the incomplete
operation may cause future RMM operations to fail)".

However, when rmi_op_cancel() issues SMC_RMI_OP_CANCEL, it returns RMI_SUCCESS
to acknowledge the cancel request, overwriting regs->a0. As a result,
RMI_RESULT_STATUS(regs->a0) != RMI_INCOMPLETE evaluates to true, triggering
this WARN_ON_ONCE and causing an immediate return. The host fails to issue the
required SMC_RMI_OP_CONTINUE calls to drive the cancellation to completion.

> +		}
> +	}
> +
> +	if (cancelled)
> +		return -ECANCELED;
> +
> +	return regs->a0;
> +}
> +EXPORT_SYMBOL_GPL(rmi_sro_memxfer_execute);

[ ... ]

> +long rmi_sro_execute(struct arm_smccc_1_2_regs *regs)
> +{
> +	bool cancelled = false;
> +	unsigned long sro_handle = regs->a1;
> +
> +	rmi_smccc_invoke(regs);
> +
> +	sro_handle = regs->a1;
> +	while (RMI_RESULT_STATUS(regs->a0) == RMI_INCOMPLETE) {
> +		bool can_cancel = RMI_RESULT_CAN_CANCEL(regs->a0) == RMI_OP_CAN_CANCEL;
> +
> +		switch (RMI_RESULT_MEMREQ(regs->a0)) {
> +		case RMI_OP_MEM_REQ_NONE:
> +			rmi_op_continue(sro_handle, RMI_CONTINUE_KEEP_GOING,
> +					regs);
> +			break;
> +		default:
> +			WARN_ON_ONCE(1);
> +			if (!can_cancel)
> +				return regs->a0;
> +			/*
> +			 * We can't get here normally, but handle this anyway
> +			 * for a buggy RMM implementation.
> +			 */
> +			if (cancelled)
> +				return -ECANCELED;
> +			rmi_op_cancel(sro_handle, regs);
> +			cancelled = true;
> +		}
> +	}

[Severity: High]
Does this loop terminate prematurely without continuing the cancelled SRO?

Similar to rmi_sro_memxfer_execute() above, after rmi_op_cancel() updates
regs->a0 to RMI_SUCCESS, the while loop condition
(RMI_RESULT_STATUS(regs->a0) == RMI_INCOMPLETE) evaluates to false. The
function exits immediately without calling SMC_RMI_OP_CONTINUE to complete
the cancellation.

> +
> +	if (cancelled)
> +		return -ECANCELED;
> +
> +	return regs->a0;
> +}
> +EXPORT_SYMBOL_GPL(rmi_sro_execute);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924135201.850038-1-suzuki.poulose@arm.com?part=4