Re: [PATCH V2] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()

[Anshuman Khandual]

On 05/26/2020 07:34 PM, Suzuki K Poulose wrote:
> On 05/26/2020 02:39 PM, Anshuman Khandual wrote:
>> There is no way to proceed when requested register could not be searched in
>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
>> well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg()
>> rather than checking for return value and doing a BUG_ON() instead in some
>> individual callers. But there are also caller instances that dont error out
>> when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for
>> such cases.
>>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Cc: Mark Brown <broonie@xxxxxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> Changes in V2:
>>
>> - Added get_arm64_ftr_reg_nowarn() per Will
>> - read_sanitised_ftr_reg() returns 0 when register search fails per Catalin
>>
>> Changes in V1: (https://patchwork.kernel.org/patch/11559083/)
>>
>> Â arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++-----------
>> Â 1 file changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index bc5048f152c1..f4555b9d145c 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
>> Â }
>> Â 
> 
> ...
> 
>> Â static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ s64 ftr_val)
>> Â {
>> @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
>> ÂÂÂÂÂ const struct arm64_ftr_bits *ftrp;
>> ÂÂÂÂÂ struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>> Â -ÂÂÂ BUG_ON(!reg);
>> -
>> ÂÂÂÂÂ for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>> ÂÂÂÂÂÂÂÂÂ u64 ftr_mask = arm64_ftr_mask(ftrp);
>> ÂÂÂÂÂÂÂÂÂ s64 ftr_new = arm64_ftr_value(ftrp, new);
>> @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
>> Â {
>> ÂÂÂÂÂ struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>> Â -ÂÂÂ BUG_ON(!regp);
>> ÂÂÂÂÂ update_cpu_ftr_reg(regp, val);
>> ÂÂÂÂÂ if ((boot & regp->strict_mask) == (val & regp->strict_mask))
>> ÂÂÂÂÂÂÂÂÂ return 0;
>> @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field)
>> ÂÂÂÂÂ const struct arm64_ftr_bits *ftrp;
>> ÂÂÂÂÂ struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
>> Â -ÂÂÂ if (WARN_ON(!regp))
>> -ÂÂÂÂÂÂÂ return;
>> -
> 
> You need to return here, on !regp. Rest looks fine to me.

Catalin had suggested and agreed on for this change in behavior here.
If the register is not found, there is already some problem.