All of lore.kernel.org
 help / color / mirror / Atom feed
From: Catalin Marinas <catalin.marinas@arm.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: mark.rutland@arm.com, Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
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

  reply	other threads:[~2020-05-26 12:19 UTC|newest]

Thread overview: 22+ 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  1:22 ` Anshuman Khandual
2020-05-20 11:49 ` Catalin Marinas
2020-05-20 11:49   ` Catalin Marinas
2020-05-20 12:20 ` Will Deacon
2020-05-20 12:20   ` Will Deacon
2020-05-20 15:47   ` Catalin Marinas
2020-05-20 15:47     ` Catalin Marinas
2020-05-20 17:39     ` Will Deacon
2020-05-20 17:39       ` Will Deacon
2020-05-21  3:15       ` Anshuman Khandual
2020-05-21  3:15         ` Anshuman Khandual
2020-05-21 16:22         ` Will Deacon
2020-05-21 16:22           ` Will Deacon
2020-05-21 16:59           ` Catalin Marinas
2020-05-21 16:59             ` Catalin Marinas
2020-05-24 23:52             ` Anshuman Khandual
2020-05-24 23:52               ` Anshuman Khandual
2020-05-26 12:19               ` Catalin Marinas [this message]
2020-05-26 12:19                 ` Catalin Marinas
2020-05-21  3:12   ` Anshuman Khandual
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.