Re: [PATCH v2 3/3] acpi/apei: Add NVIDIA GHES vendor CPER record handler

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 19 Mar 2026 19:13:09 +0800
Kai-Heng Feng <kaihengf@nvidia.com> wrote:

> Add support for decoding NVIDIA-specific CPER sections delivered via
> the APEI GHES vendor record notifier chain. NVIDIA hardware generates
> vendor-specific CPER sections containing error signatures and diagnostic
> register dumps. This implementation registers a notifier_block with the
> GHES vendor record notifier and decodes these sections, printing error
> details via dev_info().
> 
> The driver binds to ACPI device NVDA2012, present on NVIDIA server
> platforms. The NVIDIA CPER section contains a fixed header with error
> metadata (signature, error type, severity, socket) followed by
> variable-length register address-value pairs for hardware diagnostics.
> 
> This work is based on libcper [0].
> 
> Example output:
> nvidia-ghes NVDA2012:00: NVIDIA CPER section, error_data_length: 544
> nvidia-ghes NVDA2012:00: signature: CMET-INFO
> nvidia-ghes NVDA2012:00: error_type: 0
> nvidia-ghes NVDA2012:00: error_instance: 0
> nvidia-ghes NVDA2012:00: severity: 3
> nvidia-ghes NVDA2012:00: socket: 0
> nvidia-ghes NVDA2012:00: number_regs: 32
> nvidia-ghes NVDA2012:00: instance_base: 0x0000000000000000
> nvidia-ghes NVDA2012:00: register[0]: address=0x8000000100000000 value=0x0000000100000000
> 
> [0] https://github.com/openbmc/libcper/commit/683e055061ce
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Kai-Heng Feng <kaihengf@nvidia.com>
Only significant thing is around use of dev_err_probe().

I'm surprised that didn't give you error messages in the log even on success.

With that fixed (other stuff is all up to you).
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>


>  apei-y := apei-base.o hest.o erst.o bert.o
> diff --git a/drivers/acpi/apei/nvidia-ghes.c b/drivers/acpi/apei/nvidia-ghes.c
> new file mode 100644
> index 000000000000..aa2e3a387b49
> --- /dev/null
> +++ b/drivers/acpi/apei/nvidia-ghes.c

> +static void nvidia_ghes_print_error(struct device *dev,
> +				    const struct cper_sec_nvidia *nvidia_err,
> +				    size_t error_data_length, bool fatal)
> +{
> +	const char *level = fatal ? KERN_ERR : KERN_INFO;
> +	size_t min_size;
> +	int i;
...


> +	 * Validate that all registers fit within error_data_length.
> +	 * Each register pair is two little-endian u64s.
> +	 */
> +	min_size = struct_size(nvidia_err, regs, nvidia_err->number_regs);
> +	if (error_data_length < min_size) {
> +		dev_err(dev, "Invalid number_regs %u (section size %zu, need %zu)\n",
> +			nvidia_err->number_regs, error_data_length, min_size);
> +		return;
> +	}
> +
> +	for (i = 0; i < nvidia_err->number_regs; i++)

Trivial but I'd take advantage of it now being acceptable (in general) to do
	for (int i = 0; i < ....)

> +		dev_printk(level, dev, "register[%d]: address=0x%016llx value=0x%016llx\n",
> +			   i, le64_to_cpu(nvidia_err->regs[i].addr),
> +			   le64_to_cpu(nvidia_err->regs[i].val));
> +}

> +static int nvidia_ghes_probe(struct platform_device *pdev)
> +{
> +	struct nvidia_ghes_private *priv;
> +
> +	priv = devm_kmalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	*priv = (struct nvidia_ghes_private) {
> +		.nb.notifier_call = nvidia_ghes_notify,
> +		.dev = &pdev->dev,
> +	};
> +
> +	return dev_err_probe(&pdev->dev,
> +			     devm_ghes_register_vendor_record_notifier(&pdev->dev, &priv->nb),
That's too not great for readability and dev_err_probe() should only be called on errors
I'm fairly sure it doesn't have special handling for 0 so will call dev_err() or dev_warn()
and print some stuff before saying 'no error'.

	int ret;
	...

	ret = devm_ghes_register_vendor_record_notifier(&pdev->dev, &priv->nb);
	if (ret)
		return dev_err_probe(&pdev->dev,
				      "Failed to register NVIDIA GHES vendor record notifier\n");

	return 0;



> +			     "Failed to register NVIDIA GHES vendor record notifier\n");
> +}
> +
> +static const struct acpi_device_id nvidia_ghes_acpi_match[] = {
> +	{ "NVDA2012" },

London Olympics :)

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, nvidia_ghes_acpi_match);
> +
> +static struct platform_driver nvidia_ghes_driver = {
> +	.driver = {
> +		.name		= "nvidia-ghes",
> +		.acpi_match_table = nvidia_ghes_acpi_match,
> +	},
> +	.probe	= nvidia_ghes_probe,

I'd just not attempt to align the = 
static struct platform_driver nvidia_ghes_driver = {
	.driver = {
		.name = "nvidia-ghes",
		.acpi_match_table = nvidia_ghes_acpi_match,
	},
	.probe = nvidia_ghes_probe,

There aren't enough of them to make it much of a readability improvement
and doing this often results in unnecessary churn as a driver evolves.
Also it's already broken!

> +};
> +module_platform_driver(nvidia_ghes_driver);
> +
> +MODULE_AUTHOR("Kai-Heng Feng <kaihengf@nvidia.com>");
> +MODULE_DESCRIPTION("NVIDIA GHES vendor CPER record handler");
> +MODULE_LICENSE("GPL");