Re: [PATCH v21 4/9] firmware: arm_rmm: Add support for SRO

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 01/10/2026 10:01, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] arm_rmm: spurious SRO cancellation when contig donate cache fills up
> - [Medium] arm_rmm: infinite loop in SRO execution if RMM consumes zero granules
> --
> 
> commit 11de55b0945f49bf7651f24a6bc346b286810815
> Author: Steven Price <steven.price@arm.com>
> 
> firmware: arm_rmm: Add support for SRO
> 
> The commit introduces support for Stateful RMI Operations (SRO) in RMM v2.0.
> It adds logic to handle memory donate and reclaim operations, allowing SMCs
> to return in progress and track state using an rmi_sro_state structure.
> Wrappers for SRO RMI commands are also provided to integrate with the stateful
> command infrastructure.
> 
>> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
>> index b558965a05a2c..7d4e04a5d7c0a 100644
>> --- a/drivers/firmware/arm_rmm/rmi.c
>> +++ b/drivers/firmware/arm_rmm/rmi.c
> [ ... ]
>> +static int rmi_sro_donate_contig(struct rmi_sro_state *sro,
>> +				 unsigned long sro_handle,
>> +				 unsigned long donatereq,
>> +				 struct arm_smccc_1_2_regs *out_regs,
>> +				 gfp_t gfp)
>> +{
> [ ... ]
>> +	/* Reuse the cached address range if we have one */
>> +	for (int i = 0; i < sro->addr_count; i++) {
>> +		unsigned long entry = sro->addr_list[i];
>> +
>> +		if (RMI_ADDR_RANGE_BLOCK_SIZE(entry) == block_size_fld &&
>> +		    RMI_ADDR_RANGE_COUNT(entry) == count &&
>> +		    RMI_ADDR_RANGE_STATE(entry) == state &&
>> +		    IS_ALIGNED(RMI_ADDR_RANGE_ADDR(entry), size)) {
>> +			sro->addr_count--;
>> +			swap(sro->addr_list[sro->addr_count],
>> +			     sro->addr_list[i]);
>> +
>> +			goto mem_donate;
>> +		}
>> +	}
>> +
>> +	ret = rmi_sro_ensure_capacity(sro, 1);
>> +	if (ret)
>> +		return ret;
> 
> [Severity: Medium]
> When the cache fills up with unconsumed fragments, rmi_sro_ensure_capacity()
> returns -ENOSPC. Does returning the error here cause a valid SRO command to be
> spuriously cancelled?


Agree, we could handle this better like we do for the noncontig case 
with the following hunk:

diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
index a01ab69a64c7c..0941cd8c34d66 100644
--- a/drivers/firmware/arm_rmm/rmi.c
+++ b/drivers/firmware/arm_rmm/rmi.c
@@ -343,8 +343,9 @@ static int rmi_sro_donate_contig(struct 
rmi_sro_state *sro,
         }

         ret = rmi_sro_ensure_capacity(sro, 1);
+       /* In the odd case we have filled up the cache, free up the 
entire list */
         if (ret)
-               return ret;
+               rmi_sro_free(sro);


> 
> Unlike rmi_sro_donate_noncontig() which handles a full cache by calling
> rmi_sro_free() to flush the unconsumed fragments, the contiguous path simply
> bails out. Could this be triggered if the firmware requests memory in patterns
> that fragment the cache?
> 
>> +
>> +	virt = alloc_pages_exact(size, gfp);
>> +	if (!virt)
>> +		return -ENOMEM;
> [ ... ]
>> +mem_donate:
>> +	rmi_op_mem_donate(sro_handle,
>> +			  virt_to_phys(&sro->addr_list[sro->addr_count]), 1,
>> +			  0, out_regs);
>> +	donated_size = out_regs->a1 << PAGE_SHIFT;
>> +
>> +	if (WARN_ON(out_regs->a1 > (size >> PAGE_SHIFT)))
>> +		donated_size = size;
>> +
>> +	/* All granules consumed by the RMM */
>> +	if (donated_size == size)
>> +		return 0;
>> +	/* No granules were consumed by the RMM, cache them */
>> +	if (donated_size == 0) {
>> +		sro->addr_count++;
>> +		return 0;
>> +	}
> 
> [Severity: Medium]
> If a buggy or interrupted RMM consumes 0 granules, does caching the block and
> returning 0 here create an infinite loop?
> 
> Because 0 is a success return code, the execution loop in
> rmi_sro_memxfer_execute() will skip the cancellation logic and re-issue the
> same request, as shown below:

RMM not consuming any granules is a valid case, where another thread of
host donated the memory that we were also trying to donate. Thus RMM
could report 0. Also, as for the infinite loop, there are other ways the
buggy RMM could trigger this. Keep on asking the host to continue
(forever) ! So, I wouldn't worry about this  pathological case.


Suzuki