Re: [PATCH v17 2/7] firmware: arm_rmm: Check for RMI support at init

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

On 08/09/2026 07:46, Gavin Shan wrote:
> Hi Suzuki,
> 
> On 9/7/26 7:59 PM, Suzuki K Poulose wrote:
>> From: Steven Price <steven.price@arm.com>
>>
>> Query the RMI version number and check if it is a compatible version.
>> The first two feature registers are read and exposed for future code to
>> use.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

...

>> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/ 
>> arm_rmm/rmi.c
>> new file mode 100644
>> index 0000000000000..008a783407b4e
>> --- /dev/null
>> +++ b/drivers/firmware/arm_rmm/rmi.c
>> @@ -0,0 +1,101 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023-2026 ARM Ltd.
>> + */
>> +
>> +#include <linux/cpufeature.h>
>> +#include <linux/memblock.h>
>> +#include <linux/arm-rmi-cmds.h>
>> +#include <linux/slab.h>
>> +
>> +#include <asm/memory.h>
>> +#include <asm/pgtable-hwdef.h>
>> +
>> +/* Currently only the first 2 registers are used by Linux */
>> +#define RMI_FEAT_REG_COUNT    2
>> +static __ro_after_init unsigned long 
>> rmi_feat_reg_cache[RMI_FEAT_REG_COUNT];
>> +
> 
> I would drop RMI_FEAT_REG_COUNT and use ARRAY_SIZE(rmi_feat_reg_cache) 
> in the code.
> Besides, __ro_after_init is usually put at the last the declaration.
> 
> static unsigned long rmi_feat_reg_cache[RMI_FEAT_REG_COUNT] 
> __ro_after_init;
> 
>> +unsigned long rmi_feat_reg(unsigned long id)
>> +{
>> +    if (WARN_ON(id >= RMI_FEAT_REG_COUNT))
>> +        return 0;
>> +
>> +    return rmi_feat_reg_cache[id];
>> +}
>> +EXPORT_SYMBOL_GPL(rmi_feat_reg);
>> +
> 
> I would suggest to rename 'id' to 'index' since it represents the feature
> register index instead of feature register ID.
> 
> unsigned long rmi_feat_reg(unsigned long index)
> {
>     :
> }
> EXPORT_SYMBOL_GPL(rmi_feat_reg);
> 


>> +/*
>> + * Note arm64_init_rmi() must be called before kvm_init_rmi() 
>> otherwise KVM
>> + * will not support realm guests. subsys_initcall() is called before
>> + * module_init() (used for KVM) so this is OK.
>> + */
>> +subsys_initcall(arm64_init_rmi);
>> diff --git a/include/linux/arm-rmi-cmds.h b/include/linux/arm-rmi-cmds.h
>> new file mode 100644
>> index 0000000000000..2fb1f7f86d71e

>> +static inline int rmi_features(unsigned long index, unsigned long *out)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    arm_smccc_1_1_invoke(SMC_RMI_FEATURES, index, &res);
>> +
>> +    if (res.a0 == RMI_SUCCESS && out)
>> +        *out = res.a1;
>> +
>> +    return res.a0;
>> +}
>> +
> 
> We needn't expose rmi_features() through include/linux/arm-rmi-cmds.h 
> since it's
> used for once in rmi.c::rmi_read_features(). I would suggest to drop it 
> by combining
> the code with that function.

Thanks for the review, I will address them in the next version.

Cheers
Suzuki