Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Catalin Marinas]

On Mon, May 25, 2020 at 05:22:23AM +0530, Anshuman Khandual wrote:
> On 05/21/2020 10:29 PM, Catalin Marinas wrote:
> > On Thu, May 21, 2020 at 05:22:15PM +0100, Will Deacon wrote:
> >> On Thu, May 21, 2020 at 08:45:38AM +0530, Anshuman Khandual wrote:
> >>> The existing code has BUG_ON() in three different callers doing exactly the
> >>> same thing that can easily be taken care in get_arm64_ftr_reg() itself. As
> >>> mentioned before an enum variable (as preferred - over a bool) can still
> >>> preserve the existing behavior for emulate_sys_reg().
> >>>
> >>> IMHO these are very good reasons for us to change the code which will make
> >>> it cleaner while also removing three redundant BUG_ON() instances. Hence I
> >>> will request you to please reconsider this proposal.
> >>
> >> Hmm, then how about trying my proposal with the WARN_ON(), but having a
> >> get_arm64_ftr_reg_nowarn() variant for the user emulation case?
[...]
> > read_sanitised_ftr_reg() would need to return something though. Would
> > all 0s be ok? I think it works as long as we don't have negative CPUID
> > fields.
> 
> Just trying to understand. If get_arm64_ftr_reg() returns NULL, then
> read_sanitised_ftr_reg() should also return 0 for that non existent
> register (already been warned in get_arm64_ftr_reg).
> 
> @@ -961,8 +972,8 @@ u64 read_sanitised_ftr_reg(u32 id)
>  {
>         struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id);
> 
> -       /* We shouldn't get a request for an unsupported register */
> -       BUG_ON(!regp);
> +       if (!regp)
> +               return 0;
>         return regp->sys_val;
>  }

Yes, as long as we also get the WARN_ON().

-- 
Catalin