linux-kernel - Re: [PATCH v3 5/9] ACPI: APEI: EINJ: Enable the discovery of EINJv2 capabilities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250212164745.00000c5f@huawei.com>
Date: Wed, 12 Feb 2025 16:47:45 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Zaid Alali <zaidal@...amperecomputing.com>
CC: <rafael@...nel.org>, <lenb@...nel.org>, <james.morse@....com>,
	<tony.luck@...el.com>, <bp@...en8.de>, <robert.moore@...el.com>,
	<dan.j.williams@...el.com>, <Benjamin.Cheatham@....com>,
	<Avadhut.Naik@....com>, <viro@...iv.linux.org.uk>, <arnd@...db.de>,
	<ira.weiny@...el.com>, <dave.jiang@...el.com>,
	<sthanneeru.opensrc@...ron.com>, <linux-acpi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <acpica-devel@...ts.linux.dev>
Subject: Re: [PATCH v3 5/9] ACPI: APEI: EINJ: Enable the discovery of EINJv2
 capabilities

On Mon, 10 Feb 2025 10:37:01 -0800
Zaid Alali <zaidal@...amperecomputing.com> wrote:

> Enable the driver to show all supported error injections for EINJ
> and EINJv2 at the same time. EINJv2 capabilities can be discovered
> by checking the return value of get_error_type, where bit 30 set
> indicates EINJv2 support.
> 
> Signed-off-by: Zaid Alali <zaidal@...amperecomputing.com>
Hi Zaid,
A few comments inline.

Jonathan

> diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> index 0f65e8bc4c30..369d92e410c1 100644
> --- a/drivers/acpi/apei/einj-core.c
> +++ b/drivers/acpi/apei/einj-core.c

> @@ -649,6 +651,8 @@ static u64 error_param2;
>  static u64 error_param3;
>  static u64 error_param4;
>  static struct dentry *einj_debug_dir;
> +#define BUFF_SIZE	32

That's very generic name. I'd just put value inline and where
you use it currently just use sizeof(einj_buf) instead.

> +static char einj_buf[BUFF_SIZE];
>  static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>  	{ BIT(0), "Processor Correctable" },
>  	{ BIT(1), "Processor Uncorrectable non-fatal" },
> @@ -665,6 +669,12 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>  	{ BIT(31), "Vendor Defined Error Types" },
>  };
>  
> +static struct { u32 mask; const char *str; } const einjv2_error_type_string[] = {
> +	{ BIT(0), "EINJV2 Processor Error" },
> +	{ BIT(1), "EINJV2 Memory Error" },
> +	{ BIT(2), "EINJV2 PCI Express Error" },
> +};
> +
>  static int available_error_type_show(struct seq_file *m, void *v)
>  {
>  
> @@ -672,17 +682,21 @@ static int available_error_type_show(struct seq_file *m, void *v)
>  		if (available_error_type & einj_error_type_string[pos].mask)
>  			seq_printf(m, "0x%08x\t%s\n", einj_error_type_string[pos].mask,
>  				   einj_error_type_string[pos].str);
> -
> +	if (available_error_type & ACPI65_EINJV2_SUPP) {
> +		for (int pos = 0; pos < ARRAY_SIZE(einjv2_error_type_string); pos++)

Add some {} probably as multiple lines.  Coding style is a little unclear on this
but here I think it would help a little.

> +			if (available_error_type_v2 & einjv2_error_type_string[pos].mask)
> +				seq_printf(m, "V2_0x%08x\t%s\n", einjv2_error_type_string[pos].mask,
> +					   einjv2_error_type_string[pos].str);
> +	}
>  	return 0;
>  }

>  bool einj_is_cxl_error_type(u64 type)
> @@ -715,9 +729,22 @@ int einj_validate_error_type(u64 type)
>  	return 0;
>  }
>  
> -static int error_type_set(void *data, u64 val)
> +static ssize_t error_type_set(struct file *file, const char __user *buf,
> +				size_t count, loff_t *ppos)
>  {
>  	int rc;
> +	u64 val;
> +
> +	memset(einj_buf, 0, BUFF_SIZE);
> +	if (copy_from_user(einj_buf, buf, count))
> +		return -EFAULT;
> +
> +	if (strncmp(einj_buf, "V2_", 3) == 0) {
> +		if (!sscanf(einj_buf, "V2_%llx", &val))
> +			return -EINVAL;
> +	} else

Brackets in this leg as well.
Convention is put them in all legs + this one is also multiline so
should be here anyway.

> +		if (!sscanf(einj_buf, "%llx", &val))
> +			return -EINVAL;
>  
>  	rc = einj_validate_error_type(val);
>  	if (rc)
> @@ -725,11 +752,13 @@ static int error_type_set(void *data, u64 val)
>  
>  	error_type = val;
>  
> -	return 0;
> +	return count;
>  }
>  
> -DEFINE_DEBUGFS_ATTRIBUTE(error_type_fops, error_type_get, error_type_set,
> -			 "0x%llx\n");
> +static const struct file_operations error_type_fops = {
> +	.read		= error_type_get,
> +	.write		= error_type_set,
> +};
>  
>  static int error_inject_set(void *data, u64 val)
>  {
> @@ -737,7 +766,7 @@ static int error_inject_set(void *data, u64 val)
>  		return -EINVAL;
>  
>  	return einj_error_inject(error_type, error_flags, error_param1, error_param2,
> -		error_param3, error_param4);
> +				error_param3, error_param4);
I'd avoid reformatting in a patch doing anything else.
Hard to spot if real changes...

>  }
>