diff mbox series

[3/5] riscv: alternatives: Rename errata_id to patch_id

Message ID 20230221185603.570882-4-ajones@ventanamicro.com (mailing list archive)
State Superseded
Headers show
Series riscv: alternative/cpufeature related cleanups | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 13 and now 13
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 2471 this patch: 2471
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 17334 this patch: 17334
conchuod/alphanumeric_selects success Out of order selects before the patch: 727 and now 727
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 2 this patch: 2
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch fail ERROR: Macros with complex values should be enclosed in parentheses WARNING: Avoid unnecessary line continuations
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK
conchuod/tree_selection fail Failed to apply to next/pending-fixes or riscv/for-next

Commit Message

Andrew Jones Feb. 21, 2023, 6:56 p.m. UTC 
Alternatives are used for both errata and cpufeatures. Use a more
generic name, 'patch_id', as in "ID of code patching site", to
avoid confusion when alternatives are used for cpufeatures.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/Kconfig                          |  6 +-
 arch/riscv/errata/sifive/errata.c           |  6 +-
 arch/riscv/errata/thead/errata.c            |  4 +-
 arch/riscv/include/asm/alternative-macros.h | 72 ++++++++++-----------
 arch/riscv/include/asm/alternative.h        |  2 +-
 arch/riscv/kernel/cpufeature.c              |  6 +-
 6 files changed, 48 insertions(+), 48 deletions(-)

Comments

Conor Dooley Feb. 23, 2023, 11:24 p.m. UTC | #1 
On Tue, Feb 21, 2023 at 07:56:01PM +0100, Andrew Jones wrote:
> Alternatives are used for both errata and cpufeatures. Use a more
> generic name, 'patch_id', as in "ID of code patching site", to
> avoid confusion when alternatives are used for cpufeatures.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/Kconfig                          |  6 +-
>  arch/riscv/errata/sifive/errata.c           |  6 +-
>  arch/riscv/errata/thead/errata.c            |  4 +-
>  arch/riscv/include/asm/alternative-macros.h | 72 ++++++++++-----------
>  arch/riscv/include/asm/alternative.h        |  2 +-
>  arch/riscv/kernel/cpufeature.c              |  6 +-
>  6 files changed, 48 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 736f42f572aa..0c494c36e911 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -377,9 +377,9 @@ config RISCV_ALTERNATIVE
>  	depends on !XIP_KERNEL
>  	help
>  	  This Kconfig allows the kernel to automatically patch the
> -	  errata required by the execution platform at run time. The
> -	  code patching is performed once in the boot stages. It means
> -	  that the overhead from this mechanism is just taken once.
> +	  erratum or cpufeature required by the execution platform at run
> +	  time. The code patching is performed once in the boot stages. It
> +	  means that the overhead from this mechanism is just taken once.

A line in this comment doesn't really make much sense to an unfamiliar
reader. "Once in the boot stages"? That's just not true, is it?
Code patching for alternatives happens more than once and not just
during boot? Not this patch's doing though!

> diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h
> index 3beef400a971..8f39d4e8598d 100644
> --- a/arch/riscv/include/asm/alternative.h
> +++ b/arch/riscv/include/asm/alternative.h
> @@ -38,7 +38,7 @@ struct alt_entry {
>  	s32 alt_offset;		/* offset relative to replacement instruction or data */
>  	u16 vendor_id;		/* cpu vendor id */
>  	u16 alt_len;		/* The replacement size */
> -	u32 errata_id;		/* The errata id */
> +	u32 patch_id;		/* The patch ID (erratum ID or cpufeature ID) */

The inconsistent use of ID versus id in these comments bothers my OCD!

Either way, "patch" is a more accurate description of the usage than
errata, so I am on board.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.
Andrew Jones Feb. 24, 2023, 8:06 a.m. UTC | #2 
On Thu, Feb 23, 2023 at 11:24:41PM +0000, Conor Dooley wrote:
> On Tue, Feb 21, 2023 at 07:56:01PM +0100, Andrew Jones wrote:
> > Alternatives are used for both errata and cpufeatures. Use a more
> > generic name, 'patch_id', as in "ID of code patching site", to
> > avoid confusion when alternatives are used for cpufeatures.
> > 
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/Kconfig                          |  6 +-
> >  arch/riscv/errata/sifive/errata.c           |  6 +-
> >  arch/riscv/errata/thead/errata.c            |  4 +-
> >  arch/riscv/include/asm/alternative-macros.h | 72 ++++++++++-----------
> >  arch/riscv/include/asm/alternative.h        |  2 +-
> >  arch/riscv/kernel/cpufeature.c              |  6 +-
> >  6 files changed, 48 insertions(+), 48 deletions(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 736f42f572aa..0c494c36e911 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -377,9 +377,9 @@ config RISCV_ALTERNATIVE
> >  	depends on !XIP_KERNEL
> >  	help
> >  	  This Kconfig allows the kernel to automatically patch the
> > -	  errata required by the execution platform at run time. The
> > -	  code patching is performed once in the boot stages. It means
> > -	  that the overhead from this mechanism is just taken once.
> > +	  erratum or cpufeature required by the execution platform at run
> > +	  time. The code patching is performed once in the boot stages. It
> > +	  means that the overhead from this mechanism is just taken once.
> 
> A line in this comment doesn't really make much sense to an unfamiliar
> reader. "Once in the boot stages"? That's just not true, is it?
> Code patching for alternatives happens more than once and not just
> during boot? Not this patch's doing though!

Right, code patching also happens at module load time. Reworking this
paragraph would be another nice cleanup. I can respin this series with
another patch doing that, along with picking up other suggestions you
have.

> 
> > diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h
> > index 3beef400a971..8f39d4e8598d 100644
> > --- a/arch/riscv/include/asm/alternative.h
> > +++ b/arch/riscv/include/asm/alternative.h
> > @@ -38,7 +38,7 @@ struct alt_entry {
> >  	s32 alt_offset;		/* offset relative to replacement instruction or data */
> >  	u16 vendor_id;		/* cpu vendor id */
> >  	u16 alt_len;		/* The replacement size */
> > -	u32 errata_id;		/* The errata id */
> > +	u32 patch_id;		/* The patch ID (erratum ID or cpufeature ID) */
> 
> The inconsistent use of ID versus id in these comments bothers my OCD!

I'll sneak a change in of vendor_id's comment to 'CPU vendor ID' for v2.

> 
> Either way, "patch" is a more accurate description of the usage than
> errata, so I am on board.
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
drew
Heiko Stuebner Feb. 24, 2023, 12:46 p.m. UTC | #3 
Am Dienstag, 21. Februar 2023, 19:56:01 CET schrieb Andrew Jones:
> Alternatives are used for both errata and cpufeatures. Use a more
> generic name, 'patch_id', as in "ID of code patching site", to
> avoid confusion when alternatives are used for cpufeatures.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

We moved away from alternatives only being used for erratas, so it makes
a lot of sense to name this sensible

Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 736f42f572aa..0c494c36e911 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -377,9 +377,9 @@  config RISCV_ALTERNATIVE
 	depends on !XIP_KERNEL
 	help
 	  This Kconfig allows the kernel to automatically patch the
-	  errata required by the execution platform at run time. The
-	  code patching is performed once in the boot stages. It means
-	  that the overhead from this mechanism is just taken once.
+	  erratum or cpufeature required by the execution platform at run
+	  time. The code patching is performed once in the boot stages. It
+	  means that the overhead from this mechanism is just taken once.
 
 config RISCV_ALTERNATIVE_EARLY
 	bool
diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c
index 9e7dffa800f2..3fe8bab5bb9e 100644
--- a/arch/riscv/errata/sifive/errata.c
+++ b/arch/riscv/errata/sifive/errata.c
@@ -100,12 +100,12 @@  void __init_or_module sifive_errata_patch_func(struct alt_entry *begin,
 	for (alt = begin; alt < end; alt++) {
 		if (alt->vendor_id != SIFIVE_VENDOR_ID)
 			continue;
-		if (alt->errata_id >= ERRATA_SIFIVE_NUMBER) {
-			WARN(1, "This errata id:%d is not in kernel errata list", alt->errata_id);
+		if (alt->patch_id >= ERRATA_SIFIVE_NUMBER) {
+			WARN(1, "This errata id:%d is not in kernel errata list", alt->patch_id);
 			continue;
 		}
 
-		tmp = (1U << alt->errata_id);
+		tmp = (1U << alt->patch_id);
 		if (cpu_req_errata & tmp) {
 			patch_text_nosync(ALT_OLD_PTR(alt), ALT_ALT_PTR(alt),
 					  alt->alt_len);
diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index 1dd90a5f86f0..39fc144f1b1f 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c
@@ -92,10 +92,10 @@  void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct al
 	for (alt = begin; alt < end; alt++) {
 		if (alt->vendor_id != THEAD_VENDOR_ID)
 			continue;
-		if (alt->errata_id >= ERRATA_THEAD_NUMBER)
+		if (alt->patch_id >= ERRATA_THEAD_NUMBER)
 			continue;
 
-		tmp = (1U << alt->errata_id);
+		tmp = (1U << alt->patch_id);
 		if (cpu_req_errata & tmp) {
 			oldptr = ALT_OLD_PTR(alt);
 			altptr = ALT_ALT_PTR(alt);
diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
index 51c6867e02f3..993a44a8fdac 100644
--- a/arch/riscv/include/asm/alternative-macros.h
+++ b/arch/riscv/include/asm/alternative-macros.h
@@ -6,18 +6,18 @@ 
 
 #ifdef __ASSEMBLY__
 
-.macro ALT_ENTRY oldptr newptr vendor_id errata_id new_len
+.macro ALT_ENTRY oldptr newptr vendor_id patch_id new_len
 	.4byte \oldptr - .
 	.4byte \newptr - .
 	.2byte \vendor_id
 	.2byte \new_len
-	.4byte \errata_id
+	.4byte \patch_id
 .endm
 
-.macro ALT_NEW_CONTENT vendor_id, errata_id, enable = 1, new_c : vararg
+.macro ALT_NEW_CONTENT vendor_id, patch_id, enable = 1, new_c : vararg
 	.if \enable
 	.pushsection .alternative, "a"
-	ALT_ENTRY 886b, 888f, \vendor_id, \errata_id, 889f - 888f
+	ALT_ENTRY 886b, 888f, \vendor_id, \patch_id, 889f - 888f
 	.popsection
 	.subsection 1
 888 :
@@ -33,7 +33,7 @@ 
 	.endif
 .endm
 
-.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable
+.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, patch_id, enable
 886 :
 	.option push
 	.option norvc
@@ -41,13 +41,13 @@ 
 	\old_c
 	.option pop
 887 :
-	ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c
+	ALT_NEW_CONTENT \vendor_id, \patch_id, \enable, \new_c
 .endm
 
-.macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1,	\
-				new_c_2, vendor_id_2, errata_id_2, enable_2
-	ALTERNATIVE_CFG "\old_c", "\new_c_1", \vendor_id_1, \errata_id_1, \enable_1
-	ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2
+.macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, patch_id_1, enable_1,	\
+				new_c_2, vendor_id_2, patch_id_2, enable_2
+	ALTERNATIVE_CFG "\old_c", "\new_c_1", \vendor_id_1, \patch_id_1, \enable_1
+	ALT_NEW_CONTENT \vendor_id_2, \patch_id_2, \enable_2, \new_c_2
 .endm
 
 #define __ALTERNATIVE_CFG(...)		ALTERNATIVE_CFG __VA_ARGS__
@@ -58,17 +58,17 @@ 
 #include <asm/asm.h>
 #include <linux/stringify.h>
 
-#define ALT_ENTRY(oldptr, newptr, vendor_id, errata_id, newlen)		\
+#define ALT_ENTRY(oldptr, newptr, vendor_id, patch_id, newlen)		\
 	".4byte	((" oldptr ") - .) \n"					\
 	".4byte	((" newptr ") - .) \n"					\
 	".2byte	" vendor_id "\n"					\
 	".2byte " newlen "\n"						\
-	".4byte	" errata_id "\n"
+	".4byte	" patch_id "\n"
 
-#define ALT_NEW_CONTENT(vendor_id, errata_id, enable, new_c)		\
+#define ALT_NEW_CONTENT(vendor_id, patch_id, enable, new_c)		\
 	".if " __stringify(enable) " == 1\n"				\
 	".pushsection .alternative, \"a\"\n"				\
-	ALT_ENTRY("886b", "888f", __stringify(vendor_id), __stringify(errata_id), "889f - 888f") \
+	ALT_ENTRY("886b", "888f", __stringify(vendor_id), __stringify(patch_id), "889f - 888f") \
 	".popsection\n"							\
 	".subsection 1\n"						\
 	"888 :\n"							\
@@ -83,7 +83,7 @@ 
 	".previous\n"							\
 	".endif\n"
 
-#define __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, enable)	\
+#define __ALTERNATIVE_CFG(old_c, new_c, vendor_id, patch_id, enable)	\
 	"886 :\n"							\
 	".option push\n"						\
 	".option norvc\n"						\
@@ -91,22 +91,22 @@ 
 	old_c "\n"							\
 	".option pop\n"							\
 	"887 :\n"							\
-	ALT_NEW_CONTENT(vendor_id, errata_id, enable, new_c)
+	ALT_NEW_CONTENT(vendor_id, patch_id, enable, new_c)
 
-#define __ALTERNATIVE_CFG_2(old_c, new_c_1, vendor_id_1, errata_id_1, enable_1,	\
-				   new_c_2, vendor_id_2, errata_id_2, enable_2)	\
-	__ALTERNATIVE_CFG(old_c, new_c_1, vendor_id_1, errata_id_1, enable_1)	\
-	ALT_NEW_CONTENT(vendor_id_2, errata_id_2, enable_2, new_c_2)
+#define __ALTERNATIVE_CFG_2(old_c, new_c_1, vendor_id_1, patch_id_1, enable_1,	\
+				   new_c_2, vendor_id_2, patch_id_2, enable_2)	\
+	__ALTERNATIVE_CFG(old_c, new_c_1, vendor_id_1, patch_id_1, enable_1)	\
+	ALT_NEW_CONTENT(vendor_id_2, patch_id_2, enable_2, new_c_2)
 
 #endif /* __ASSEMBLY__ */
 
-#define _ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, CONFIG_k)	\
-	__ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, IS_ENABLED(CONFIG_k))
+#define _ALTERNATIVE_CFG(old_c, new_c, vendor_id, patch_id, CONFIG_k)	\
+	__ALTERNATIVE_CFG(old_c, new_c, vendor_id, patch_id, IS_ENABLED(CONFIG_k))
 
-#define _ALTERNATIVE_CFG_2(old_c, new_c_1, vendor_id_1, errata_id_1, CONFIG_k_1,		\
-				  new_c_2, vendor_id_2, errata_id_2, CONFIG_k_2)		\
-	__ALTERNATIVE_CFG_2(old_c, new_c_1, vendor_id_1, errata_id_1, IS_ENABLED(CONFIG_k_1),	\
-				   new_c_2, vendor_id_2, errata_id_2, IS_ENABLED(CONFIG_k_2))
+#define _ALTERNATIVE_CFG_2(old_c, new_c_1, vendor_id_1, patch_id_1, CONFIG_k_1,		\
+				  new_c_2, vendor_id_2, patch_id_2, CONFIG_k_2)		\
+	__ALTERNATIVE_CFG_2(old_c, new_c_1, vendor_id_1, patch_id_1, IS_ENABLED(CONFIG_k_1),	\
+				   new_c_2, vendor_id_2, patch_id_2, IS_ENABLED(CONFIG_k_2))
 
 #else /* CONFIG_RISCV_ALTERNATIVE */
 #ifdef __ASSEMBLY__
@@ -137,19 +137,19 @@ 
 
 /*
  * Usage:
- *   ALTERNATIVE(old_content, new_content, vendor_id, errata_id, CONFIG_k)
+ *   ALTERNATIVE(old_content, new_content, vendor_id, patch_id, CONFIG_k)
  * in the assembly code. Otherwise,
- *   asm(ALTERNATIVE(old_content, new_content, vendor_id, errata_id, CONFIG_k));
+ *   asm(ALTERNATIVE(old_content, new_content, vendor_id, patch_id, CONFIG_k));
  *
  * old_content: The old content which is probably replaced with new content.
  * new_content: The new content.
  * vendor_id: The CPU vendor ID.
- * errata_id: The errata ID.
- * CONFIG_k: The Kconfig of this errata. When Kconfig is disabled, the old
+ * patch_id: The patch ID (erratum ID or cpufeature ID).
+ * CONFIG_k: The Kconfig of this patch ID. When Kconfig is disabled, the old
  *	     content will alwyas be executed.
  */
-#define ALTERNATIVE(old_content, new_content, vendor_id, errata_id, CONFIG_k) \
-	_ALTERNATIVE_CFG(old_content, new_content, vendor_id, errata_id, CONFIG_k)
+#define ALTERNATIVE(old_content, new_content, vendor_id, patch_id, CONFIG_k) \
+	_ALTERNATIVE_CFG(old_content, new_content, vendor_id, patch_id, CONFIG_k)
 
 /*
  * A vendor wants to replace an old_content, but another vendor has used
@@ -158,9 +158,9 @@ 
  * on the following sample code and then replace ALTERNATIVE() with
  * ALTERNATIVE_2() to append its customized content.
  */
-#define ALTERNATIVE_2(old_content, new_content_1, vendor_id_1, errata_id_1, CONFIG_k_1,		\
-				   new_content_2, vendor_id_2, errata_id_2, CONFIG_k_2)		\
-	_ALTERNATIVE_CFG_2(old_content, new_content_1, vendor_id_1, errata_id_1, CONFIG_k_1,	\
-					new_content_2, vendor_id_2, errata_id_2, CONFIG_k_2)
+#define ALTERNATIVE_2(old_content, new_content_1, vendor_id_1, patch_id_1, CONFIG_k_1,		\
+				   new_content_2, vendor_id_2, patch_id_2, CONFIG_k_2)		\
+	_ALTERNATIVE_CFG_2(old_content, new_content_1, vendor_id_1, patch_id_1, CONFIG_k_1,	\
+					new_content_2, vendor_id_2, patch_id_2, CONFIG_k_2)
 
 #endif
diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h
index 3beef400a971..8f39d4e8598d 100644
--- a/arch/riscv/include/asm/alternative.h
+++ b/arch/riscv/include/asm/alternative.h
@@ -38,7 +38,7 @@  struct alt_entry {
 	s32 alt_offset;		/* offset relative to replacement instruction or data */
 	u16 vendor_id;		/* cpu vendor id */
 	u16 alt_len;		/* The replacement size */
-	u32 errata_id;		/* The errata id */
+	u32 patch_id;		/* The patch ID (erratum ID or cpufeature ID) */
 };
 
 void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end,
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 21fb567e1b22..85c99ae64017 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -281,13 +281,13 @@  void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
 	for (alt = begin; alt < end; alt++) {
 		if (alt->vendor_id != 0)
 			continue;
-		if (alt->errata_id >= RISCV_ISA_EXT_MAX) {
+		if (alt->patch_id >= RISCV_ISA_EXT_MAX) {
 			WARN(1, "This extension id:%d is not in ISA extension list",
-				alt->errata_id);
+				alt->patch_id);
 			continue;
 		}
 
-		if (!__riscv_isa_extension_available(NULL, alt->errata_id))
+		if (!__riscv_isa_extension_available(NULL, alt->patch_id))
 			continue;
 
 		oldptr = ALT_OLD_PTR(alt);