[V3] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()

Commit Message

Anshuman Khandual May 27, 2020, 10:04 a.m. UTC

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 <[e-mail]>
Cc: Will Deacon <[e-mail]>
Cc: Suzuki K Poulose <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Anshuman Khandual <[e-mail]>
---
Changes in V3:

- Early return on register search failure in some callers per Will
- Return 0, when search fails in check_update_ftr_reg() per Will

Changes in V2: (https://patchwork.kernel.org/patch/11570575/)

- 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 | 45 ++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 10 deletions(-)

Comments

Catalin Marinas May 27, 2020, 11:37 a.m. UTC | #1

On Wed, May 27, 2020 at 03:34:36PM +0530, Anshuman Khandual wrote:
> +/*
> + * get_arm64_ftr_reg - Looks up a feature register entry using
> + * its sys_reg() encoding. This calls get_arm64_ftr_reg_nowarn().
> + *
> + * returns - Upon success,  matching ftr_reg entry for id.
> + *         - NULL on failure but with an WARN_ON().
> + */
> +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
> +{
> +	struct arm64_ftr_reg *reg;
> +
> +	reg = get_arm64_ftr_reg_nowarn(sys_id);
> +
> +	/*
> +	 * Can not really proceed when the search fails here.
> +	 * Requesting for a non existent register search will
> +	 * be an error. Warn but let it continue for now.
> +	 */
> +	WARN_ON(!reg);
> +	return reg;

I find the comment here slightly confusing: cannot proceed but continue.
Maybe something like:

	/*
	 * Requesting a non-existent register search is an error. Warn
	 * and let the caller handle it.
	 */

Otherwise it looks fine:

Reviewed-by: Catalin Marinas <[e-mail]>

Will Deacon May 27, 2020, 12:25 p.m. UTC | #2

On Wed, 27 May 2020 15:34:36 +0530, 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.

Applied to arm64 (for-next/cpufeature), thanks!

[1/1] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()
      https://git.kernel.org/arm64/c/3577dd37c703

Cheers,

Anshuman Khandual May 28, 2020, 7:46 a.m. UTC | #3

On 05/27/2020 05:55 PM, Will Deacon wrote:
> On Wed, 27 May 2020 15:34:36 +0530, 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.
> 
> Applied to arm64 (for-next/cpufeature), thanks!
> 
> [1/1] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn()
>       https://git.kernel.org/arm64/c/3577dd37c703

Thanks Will, for changing the comment per Catalin.

Patch

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index e744f647cbac..64e87a7b3c3c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -597,16 +597,16 @@  static int search_cmp_ftr_reg(const void *id, const void *regp)
 }
 
 /*
- * get_arm64_ftr_reg - Lookup a feature register entry using its
- * sys_reg() encoding. With the array arm64_ftr_regs sorted in the
- * ascending order of sys_id , we use binary search to find a matching
+ * get_arm64_ftr_reg_nowarn - Looks up a feature register entry using
+ * its sys_reg() encoding. With the array arm64_ftr_regs sorted in the
+ * ascending order of sys_id, we use binary search to find a matching
  * entry.
  *
  * returns - Upon success,  matching ftr_reg entry for id.
  *         - NULL on failure. It is upto the caller to decide
  *	     the impact of a failure.
  */
-static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
+static struct arm64_ftr_reg *get_arm64_ftr_reg_nowarn(u32 sys_id)
 {
 	const struct __ftr_reg_entry *ret;
 
@@ -620,6 +620,28 @@  static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
 	return NULL;
 }
 
+/*
+ * get_arm64_ftr_reg - Looks up a feature register entry using
+ * its sys_reg() encoding. This calls get_arm64_ftr_reg_nowarn().
+ *
+ * returns - Upon success,  matching ftr_reg entry for id.
+ *         - NULL on failure but with an WARN_ON().
+ */
+static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
+{
+	struct arm64_ftr_reg *reg;
+
+	reg = get_arm64_ftr_reg_nowarn(sys_id);
+
+	/*
+	 * Can not really proceed when the search fails here.
+	 * Requesting for a non existent register search will
+	 * be an error. Warn but let it continue for now.
+	 */
+	WARN_ON(!reg);
+	return reg;
+}
+
 static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
 			       s64 ftr_val)
 {
@@ -681,7 +703,8 @@  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);
+	if (!reg)
+		return;
 
 	for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
 		u64 ftr_mask = arm64_ftr_mask(ftrp);
@@ -815,7 +838,9 @@  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);
+	if (!regp)
+		return 0;
+
 	update_cpu_ftr_reg(regp, val);
 	if ((boot & regp->strict_mask) == (val & regp->strict_mask))
 		return 0;
@@ -829,7 +854,7 @@  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))
+	if (!regp)
 		return;
 
 	for (ftrp = regp->ftr_bits; ftrp->width; ftrp++) {
@@ -1022,8 +1047,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;
 }
 
@@ -2663,7 +2688,7 @@  static int emulate_sys_reg(u32 id, u64 *valp)
 	if (sys_reg_CRm(id) == 0)
 		return emulate_id_reg(id, valp);
 
-	regp = get_arm64_ftr_reg(id);
+	regp = get_arm64_ftr_reg_nowarn(id);
 	if (regp)
 		*valp = arm64_ftr_reg_user_value(regp);
 	else