Re: [PATCH v2 1/5] RAS: Report all ARM processor CPER information to userspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Boris,

Em Thu, 29 Aug 2024 16:38:11 +0200
Borislav Petkov <bp@xxxxxxxxx> escreveu:

> On Thu, Jul 11, 2024 at 08:28:52AM +0200, Mauro Carvalho Chehab wrote:
> > In addition to those data, it also exports two fields that are
> > parsed by the GHES driver when firmware reports it, e. g.:
> > 
> > - error severity
> > - cpu logical index  
> 
> s/cpu/CPU/g
> 
> check your whole set pls.

Ok. Will address those at the hole series, sending you later today
a new version. Except for those, are patches 2-5 ok?

Regards,
Mauro

> > Report all of these information to userspace via trace uAPI, So that
> > userspace can properly record the error and take decisions related
> > to cpu core isolation according to error severity and other info.
> > 
> > After this patch, all the data from ARM Processor record from table  
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.

Usually, I don't use "this patch". In this specific case, I wanted
to bold the new fields that were added to the ARM trace event, making
clear that before the changeset, none of such fields exist; they were
added on such change. On other words, the keyword here is not patch,
but instead "After". Maybe I can replace it with "now", e. g.:

	The updated ARM trace event now contains the following fields:

	======================================	=============================
	UEFI field on table N.16		ARM Processor trace fields
	======================================	=============================
	Validation				handled when filling data for
						affinity MPIDR and running
						state.
	ERR_INFO_NUM				pei_len
	CONTEXT_INFO_NUM			ctx_len
	Section Length				indirectly reported by
						pei_len, ctx_len and oem_len
	Error affinity level			affinity
	MPIDR_EL1				mpidr
	MIDR_EL1				midr
	Running State				running_state
	PSCI State				psci_state
	Processor Error Information Structure	pei_err - count at pei_len
	Processor Context			ctx_err- count at ctx_len
	Vendor Specific Error Info		oem - count at oem_len
	======================================	=============================


> 
> ...
> 
> > [mchehab: modified patch description, solve merge conflicts and fix coding style]
> > Fixes: e9279e83ad1f ("trace, ras: add ARM processor error trace event")
> > Signed-off-by: Shengwei Luo <luoshengwei@xxxxxxxxxx>
> > Signed-off-by: Jason Tian <jason@xxxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>  
> 
> What is this SOB chain trying to tell me?
> 
> All those folks handled the patch?

They reflect what happened with past attempts of upstreaming this
change at the EDAC mailing list.

See, originally this seems to come from Jason Tian in 2021:
	https://lore.kernel.org/linux-edac/20210205022229.313030-1-jason@xxxxxxxxxxxxxxxxxxxxxx/
	https://lore.kernel.org/linux-edac/20210422084944.3718-1-jason@xxxxxxxxxxxxxxxxxxxxxx/
	https://lore.kernel.org/linux-edac/20210802135929.5283-1-shijie@xxxxxxxxxxxxxxxxxxxxxx/

In 2022, it came a new version from Shengwei Luo:
	https://lore.kernel.org/lkml/20220126030906.56765-1-lostway@xxxxxxxxxx/
	https://lore.kernel.org/linux-edac/20220214030813.135766-1-lostway@xxxxxxxxxx/

A new version from Daniel Ferguson arrived in 2023:
	https://lore.kernel.org/linux-edac/20231214232330.306526-2-danielf@xxxxxxxxxxxxxxxxxxxxxx/

Hard to reconstruct the entire history of this changeset, as there were
several attempts to fix it, and patches got renamed on some of such
attempts.

Anyway, it sounds that the custody chan can better be written as:

	Co-authored-by: Jason Tian <jason@xxxxxxxxxxxxxxxxxxxxxx>
	Co-authored-by: Signed-off-by: Shengwei Luo <luoshengwei@xxxxxxxxxx>
	Co-authored-by: Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>  
	Signed-off-by: Jason Tian <jason@xxxxxxxxxxxxxxxxxxxxxx>
	Signed-off-by: Shengwei Luo <luoshengwei@xxxxxxxxxx>
	Signed-off-by: Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>  

It probably makes sense to also indicate the original author of
it by change the "From" metadata to:

	From: Jason Tian <jason@xxxxxxxxxxxxxxxxxxxxxx>

> 
> > Tested-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-section
> > ---
> >  drivers/acpi/apei/ghes.c | 11 ++++-----
> >  drivers/ras/ras.c        | 45 +++++++++++++++++++++++++++++++++++--
> >  include/linux/ras.h      | 16 +++++++++++---
> >  include/ras/ras_event.h  | 48 +++++++++++++++++++++++++++++++++++-----
> >  4 files changed, 103 insertions(+), 17 deletions(-)  
> 
> ...
> 
> > -void log_arm_hw_error(struct cper_sec_proc_arm *err)
> > +void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev)
> >  {
> > -	trace_arm_event(err);
> > +	struct cper_arm_err_info *err_info;
> > +	struct cper_arm_ctx_info *ctx_info;
> > +	u8 *ven_err_data;
> > +	u32 ctx_len = 0;
> > +	int n, sz, cpu;
> > +	s32 vsei_len;
> > +	u32 pei_len;
> > +	u8 *pei_err;
> > +	u8 *ctx_err;
> > +
> > +	pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num;
> > +	pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm);
> > +
> > +	err_info = (struct cper_arm_err_info *)(err + 1);
> > +	ctx_info = (struct cper_arm_ctx_info *)(err_info + err->err_info_num);
> > +	ctx_err = (u8 *)ctx_info;
> > +	for (n = 0; n < err->context_info_num; n++) {
> > +		sz = sizeof(struct cper_arm_ctx_info) + ctx_info->size;
> > +		ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + sz);
> > +		ctx_len += sz;
> > +	}
> > +
> > +	vsei_len = err->section_length - (sizeof(struct cper_sec_proc_arm) +
> > +					  pei_len + ctx_len);
> > +	if (vsei_len < 0) {
> > +		pr_warn(FW_BUG
> > +			"section length: %d\n", err->section_length);
> > +		pr_warn(FW_BUG
> > +			"section length is too small\n");
> > +		pr_warn(FW_BUG
> > +			"firmware-generated error record is incorrect\n");  
> 
> No need to break those lines.
> 
> > +		vsei_len = 0;
> > +	}
> > +	ven_err_data = (u8 *)ctx_info;
> > +
> > +	cpu = GET_LOGICAL_INDEX(err->mpidr);
> > +	/* when return value is invalid, set cpu index to -1 */  
> 
> Obvious comment - no need for it.
> 

Will address at the next review.

Thanks,
Mauro




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux