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

From: Conor Dooley <conor@kernel.org>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: linux-riscv@lists.infradead.org,
	'Jisheng Zhang ' <jszhang@kernel.org>,
	'Heiko Stuebner ' <heiko@sntech.de>,
	'Paul Walmsley ' <paul.walmsley@sifive.com>,
	'Palmer Dabbelt ' <palmer@dabbelt.com>,
	'Conor Dooley ' <conor.dooley@microchip.com>,
	'Albert Ou ' <aou@eecs.berkeley.edu>
Subject: Re: [PATCH 3/5] riscv: alternatives: Rename errata_id to patch_id
Date: Thu, 23 Feb 2023 23:24:41 +0000	[thread overview]
Message-ID: <Y/f1uRERxTU5kAgh@spud> (raw)
In-Reply-To: <20230221185603.570882-4-ajones@ventanamicro.com>

[-- Attachment #1.1: Type: text/plain, Size: 2509 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

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