On ChromiumOS devices, the ecc_size is set to 0 (check dmesg | grep ecc 
to see `ecc: 0`): this disables ECC for ramoops region, even when 
ramoops.ecc=1 is given to kernel command line parameter.

This patch introduces ecc_size module parameter to provide an method to 
turn on ECC for ramoops and set different values of ecc_size per devices.

Signed-off-by: Naoya Tezuka <naoyatezuka@chromium.org>
---
v2:
- Remove an unnecessary blank line

v1: https://lore.kernel.org/lkml/20250610025152.3844404-1-naoyatezuka@chromium.org/ 
---
 drivers/platform/chrome/chromeos_pstore.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/chrome/chromeos_pstore.c b/drivers/platform/chrome/chromeos_pstore.c
index f37c0ef4af1f..28e26aa99cdf 100644
--- a/drivers/platform/chrome/chromeos_pstore.c
+++ b/drivers/platform/chrome/chromeos_pstore.c
@@ -9,6 +9,10 @@
 #include <linux/platform_device.h>
 #include <linux/pstore_ram.h>
 
+static int ecc_size;
+module_param(ecc_size, int, 0444);
+MODULE_PARM_DESC(ecc_size, "ECC parity data size in bytes. A positive value enables ECC for the ramoops region.");
+
 static const struct dmi_system_id chromeos_pstore_dmi_table[] __initconst = {
 	{
 		/*
@@ -117,6 +121,9 @@ static int __init chromeos_pstore_init(void)
 {
 	bool acpi_dev_found;
 
+	if (ecc_size > 0)
+		chromeos_ramoops_data.ecc_info.ecc_size = ecc_size;
+
 	/* First check ACPI for non-hardcoded values from firmware. */
 	acpi_dev_found = chromeos_check_acpi();
 
-- 
2.50.0.rc0.604.gd4ff7b7c86-goog

On Tue, Jun 10, 2025 at 02:04:58PM +0900, Naoya Tezuka wrote:
> On ChromiumOS devices, the ecc_size is set to 0 (check dmesg | grep ecc 
> to see `ecc: 0`): this disables ECC for ramoops region, even when 
> ramoops.ecc=1 is given to kernel command line parameter.
> 
> This patch introduces ecc_size module parameter to provide an method to 
> turn on ECC for ramoops and set different values of ecc_size per devices.

The doc [1] suggests to describe changes in imperative mood. If you have
chance to send next version, please fix it.  Otherwise, it doesn't really
bother me.

[1]: https://docs.kernel.org/process/submitting-patches.html

> @@ -9,6 +9,10 @@
>  #include <linux/platform_device.h>
>  #include <linux/pstore_ram.h>
>  
> +static int ecc_size;
> +module_param(ecc_size, int, 0444);

Does it need to be world-readable? How about 0400?

> @@ -117,6 +121,9 @@ static int __init chromeos_pstore_init(void)
>  {
>  	bool acpi_dev_found;
>  
> +	if (ecc_size > 0)
> +		chromeos_ramoops_data.ecc_info.ecc_size = ecc_size;

It seems `ecc_size` doesn't have an upper bound.  Wondering what would
be happened if it is a somehow large value.

Hi Tzung-Bi and others,

Thank you so much for taking time and valuable feedback on my patch.

On Wed, Jun 11, 2025 at 12:10 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> The doc [1] suggests to describe changes in imperative mood. If you have
> chance to send next version, please fix it.  Otherwise, it doesn't really
> bother me.

Thank you for pointing out the convention. I'll fix the commit message to follow
this in the next version (v3).

> > @@ -9,6 +9,10 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pstore_ram.h>
> >
> > +static int ecc_size;
> > +module_param(ecc_size, int, 0444);
>
> Does it need to be world-readable? How about 0400?

Good point, there is no need for it to be world-readable. I'll change
the permission
to 0400 as you suggested in the next version.


> > @@ -117,6 +121,9 @@ static int __init chromeos_pstore_init(void)
> >  {
> >       bool acpi_dev_found;
> >
> > +     if (ecc_size > 0)
> > +             chromeos_ramoops_data.ecc_info.ecc_size = ecc_size;
>
> It seems `ecc_size` doesn't have an upper bound.  Wondering what would
> be happened if it is a somehow large value.

I have investigated this, and you are right to be concerned. Providing a large
value for `ecc_size` can indeed lead to a kernel panic.
The panic occurs within the Reed-Solomon library, specifically from a BUG_ON
check in `decode_rs()` [1] when the ECC parameters are invalid. Here
is the crash
log I observed (edited for simplicity):

[    2.395351] kernel BUG at lib/reed_solomon/decode_rs.c:43!
[    2.395355] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[    2.395358] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W
  5.15.178-24446-gf4364e2b1c85-dirty #1
f18df54893409d10705efc03f3f58f5431f53e8b
[    2.395361] Hardware name: Google Kindred/Kindred, BIOS
Google_Kindred.12672.534.0 01/19/2023
[    2.395362] RIP: 0010:decode_rs8+0xee0/0xef0
[    2.395378] Call Trace:
[    2.395379]  <TASK>
[    2.395380]  ? __die_body+0xac/0xb0
[    2.395383]  ? die+0x2f/0x50
[    2.395385]  ? do_trap+0x9e/0x170
[    2.395386]  ? decode_rs8+0xee0/0xef0
[    2.395388]  ? decode_rs8+0xee0/0xef0
[    2.395390]  ? handle_invalid_op+0x69/0x80
[    2.395391]  ? decode_rs8+0xee0/0xef0
[    2.395392]  ? exc_invalid_op+0x3b/0x50
[    2.395395]  ? asm_exc_invalid_op+0x16/0x20
[    2.395397]  ? decode_rs8+0xee0/0xef0
[    2.395399]  ? down_trylock+0x27/0x40
[    2.395401]  ? console_trylock+0x46/0xd0
[    2.395404]  persistent_ram_save_old+0xfd/0x1b0
[    2.395407]  persistent_ram_new+0x385/0x720
[    2.395410]  ramoops_init_prz+0x8e/0x120
[    2.395412]  ramoops_probe+0x25e/0x460
[    2.395414]  ? acpi_dev_pm_attach+0x27/0x110
[    2.395416]  platform_probe+0x6b/0xa0
[    2.395419]  really_probe+0xd5/0x340
[    2.395421]  __driver_probe_device+0x78/0xc0
[    2.395423]  driver_probe_device+0x28/0x180
[    2.395425]  __device_attach_driver+0x11b/0x130
[    2.395427]  ? deferred_probe_work_func+0xc0/0xc0
[    2.395429]  bus_for_each_drv+0x9d/0xe0
[    2.395430]  __device_attach+0xec/0x1a0
[    2.395432]  bus_probe_device+0x32/0xa0
[    2.395434]  device_add+0x281/0x3b0
[    2.395436]  platform_device_add+0x15e/0x200
[    2.395438]  ? chromeos_privacy_screen_driver_init+0x20/0x20
[    2.395441]  do_one_initcall+0x10e/0x2d0
[    2.395445]  ? strlen+0x10/0x20
[    2.395447]  ? parse_args+0x11f/0x3a0
[    2.395450]  do_initcall_level+0x80/0xe0
[    2.395453]  do_initcalls+0x50/0x80
[    2.395455]  kernel_init_freeable+0xee/0x160
[    2.395456]  ? rest_init+0xd0/0xd0
[    2.395458]  kernel_init+0x1a/0x110
[    2.395460]  ret_from_fork+0x1f/0x30
[    2.395463]  </TASK>
[    2.395463] Modules linked in:
[    2.396278] gsmi: Log Shutdown Reason 0x03
[    2.397390] ---[ end trace 52a9249d98b7a130 ]---

Since this validation issue exists in the pstore/ram core rather than
being specific to this driver, I believe the best approach is to address
it in a separate, new patch. My proposed fix is to add a check in
`ram_set_ecc_info()` [2] to validate the ECC parameters against the
requirements of the Reed-Solomon library, and return -EINVAL if the check
fails.
I will prepare and send this new patch for review, and add you to CC.

Thank you again for your guidance.

Best,
Naoya Tezuka


[1] https://elixir.bootlin.com/linux/v6.15.1/source/lib/reed_solomon/decode_rs.c#L43
[2] https://elixir.bootlin.com/linux/v6.15/source/fs/pstore/ram_core.c#L188