| 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
| ||
|
Message-ID: <CAAhV-H6+c8sJySdz0QuXWQ_XzXjLZ1WTnN7CimBf-c7U-5JzUQ@mail.gmail.com>
Date: Wed, 13 Nov 2024 12:14:27 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Zhao Qunqin <zhaoqunqin@...ngson.cn>
Cc: kernel@...0n.name, bp@...en8.de, tony.luck@...el.com, james.morse@....com,
mchehab@...nel.org, rric@...nel.org, linux-edac@...r.kernel.org,
linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev, xry111@...111.site,
Markus.Elfring@....de, krzk@...nel.org
Subject: Re: [PATCH V8] EDAC: Add EDAC driver for loongson memory controller
Hi, Qunqin
On Wed, Nov 13, 2024 at 11:23 AM Zhao Qunqin <zhaoqunqin@...ngson.cn> wrote:
>
> Add ECC support for Loongson SoC DDR controller. This
> driver reports single bit errors (CE) only.
>
> Only ACPI firmware is supported.
Fair enough, if there is only ACPI firmware support EDAC, we don't
need to bother FDT.
>
> Signed-off-by: Zhao Qunqin <zhaoqunqin@...ngson.cn>
> ---
> Changes in v8:
> - Used readl() instead of readq()
> - Used acpi_device_id instead of of_device_id, then removed
> dt-bindings
>
> Changes in v7:
> - Fixed sparse's "incorrect type in assignment"
> - Cleaned up coding style
>
> Changes in v6:
> - Changed the Kconfig name to CONFIG_EDAC_LOONGSON
>
> Changes in v5:
> - Dropepd the loongson_ prefix from all static functions.
> - Aligned function arguments on the opening brace.
> - Dropepd useless comments and useless wrapper. Dropped side
> comments.
> - Reordered variable declarations.
>
> Changes in v4:
> - None
>
> Changes in v3:
> - Addressed review comments raised by Krzysztof and Huacai
>
> Changes in v2:
> - Addressed review comments raised by Krzysztof
>
> MAINTAINERS | 6 ++
> arch/loongarch/Kconfig | 1 +
> drivers/edac/Kconfig | 8 ++
> drivers/edac/Makefile | 1 +
> drivers/edac/loongson_edac.c | 155 +++++++++++++++++++++++++++++++++++
> 5 files changed, 171 insertions(+)
> create mode 100644 drivers/edac/loongson_edac.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9659a5a7..b36a45051 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13397,6 +13397,12 @@ S: Maintained
> F: Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> F: drivers/thermal/loongson2_thermal.c
>
> +LOONGSON EDAC DRIVER
> +M: Zhao Qunqin <zhaoqunqin@...ngson.cn>
> +L: linux-edac@...r.kernel.org
> +S: Maintained
> +F: drivers/edac/loongson_edac.c
> +
> LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
> M: Sathya Prakash <sathya.prakash@...adcom.com>
> M: Sreekanth Reddy <sreekanth.reddy@...adcom.com>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index bb35c34f8..10b9ba587 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -185,6 +185,7 @@ config LOONGARCH
> select PCI_MSI_ARCH_FALLBACKS
> select PCI_QUIRKS
> select PERF_USE_VMALLOC
> + select EDAC_SUPPORT
Please use alpha-betical order, which means "select EDAC_SUPPORT"
should be before "select EFI".
> select RTC_LIB
> select SPARSE_IRQ
> select SYSCTL_ARCH_UNALIGN_ALLOW
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 81af6c344..4dce2b92a 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -564,5 +564,13 @@ config EDAC_VERSAL
> Support injecting both correctable and uncorrectable errors
> for debugging purposes.
>
> +config EDAC_LOONGSON
> + tristate "Loongson Memory Controller"
> + depends on (LOONGARCH && ACPI) || COMPILE_TEST
> + help
> + Support for error detection and correction on the Loongson
> + family memory controller. This driver reports single bit
> + errors (CE) only. Loongson-3A5000/3C5000/3D5000/3A6000/3C6000
> + are compatible.
>
> endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index faf310eec..f8bdbc895 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -88,3 +88,4 @@ obj-$(CONFIG_EDAC_DMC520) += dmc520_edac.o
> obj-$(CONFIG_EDAC_NPCM) += npcm_edac.o
> obj-$(CONFIG_EDAC_ZYNQMP) += zynqmp_edac.o
> obj-$(CONFIG_EDAC_VERSAL) += versal_edac.o
> +obj-$(CONFIG_EDAC_LOONGSON) += loongson_edac.o
> diff --git a/drivers/edac/loongson_edac.c b/drivers/edac/loongson_edac.c
> new file mode 100644
> index 000000000..340722db1
> --- /dev/null
> +++ b/drivers/edac/loongson_edac.c
> @@ -0,0 +1,155 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Loongson Technology Corporation Limited.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/edac.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
Use alpha-betical order too, which means exchange module.h and init.h.
> +#include <linux/platform_device.h>
> +#include "edac_module.h"
> +
> +#define ECC_CS_COUNT_REG 0x18
> +
> +struct loongson_edac_pvt {
> + void __iomem *ecc_base;
> + int last_ce_count;
> +};
> +
> +static int read_ecc(struct mem_ctl_info *mci)
> +{
> + struct loongson_edac_pvt *pvt = mci->pvt_info;
> + u32 ecc;
> + int cs;
> +
> + if (!pvt->ecc_base)
> + return pvt->last_ce_count;
> +
> + ecc = readl(pvt->ecc_base + ECC_CS_COUNT_REG);
If the register is actually 64bit, it is better to use readq, and add
"depends on 64BIT" after config EDAC_LOONGSON.
> + /* cs0 -- cs3 */
> + cs = ecc & 0xff;
> + cs += (ecc >> 8) & 0xff;
> + cs += (ecc >> 16) & 0xff;
> + cs += (ecc >> 24) & 0xff;
> +
> + return cs;
> +}
> +
> +static void edac_check(struct mem_ctl_info *mci)
> +{
> + struct loongson_edac_pvt *pvt = mci->pvt_info;
> + int new, add;
> +
> + new = read_ecc(mci);
> + add = new - pvt->last_ce_count;
> + pvt->last_ce_count = new;
> + if (add <= 0)
> + return;
> +
> + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, add,
> + 0, 0, 0, 0, 0, -1, "error", "");
> + edac_mc_printk(mci, KERN_INFO, "add: %d", add);
> +}
> +
> +static void dimm_config_init(struct mem_ctl_info *mci)
> +{
> + struct dimm_info *dimm;
> + u32 size, npages;
> +
> + /* size not used */
> + size = -1;
> + npages = MiB_TO_PAGES(size);
> +
> + dimm = edac_get_dimm(mci, 0, 0, 0);
> + dimm->nr_pages = npages;
> + snprintf(dimm->label, sizeof(dimm->label),
> + "MC#%uChannel#%u_DIMM#%u", mci->mc_idx, 0, 0);
> + dimm->grain = 8;
> +}
> +
> +static void pvt_init(struct mem_ctl_info *mci, void __iomem *vbase)
> +{
> + struct loongson_edac_pvt *pvt = mci->pvt_info;
> +
> + pvt->ecc_base = vbase;
> + pvt->last_ce_count = read_ecc(mci);
> +}
> +
> +static int edac_probe(struct platform_device *pdev)
> +{
> + struct edac_mc_layer layers[2];
> + struct mem_ctl_info *mci;
> + void __iomem *vbase;
> + int ret;
> +
> + vbase = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(vbase))
> + return PTR_ERR(vbase);
> +
> + /* allocate a new MC control structure */
> + layers[0].type = EDAC_MC_LAYER_CHANNEL;
> + layers[0].size = 1;
> + layers[0].is_virt_csrow = false;
> + layers[1].type = EDAC_MC_LAYER_SLOT;
> + layers[1].size = 1;
> + layers[1].is_virt_csrow = true;
> + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> + sizeof(struct loongson_edac_pvt));
> + if (mci == NULL)
> + return -ENOMEM;
> +
> + mci->mc_idx = edac_device_alloc_index();
> + mci->mtype_cap = MEM_FLAG_RDDR4;
> + mci->edac_ctl_cap = EDAC_FLAG_NONE;
> + mci->edac_cap = EDAC_FLAG_NONE;
> + mci->mod_name = "loongson_edac.c";
> + mci->ctl_name = "loongson_edac_ctl";
> + mci->dev_name = "loongson_edac_dev";
> + mci->ctl_page_to_phys = NULL;
> + mci->pdev = &pdev->dev;
> + mci->error_desc.grain = 8;
> + /* Set the function pointer to an actual operation function */
> + mci->edac_check = edac_check;
> +
> + pvt_init(mci, vbase);
> + dimm_config_init(mci);
> +
> + ret = edac_mc_add_mc(mci);
> + if (ret) {
> + edac_dbg(0, "MC: failed edac_mc_add_mc()\n");
> + edac_mc_free(mci);
> + return ret;
> + }
> + edac_op_state = EDAC_OPSTATE_POLL;
> +
> + return 0;
> +}
> +
> +static void edac_remove(struct platform_device *pdev)
> +{
> + struct mem_ctl_info *mci = edac_mc_del_mc(&pdev->dev);
> +
> + if (mci)
> + edac_mc_free(mci);
> +}
> +
> +static const struct acpi_device_id loongson_edac_acpi_match[] = {
> + {"LOON000G", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, loongson_edac_acpi_match);
> +
> +static struct platform_driver loongson_edac_driver = {
> + .probe = edac_probe,
> + .remove = edac_remove,
> + .driver = {
> + .name = "loongson-mc-edac",
> + .acpi_match_table = loongson_edac_acpi_match,
> + },
> +};
> +module_platform_driver(loongson_edac_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Zhao Qunqin <zhaoqunqin@...ngson.cn>");
Family name is usually at last, but still depends on yourself. If you
change the order, please keep consistency in the whole patch.
Others look good to me, and you can add "Reviewed-by: Huacai Chen
<chenhuacai@...ngson.cn>" in the next version.
Huacai
> +MODULE_DESCRIPTION("EDAC driver for loongson memory controller");
>
> base-commit: e14232afa94445e03fc3a0291b07a68f3408c120
> --
> 2.43.0
>
Powered by blists - more mailing lists