Re: [PATCH] EDAC/dummy: Add a dummy EDAC driver

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Tue, Jul 07, 2026 at 02:53:07PM -0700, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> A dummy EDAC driver is useful for testing purposes in a VM when one
> doesn't have all the hardware needed to test aspects of the EDAC
> subsystem code.

Is this intended to be a replacement for the "fake_inject" interface?

I like the idea of having a test module, but I think this is too simple.

We really need to cover the 'layers'. IMO, that's where the complexity
lies.

There can be module parameters to describe the layers and their types.

Examples:
 layer0=all
 layer0=channel layer1=slot
 layer0=slot layer1=channel layer2=chip_select

With this, we could test all the permutations of enumerating the EDAC
sysfs interface.

> 
> Assisted-by: Claude:Sonnet-5
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>  drivers/edac/Kconfig      |   9 ++
>  drivers/edac/Makefile     |   3 +-
>  drivers/edac/dummy_edac.c | 193 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/edac/dummy_edac.c
> 
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index a44b85c440ca..a8914baa5f7e 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -581,4 +581,13 @@ config EDAC_VERSALNET
>  	  and other system errors from various IP subsystems like RPU, NOCs,
>  	  HNICX, PL on the AMD Versal NET DDR memory controller.
>  
> +config EDAC_DUMMY
> +	tristate "Dummy EDAC driver"
> +	default n
> +	help
> +	  A dummy EDAC driver is useful for testing purposes in a VM when one
> +	  doesn't have all the hardware needed to test aspects of the EDAC
> +	  subsystem code.

This is verbatim the commit message.

> +
> +	  You definitely wanna say N here.
>  endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index a37534300ab9..92f8d03dad30 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -35,7 +35,7 @@ obj-$(CONFIG_EDAC_I7300)		+= i7300_edac.o
>  obj-$(CONFIG_EDAC_I7CORE)		+= i7core_edac.o
>  obj-$(CONFIG_EDAC_SBRIDGE)		+= sb_edac.o
>  obj-$(CONFIG_EDAC_PND2)			+= pnd2_edac.o
> -obj-$(CONFIG_EDAC_IGEN6)			+= igen6_edac.o
> +obj-$(CONFIG_EDAC_IGEN6)		+= igen6_edac.o

Stray change?

>  obj-$(CONFIG_EDAC_E7XXX)		+= e7xxx_edac.o
>  obj-$(CONFIG_EDAC_E752X)		+= e752x_edac.o
>  obj-$(CONFIG_EDAC_I82875P)		+= i82875p_edac.o
> @@ -91,3 +91,4 @@ obj-$(CONFIG_EDAC_VERSAL)		+= versal_edac.o
>  obj-$(CONFIG_EDAC_LOONGSON)		+= loongson_edac.o
>  obj-$(CONFIG_EDAC_VERSALNET)		+= versalnet_edac.o
>  obj-$(CONFIG_EDAC_CORTEX_A72)		+= a72_edac.o
> +obj-$(CONFIG_EDAC_DUMMY)		+= dummy_edac.o
> diff --git a/drivers/edac/dummy_edac.c b/drivers/edac/dummy_edac.c
> new file mode 100644
> index 000000000000..00b4fee3fc8d
> --- /dev/null
> +++ b/drivers/edac/dummy_edac.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Dummy EDAC driver
> + *
> + * A minimal EDAC device driver that registers its own platform device
> + * unconditionally, so it probes successfully on any system -- including
> + * VMs and guests that have no real ECC-capable cache/memory hardware
> + * exposed to them. Useful for exercising the EDAC core, sysfs interface,
> + * and userspace tooling (edac-utils, mcelog, etc.) without needing the
> + * physical hardware the "real" drivers depend on.
> + *
> + * Modeled after drivers/edac/a72_edac.c (Cortex A72 EDAC L1/L2 cache
> + * error detection), but with all of the hardware-specific bits (system
> + * register reads, SMP cross-calls, devicetree compatible matching)
> + * replaced by a software-only error source that can be poked from
> + * debugfs or left to generate nothing at all.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/debugfs.h>
> +#include <linux/slab.h>
> +#include <linux/cpu.h>

Subjective preference: order alphabetically

> +
> +#include "edac_module.h"
> +
> +#define DRVNAME		"dummy-edac"
> +#define MESSAGE_SIZE	64
> +
> +/*
> + * Simulated "error syndrome". In real hardware drivers this would be
> + * read out of a per-CPU system/MMIO register. Here userspace (via
> + * debugfs) or nothing at all drives it, which is exactly the point:
> + * the driver has no dependency on any physical error source.

This needs revision.

> + */
> +struct dummy_edac_priv {
> +	struct dentry *debugfs_dir;
> +	/* pending correctable error to report */
> +	atomic_t inject_ce;
> +	/* pending uncorrectable error to report */
> +	atomic_t inject_ue;
> +};

Redundant comments.

> +
> +static void dummy_edac_check(struct edac_device_ctl_info *edac_ctl)
> +{
> +	struct dummy_edac_priv *priv = edac_ctl->pvt_info;
> +	int cpu = raw_smp_processor_id();
> +	char msg[MESSAGE_SIZE];
> +
> +	if (atomic_xchg(&priv->inject_ce, 0)) {
> +		snprintf(msg, MESSAGE_SIZE,
> +			 "simulated correctable error on CPU %d", cpu);
> +		edac_device_handle_ce(edac_ctl, cpu, 0, msg);
> +	}
> +
> +	if (atomic_xchg(&priv->inject_ue, 0)) {
> +		snprintf(msg, MESSAGE_SIZE,
> +			 "simulated uncorrectable error on CPU %d", cpu);
> +		edac_device_handle_ue(edac_ctl, cpu, 0, msg);
> +	}
> +}
> +
> +/*
> + * debugfs knobs so you can drive the driver from userspace inside the
> + * guest, e.g.:
> + *   echo 1 > /sys/kernel/debug/dummy-edac/inject_ce
> + *   echo 1 > /sys/kernel/debug/dummy-edac/inject_ue
> + */
> +static int inject_ce_set(void *data, u64 val)
> +{
> +	struct dummy_edac_priv *priv = data;
> +
> +	if (val)
> +		atomic_set(&priv->inject_ce, 1);
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(inject_ce_fops, NULL, inject_ce_set, "%llu\n");
> +
> +static int inject_ue_set(void *data, u64 val)
> +{
> +	struct dummy_edac_priv *priv = data;
> +
> +	if (val)
> +		atomic_set(&priv->inject_ue, 1);
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(inject_ue_fops, NULL, inject_ue_set, "%llu\n");
> +
> +static int dummy_edac_probe(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *edac_ctl;
> +	struct dummy_edac_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	int rc;
> +
> +	edac_ctl = edac_device_alloc_ctl_info(sizeof(*priv), "cpu",
> +					      num_possible_cpus(), "L", 1, 1,
> +					      edac_device_alloc_index());
> +	if (!edac_ctl)
> +		return -ENOMEM;
> +
> +	priv = edac_ctl->pvt_info;
> +	atomic_set(&priv->inject_ce, 0);
> +	atomic_set(&priv->inject_ue, 0);
> +
> +	edac_ctl->edac_check = dummy_edac_check;
> +	edac_ctl->dev = dev;
> +	edac_ctl->mod_name = dev_name(dev);
> +	edac_ctl->dev_name = dev_name(dev);
> +	edac_ctl->ctl_name = DRVNAME;
> +	/* Poll fairly slowly; this is a software source, not real hardware. */
> +	edac_ctl->poll_msec = 1000;
> +	dev_set_drvdata(dev, edac_ctl);
> +
> +	rc = edac_device_add_device(edac_ctl);
> +	if (rc)
> +		goto out_dev;
> +
> +	priv->debugfs_dir = debugfs_create_dir(DRVNAME, NULL);
> +	debugfs_create_file_unsafe("inject_ce", 0200, priv->debugfs_dir,
> +				    priv, &inject_ce_fops);
> +	debugfs_create_file_unsafe("inject_ue", 0200, priv->debugfs_dir,
> +				    priv, &inject_ue_fops);
> +
> +	return 0;
> +
> +out_dev:
> +	edac_device_free_ctl_info(edac_ctl);
> +
> +	return rc;
> +}
> +
> +static void dummy_edac_remove(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *edac_ctl = dev_get_drvdata(&pdev->dev);
> +	struct dummy_edac_priv *priv = edac_ctl->pvt_info;
> +
> +	debugfs_remove_recursive(priv->debugfs_dir);
> +	edac_device_del_device(edac_ctl->dev);
> +	edac_device_free_ctl_info(edac_ctl);
> +}
> +
> +static struct platform_driver dummy_edac_driver = {
> +	.probe = dummy_edac_probe,
> +	.remove = dummy_edac_remove,
> +	.driver = {
> +		.name = DRVNAME,
> +	},
> +};
> +
> +/*
> + * No devicetree/ACPI match table, and no scan of CPU nodes for a "compatible"
> + * + enable property: simply register a platform device unconditionally so
> + * probe() always runs. There is nothing here that depends on the underlying
> + * platform actually exposing the corresponding hardware, so it loads fine
> + * under QEMU/KVM, containers-with-a-kernel, or any other guest environment.
> + */
> +static struct platform_device *dummy_pdev;
> +
> +static int __init dummy_edac_driver_init(void)
> +{
> +	int rc;
> +
> +	dummy_pdev = platform_device_register_simple(DRVNAME, -1, NULL, 0);
> +	if (IS_ERR(dummy_pdev)) {
> +		pr_err(DRVNAME ": failed to register dummy platform device\n");
> +		return PTR_ERR(dummy_pdev);
> +	}
> +
> +	rc = platform_driver_register(&dummy_edac_driver);
> +	if (rc) {
> +		platform_device_unregister(dummy_pdev);
> +		return rc;
> +	}
> +
> +	pr_info("Loading %s\n", DRVNAME);
> +
> +	return 0;
> +}
> +
> +static void __exit dummy_edac_driver_exit(void)
> +{
> +	platform_driver_unregister(&dummy_edac_driver);
> +	platform_device_unregister(dummy_pdev);
> +
> +	pr_info("Removing %s\n", DRVNAME);
> +}
> +
> +module_init(dummy_edac_driver_init);
> +module_exit(dummy_edac_driver_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Dummy EDAC driver for testing without real ECC hardware");
> -- 
> 

Recently, I've been doing an 'interactive review' with the coding
assistant. This is in contrast to the automated reviews that the bots,
et al. are doing.

Basically, once the patch[set] is mostly okay, I have the assistant go
over each patch with me one at a time. It presents the commit message
and each code hunk one at a time. And it must wait for my feedback
before moving on between steps.

I find this approach helps clean up the first draft. And it seems easier
to make minor adjustments as you go along.

Thanks,
Yazen