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

[Andrew Jones <ajones@ventanamicro.com>]

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

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