From: Catalin Marinas <catalin.marinas@arm.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org, mark.rutland@arm.com,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Mark Brown <broonie@kernel.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()
Date: Tue, 26 May 2020 13:19:03 +0100 [thread overview]
Message-ID: <20200526121903.GF17051@gaia> (raw)
In-Reply-To: <aa6436d4-c724-4933-1341-6ca417ce40ed@arm.com>
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
next prev parent reply other threads:[~2020-05-26 12:19 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-20 1:22 [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg() Anshuman Khandual
2020-05-20 11:49 ` Catalin Marinas
2020-05-20 12:20 ` Will Deacon
2020-05-20 15:47 ` Catalin Marinas
2020-05-20 17:39 ` Will Deacon
2020-05-21 3:15 ` Anshuman Khandual
2020-05-21 16:22 ` Will Deacon
2020-05-21 16:59 ` Catalin Marinas
2020-05-24 23:52 ` Anshuman Khandual
2020-05-26 12:19 ` Catalin Marinas [this message]
2020-05-21 3:12 ` Anshuman Khandual
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200526121903.GF17051@gaia \
--to=catalin.marinas@arm.com \
--cc=anshuman.khandual@arm.com \
--cc=broonie@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=suzuki.poulose@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).