Re: [PATCH v3 1/5] ACPI/RAS/AEST: Initial AEST driver

[Borislav Petkov]

Just some cursory review...

On Wed, Jan 15, 2025 at 04:42:24PM +0800, Ruidong Tian wrote:
> Add support for parsing the ARM Error Source Table and basic handling of
> errors reported through both memory mapped and system register interfaces.
> 
> Assume system register interfaces are only registered with private
> peripheral interrupts (PPIs); otherwise there is no guarantee the
> core handling the error is the core which took the error and has the
> syndrome info in its system registers.
> 
> In kernel-first mode, all configuration is controlled by kernel, include
> CE ce_threshold and interrupt enable/disable.
> 
> All detected errors will be processed as follow:
>   - CE, DE: use a workqueue to log this hare errors.
>   - UER, UEO: log it and call memory_failun workquee.
>   - UC, UEU: panic in irq context.


Use a spellchecker for all your text.

In addition, use AI to check your English formulations.

> Signed-off-by: Tyler Baicar <baicar@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ruidong Tian <tianruidong@xxxxxxxxxxxxxxxxx>

Who's the author: Tyler or you?

That's denoted with the From: field.

Make sure you go over Documentation/process/submitting-patches.rst for basic
mistakes.

> ---
>  MAINTAINERS                  |  10 +
>  arch/arm64/include/asm/ras.h |  95 ++++
>  drivers/acpi/arm64/Kconfig   |  11 +
>  drivers/acpi/arm64/Makefile  |   1 +
>  drivers/acpi/arm64/aest.c    | 335 ++++++++++++
>  drivers/acpi/arm64/init.c    |   2 +
>  drivers/acpi/arm64/init.h    |   1 +
>  drivers/ras/Kconfig          |   1 +
>  drivers/ras/Makefile         |   1 +
>  drivers/ras/aest/Kconfig     |  17 +
>  drivers/ras/aest/Makefile    |   5 +
>  drivers/ras/aest/aest-core.c | 976 +++++++++++++++++++++++++++++++++++
>  drivers/ras/aest/aest.h      | 323 ++++++++++++
>  include/linux/acpi_aest.h    |  68 +++
>  include/linux/cpuhotplug.h   |   1 +
>  include/linux/ras.h          |   8 +
>  16 files changed, 1855 insertions(+)

This patch is huuge and unreviewable: split it.

Also, I see issues like:

check_for_todos: WARNING: drivers/ras/aest/aest-core.c:207: Hunk contains unfinished TODO:
+               /* TODO: translate Logical Addresses to System Physical Addresses */


check_for_todos: WARNING: drivers/ras/aest/aest-core.c:446: Hunk contains unfinished TODO:
+       //TODO: Support 32B CEC threshold.

A TODO tells me that patch is not ready for upstream.

Also, get rid of all // comments in drivers/ras/ and use normal /* style.

Enough for now.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette