On Tue, Oct 22, 2024 at 02:34:28PM -0700, Zaid Alali wrote:
> Enable the driver to inject EINJv2 type errors. The component
> array values are parsed from user_input and expected to contain
> hex values for component id and syndrome separated by space,
> and multiple components are separated by new line as follows:
>
> component_id1 component_syndrome1
> component_id2 component_syndrome2
> :
> component_id(n) component_syndrome(n)
>
> for example:
>
> $comp_arr="0x1 0x2
> >0x1 0x4
> >0x2 0x4"
> $cd /sys/kernel/debug/apei/einj/
> $echo "$comp_arr" > einjv2_component_array
>
> Signed-off-by: Zaid Alali <zaidal@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/acpi/apei/einj-core.c | 76 +++++++++++++++++++++++++++++++++--
> 1 file changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> index bd46a611eef7..bc833f42dfc7 100644
> --- a/drivers/acpi/apei/einj-core.c
> +++ b/drivers/acpi/apei/einj-core.c
> @@ -87,6 +87,13 @@ enum {
> SETWA_FLAGS_APICID = 1,
> SETWA_FLAGS_MEM = 2,
> SETWA_FLAGS_PCIE_SBDF = 4,
> + SETWA_FLAGS_EINJV2 = 8,
> +};
> +
> +enum {
> + EINJV2_PROCESSOR_ERROR = 0x1,
> + EINJV2_MEMORY_ERROR = 0x2,
> + EINJV2_PCIE_ERROR = 0x4,
> };
>
> /*
> @@ -111,6 +118,7 @@ static char vendor_dev[64];
> static struct debugfs_blob_wrapper einjv2_component_arr;
> static u64 component_count;
> static void *user_input;
> +static int nr_components;
> static u32 available_error_type;
> static u32 available_error_type_v2;
>
> @@ -287,8 +295,18 @@ static void *einj_get_parameter_address(void)
>
> v5param = acpi_os_map_iomem(pa_v5, sizeof(*v5param));
> if (v5param) {
> + int offset, len;
> +
> acpi5 = 1;
> check_vendor_extension(pa_v5, v5param);
> + if (available_error_type & ACPI65_EINJV2_SUPP) {
> + len = v5param->einjv2_struct.length;
> + offset = offsetof(struct einjv2_extension_struct, component_arr);
> + nr_components = (len - offset) / 32;
> + acpi_os_unmap_iomem(v5param, sizeof(*v5param));
> + v5param = acpi_os_map_iomem(pa_v5, sizeof(*v5param) + (
> + (nr_components) * sizeof(struct syndrome_array)));
The way this line is broken up doesn't look quite right. That paren on
the top line should get pulled on to the next line and aligned with the
beginning of acpi_os_unmap_iomem. See below example.
Also, it's a little awkward here to map the v5param above just to unmap it
here in the case of EINJv2. Is there a reason it needs to be done like
this or can we do something like this instead?
if (available_error_type & ACPI65_EINJV2_SUPP) {
len = v5param->einjv2_struct.length;
offset = offsetof(struct einjv2_extension_struct, component_arr);
nr_components = (len - offset) / 32;
v5param = acpi_os_map_iomem(pa_v5, sizeof(*v5param) +
((nr_components) * sizeof(struct syndrome_array)));
else {
v5param = acpi_os_map_iomem(pa_v5, sizeof(*v5param));
}
if (v5param) {
acpi5 = 1;
check_vendor_extension(pa_v5, v5param);
return v5param;
}
> + }
> return v5param;
> }
> }
> @@ -496,8 +514,49 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
> v5param->flags = flags;
> v5param->memory_address = param1;
> v5param->memory_address_range = param2;
> - v5param->apicid = param3;
> - v5param->pcie_sbdf = param4;
> +
> + if (flags & SETWA_FLAGS_EINJV2) {
IMO, moving this chunk inside the conditional here to a helper function
would improve readability. As written, there are a couple too many
levels of indentation.
> + int count = 0, bytes_read, pos = 0;
> + unsigned int comp, synd;
> + struct syndrome_array *component_arr;
> +
> + if (component_count > nr_components)
> + goto err_out;
> +
> + v5param->einjv2_struct.component_arr_count = component_count;
> + component_arr = v5param->einjv2_struct.component_arr;
> +
> + while (sscanf(user_input+pos, "%x %x\n%n", &comp, &synd,
Arithmetic operators should have a single space on each side:
user_input + pos
> + &bytes_read) == 2) {
Another alignment issue here. This would be nice to have aligned after
the sscanf paren. Like:
while (sscanf(user_input+pos, "%x %x\n%n", &comp, &synd,
&bytes_read) == 2) {
Thanks,
John
> + pos += bytes_read;
> + if (count > component_count)
> + goto err_out;
> +
> + switch (type) {
> + case EINJV2_PROCESSOR_ERROR:
> + component_arr[count].comp_id.acpi_id = comp;
> + component_arr[count].comp_synd.proc_synd = synd;
> + break;
> + case EINJV2_MEMORY_ERROR:
> + component_arr[count].comp_id.device_id = comp;
> + component_arr[count].comp_synd.mem_synd = synd;
> + break;
> + case EINJV2_PCIE_ERROR:
> + component_arr[count].comp_id.pcie_sbdf = comp;
> + component_arr[count].comp_synd.pcie_synd = synd;
> + break;
> + }
> + count++;
> + }
> + if (count != component_count - 1)
> + goto err_out;
> +
> + /* clear buffer after user input for next injection */
> + memset(user_input, 0, COMP_ARR_SIZE);
> + } else {
> + v5param->apicid = param3;
> + v5param->pcie_sbdf = param4;
> + }
> } else {
> switch (type) {
> case ACPI_EINJ_PROCESSOR_CORRECTABLE:
> @@ -570,6 +629,9 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
> rc = apei_exec_run_optional(&ctx, ACPI_EINJ_END_OPERATION);
>
> return rc;
> +err_out:
> + memset(user_input, 0, COMP_ARR_SIZE);
> + return -EINVAL;
> }
>
> /* Inject the specified hardware error */
> @@ -581,9 +643,14 @@ int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
>
> /* If user manually set "flags", make sure it is legal */
> if (flags && (flags &
> - ~(SETWA_FLAGS_APICID|SETWA_FLAGS_MEM|SETWA_FLAGS_PCIE_SBDF)))
> + ~(SETWA_FLAGS_APICID|SETWA_FLAGS_MEM|SETWA_FLAGS_PCIE_SBDF|SETWA_FLAGS_EINJV2)))
> return -EINVAL;
>
> + /*check if type is a valid EINJv2 error type*/
> + if (flags & SETWA_FLAGS_EINJV2) {
> + if (!(type & available_error_type_v2))
> + return -EINVAL;
> + }
> /*
> * We need extra sanity checks for memory errors.
> * Other types leap directly to injection.
> @@ -913,7 +980,8 @@ static void __exit einj_remove(struct platform_device *pdev)
> sizeof(struct set_error_type_with_address) :
> sizeof(struct einj_parameter);
>
> - acpi_os_unmap_iomem(einj_param, size);
> + acpi_os_unmap_iomem(einj_param,
> + size + (nr_components * sizeof(struct syndrome_array)));
> if (vendor_errors.size)
> acpi_os_unmap_memory(vendor_errors.data, vendor_errors.size);
> }
> --
> 2.34.1
>
