Re: [PATCH v2 5/9] ACPI: APEI: EINJ: Enable the discovery of EINJv2 capabilities

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu,  5 Dec 2024 13:18:50 -0800
Zaid Alali <zaidal@os.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.
> 
> This update makes the driver parse the error_type as a string to
> avoid any ambiguity with EINJv1 and EINJv2 error types that has
> the same value, where EINJv2 error types has the prefix "V2_".
> 
> Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com>
Hi Zaid,

Some comments inline.

Thanks,

Jonathan

> ---
>  drivers/acpi/apei/apei-internal.h |  2 +-
>  drivers/acpi/apei/einj-core.c     | 70 ++++++++++++++++++++++++-------
>  drivers/acpi/apei/einj-cxl.c      |  2 +-
>  3 files changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index cd2766c69d78..9a3dbaeed39a 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -131,7 +131,7 @@ static inline u32 cper_estatus_len(struct acpi_hest_generic_status *estatus)
>  
>  int apei_osc_setup(void);
>  
> -int einj_get_available_error_type(u32 *type);
> +int einj_get_available_error_type(u32 *type, int version);

As below. I'm not sure version is a good name for this as it
is not the version number at all.

>  int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
>  		      u64 param4);
>  int einj_cxl_rch_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
> diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> index a6b648361d96..2c57e25252ac 100644
> --- a/drivers/acpi/apei/einj-core.c
> +++ b/drivers/acpi/apei/einj-core.c

> @@ -641,6 +643,7 @@ static u64 error_param2;
>  static u64 error_param3;
>  static u64 error_param4;
>  static struct dentry *einj_debug_dir;
> +static char *einj_buf;
>  static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>  	{ BIT(0), "Processor Correctable" },
>  	{ BIT(1), "Processor Uncorrectable non-fatal" },
> @@ -656,6 +659,11 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>  	{ BIT(11), "Platform Uncorrectable fatal"},
>  	{ BIT(31), "Vendor Defined Error Types" },
>  };

blank line here.

> +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)
>  {
> @@ -663,18 +671,22 @@ static int available_error_type_show(struct seq_file *m, void *v)
>  	for (int pos = 0; pos < ARRAY_SIZE(einj_error_type_string); pos++)
>  		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);
> -
> +				einj_error_type_string[pos].str);

Fix this up and check for any other accidental changes like this. They just
make the patches harder to review.


> +	if (available_error_type & ACPI65_EINJV2_SUPP) {
> +		for (int pos = 0; pos < ARRAY_SIZE(einjv2_error_type_string); pos++)
> +			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,

Long line. I'd wrap before last parameter for readability.

> +					einjv2_error_type_string[pos].str);

Align after closing bracket.

> +	}
>  	return 0;
>  }
>  
>  DEFINE_SHOW_ATTRIBUTE(available_error_type);
>  
> -static int error_type_get(void *data, u64 *val)
> +static ssize_t error_type_get(struct file *file, char __user *buf,
> +				size_t count, loff_t *ppos)
>  {
> -	*val = error_type;
> -
> -	return 0;
> +	return simple_read_from_buffer(buf, count, ppos, einj_buf, strlen(einj_buf));
>  }
>  
>  bool einj_is_cxl_error_type(u64 type)
> @@ -701,15 +713,28 @@ int einj_validate_error_type(u64 type)
>  	if (tval & (tval - 1))
>  		return -EINVAL;
>  	if (!vendor)
> -		if (!(type & available_error_type))
> +		if (!(type & (available_error_type | available_error_type_v2)))

Maybe a comment on this. Not obvious to me which the | makes sense.

>  			return -EINVAL;
>  
>  	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, sizeof(einj_buf));

sizeof the pointer?

> +	if (copy_from_user(einj_buf, buf, count))

What stops this being bigger than einj_buf?  Perhaps
best to check that.

> +		return -EFAULT;
> +
> +	if (strncmp(einj_buf, "V2_", 3) == 0) {
> +		if (!sscanf(einj_buf, "V2_%llx", &val))
> +			return -EINVAL;
> +	} else
	} else {

Both because you kernel style is same bracketing for all legs
of if / else and because what follows is multi line.

> +		if (!sscanf(einj_buf, "%llx", &val))
> +			return -EINVAL;
>  
>  	rc = einj_validate_error_type(val);
>  	if (rc)
> @@ -717,11 +742,13 @@ static int error_type_set(void *data, u64 val)
>  
>  	error_type = val;
>  
> -	return 0;
> +	return count;
>  }

>  static int error_inject_set(void *data, u64 val)
>  {
> @@ -778,9 +805,14 @@ static int __init einj_probe(struct platform_device *pdev)
>  		goto err_put_table;
>  	}
>  
> -	rc = einj_get_available_error_type(&available_error_type);
> +	rc = einj_get_available_error_type(&available_error_type, ACPI_EINJ_GET_ERROR_TYPE);
>  	if (rc)
>  		return rc;
> +	if (available_error_type & ACPI65_EINJV2_SUPP) {
> +		rc = einj_get_available_error_type(&available_error_type_v2, ACPI_EINJV2_GET_ERROR_TYPE);
The parameter is called version. I'd expect that to just be 1 or 2 giving naming.
Maybe a different parameter name would be less confusing?

> +		if (rc)
> +			return rc;
> +	}
>  
>  	rc = -ENOMEM;
>  	einj_debug_dir = debugfs_create_dir("einj", apei_get_debugfs_dir());
> @@ -828,6 +860,11 @@ static int __init einj_probe(struct platform_device *pdev)
>  				   einj_debug_dir, &notrigger);
>  	}
>  
> +	einj_buf = kzalloc(32, GFP_KERNEL);

Why 32? Can we base that on a define or similar?
Given it is global anyway and fairly small, why not just declare
a static array and skip the allocation and free?


> +	if (!einj_buf) {
> +		goto err_release;

Not sure on local style, but general kernel style is no brackets for single line if block.

> +	}
> +
>  	if (vendor_dev[0]) {
>  		vendor_blob.data = vendor_dev;
>  		vendor_blob.size = strlen(vendor_dev);
> @@ -875,6 +912,7 @@ static void __exit einj_remove(struct platform_device *pdev)
>  	apei_resources_fini(&einj_resources);
>  	debugfs_remove_recursive(einj_debug_dir);
>  	acpi_put_table((struct acpi_table_header *)einj_tab);
> +	kfree(einj_buf);
>  }
>  

>  	if (rc)
>  		return rc;
>