Hey Jonathan, thanks for taking a look!
On 3/7/24 6:09 AM, Jonathan Cameron wrote:
> 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!)
>
Yeah I know :(.
> 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.
>
Sorry it's been a bit, but I think I moved it to go with the other error_type
functions. I don't mind leaving it where it was though.
>> +{
>> + *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.
>
It would probably make sense, but at this point I just want to get this
across the finish line :).
>
>> + 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>
>
Will do!
>
>> +
>> +#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?
>
I agree, this was merged from some ACPI tree changes in rc4 and I don't think
they have the defines in that tree yet.
>
>
>> + { 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;
>
I agree it's ugly. I think this version has the added benfit of parity
with einj_available_error_type_show() in einj-core.c, so I think it's
better to keep it this way if it's the same to you.
>
>
>> +
>> + 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;
>
Will do.
>
>> +
>> +#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 */
>
