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