On Mon, 26 Feb 2024 16:27:02 -0600
Ben Cheatham <Benjamin.Cheatham@xxxxxxx> wrote:
> Remove CXL protocol error types from the EINJ module and move them to
> a new einj_cxl module. The einj_cxl module implements the necessary
> handling for CXL protocol error injection and exposes an API for the
> CXL core to use said functionality. Because the CXL error types
> require special handling, only allow them to be injected through the
> einj_cxl module and return an error when attempting to inject through
> "regular" EINJ.
>
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@xxxxxxx>
Hi Ben,
Some minor comments inline given you are doing a v15 (yikes!)
Jonathan
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj-core.c
> similarity index 94%
> rename from drivers/acpi/apei/einj.c
> rename to drivers/acpi/apei/einj-core.c
> index 937c69844dac..1a5f53d81d09 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj-core.c
...
> @@ -640,29 +648,21 @@ static int available_error_type_show(struct seq_file *m, void *v)
>
> DEFINE_SHOW_ATTRIBUTE(available_error_type);
>
> -static int error_type_get(void *data, u64 *val)
> -{
> - *val = error_type;
> -
> - return 0;
> -}
> -
> -static int error_type_set(void *data, u64 val)
> +int einj_validate_error_type(u64 type)
> {
> + u32 tval, vendor, available_error_type = 0;
> int rc;
> - u32 available_error_type = 0;
> - u32 tval, vendor;
>
> /* Only low 32 bits for error type are valid */
> - if (val & GENMASK_ULL(63, 32))
> + if (type & GENMASK_ULL(63, 32))
> return -EINVAL;
>
> /*
> * Vendor defined types have 0x80000000 bit set, and
> * are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
> */
> - vendor = val & ACPI5_VENDOR_BIT;
> - tval = val & 0x7fffffff;
> + vendor = type & ACPI5_VENDOR_BIT;
> + tval = type & GENMASK(30, 0);
>
> /* Only one error type can be specified */
> if (tval & (tval - 1))
> @@ -671,9 +671,37 @@ static int error_type_set(void *data, u64 val)
> rc = einj_get_available_error_type(&available_error_type);
> if (rc)
> return rc;
> - if (!(val & available_error_type))
> + if (!(type & available_error_type))
> return -EINVAL;
> }
> +
> + return 0;
> +}
> +
> +bool einj_is_cxl_error_type(u64 type)
> +{
> + return (type & CXL_ERROR_MASK) && (!(type & ACPI5_VENDOR_BIT));
> +}
> +
> +static int error_type_get(void *data, u64 *val)
This is reordered, but fairly sure no need to do so and it will
make patch cleaner to leave it above the validation code.
> +{
> + *val = error_type;
> +
> + return 0;
> +}
> +
> +static int error_type_set(void *data, u64 val)
> +{
> + int rc;
> +
> + /* CXL error types have to be injected from cxl debugfs */
> + if (einj_is_cxl_error_type(val))
> + return -EINVAL;
> +
> + rc = einj_validate_error_type(val);
Trivial but I'd have preferred this factoring out in a precursor patch
just to make reviewing this a tiny bit easier.
> + if (rc)
> + return rc;
> +
> error_type = val;
>
> return 0;
> diff --git a/drivers/acpi/apei/einj-cxl.c b/drivers/acpi/apei/einj-cxl.c
> new file mode 100644
> index 000000000000..34badc6a801e
> --- /dev/null
> +++ b/drivers/acpi/apei/einj-cxl.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * CXL Error INJection support. Used by CXL core to inject
> + * protocol errors into CXL ports.
> + *
> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
> + *
> + * Author: Ben Cheatham <benjamin.cheatham@xxxxxxx>
> + */
> +#include <linux/einj-cxl.h>
> +#include <linux/debugfs.h>
Follow include what you use principles a little closer
(there are always exceptions in kernel world...).
The following seems resonable to me.
#include <linux/array_size.h>
#include <linux/seq_file.h>
> +
> +#include "apei-internal.h"
> +
> +/* Defined in einj-core.c */
> +extern bool einj_initialized;
> +
> +static struct { u32 mask; const char *str; } const einj_cxl_error_type_string[] = {
> + { BIT(12), "CXL.cache Protocol Correctable" },
Use the defines for the bits? Not sure why the original code didn't do so other
than maybe long line lengths?
> + { BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
> + { BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
> + { BIT(15), "CXL.mem Protocol Correctable" },
> + { BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
> + { BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
> +};
> +
> +int einj_cxl_available_error_type_show(struct seq_file *m, void *v)
> +{
> + int cxl_err, rc;
> + u32 available_error_type = 0;
> +
> + if (!einj_initialized)
> + return -ENXIO;
> +
> + rc = einj_get_available_error_type(&available_error_type);
> + if (rc)
> + return rc;
> +
> + for (int pos = 0; pos < ARRAY_SIZE(einj_cxl_error_type_string); pos++) {
> + cxl_err = ACPI_EINJ_CXL_CACHE_CORRECTABLE << pos;
Hmm. This is a little ugly.
Could do something like the following bit it's of similar level of ugly
so up to you.
int bit_pos = ACPI_EINJ_CXL_CACHE_CORRECTABLE;
for_each_bit_set_bit_from(bit_pos, &available_error_type,
ARRAY_SIZE(einj_cxl_error_type_string)) {
int pos = bit_pos - ACPI_EINJ_CXL_CACHE_CORRECTABLE;
> +
> + if (available_error_type & cxl_err)
> + seq_printf(m, "0x%08x\t%s\n",
> + einj_cxl_error_type_string[pos].mask,
> + einj_cxl_error_type_string[pos].str);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(einj_cxl_available_error_type_show, CXL);
> diff --git a/include/linux/einj-cxl.h b/include/linux/einj-cxl.h
> new file mode 100644
> index 000000000000..4a1f4600539a
> --- /dev/null
> +++ b/include/linux/einj-cxl.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * CXL protocol Error INJection support.
> + *
> + * Copyright (c) 2023 Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Ben Cheatham <benjamin.cheatham@xxxxxxx>
> + */
> +#ifndef EINJ_CXL_H
> +#define EINJ_CXL_H
> +
> +#include <linux/pci.h>
Use a forwards def
struct pci_dev;
and drop the include. Also need
struct seq_file;
> +
> +#if IS_ENABLED(CONFIG_ACPI_APEI_EINJ_CXL)
> +int einj_cxl_available_error_type_show(struct seq_file *m, void *v);
> +int einj_cxl_inject_error(struct pci_dev *dport_dev, u64 type);
> +int einj_cxl_inject_rch_error(u64 rcrb, u64 type);
> +bool einj_cxl_is_initialized(void);
> +#else /* !IS_ENABLED(CONFIG_ACPI_APEI_EINJ_CXL) */
> +static inline int einj_cxl_available_error_type_show(struct seq_file *m,
> + void *v)
> +{
> + return -ENXIO;
> +}
> +
> +static inline int einj_cxl_inject_error(struct pci_dev *dport_dev, u64 type)
> +{
> + return -ENXIO;
> +}
> +
> +static inline int einj_cxl_inject_rch_error(u64 rcrb, u64 type)
> +{
> + return -ENXIO;
> +}
> +
> +static inline bool einj_cxl_is_initialized(void) { return false; }
> +#endif /* CONFIG_ACPI_APEI_EINJ_CXL */
> +
> +#endif /* EINJ_CXL_H */
