[PATCH 0/3] EDAC/igen6: Avoid segmentation fault and add polling support

[PATCH 0/3] EDAC/igen6: Avoid segmentation fault and add polling support

[Orange Kao]

Hi Qiuxu.

Thank you for your help and guidance. Here is the updated patch set to fix
segmentation fault and to add polling support.

Patch 1: Avoid segmentation fault during rmmod
Patch 2: Initialize edac_op_state according to the configuration data
Patch 3: Add polling support

Please let me know if you would like me to change or improve anything.

[PATCH 1/3] EDAC/igen6: Avoid segmentation fault when rmmod

[Orange Kao]

The segmentation fault happens because

During modprobe:
1. In igen6_probe(), igen6_pvt will be allocated with kzalloc()
2. In igen6_register_mci(), mci->pvt_info will point to
   &igen6_pvt->imc[mc]

During rmmod:
1. In mci_release() in edac_mc.c, it will kfree(mci->pvt_info)
2. In igen6_remove(), it will kfree(igen6_pvt);

Fix this issue by setting mci->pvt_info to NULL to avoid the double
kfree.

Fixes: 10590a9d4f23 ("EDAC/igen6: Add EDAC driver for Intel client SoCs using IBECC")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219360
Signed-off-by: Orange Kao <orange@aiven.io>
---
 drivers/edac/igen6_edac.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/edac/igen6_edac.c b/drivers/edac/igen6_edac.c
index 189a2fc29e74..07dacf8c10be 100644
--- a/drivers/edac/igen6_edac.c
+++ b/drivers/edac/igen6_edac.c
@@ -1245,6 +1245,7 @@ static int igen6_register_mci(int mc, u64 mchbar, struct pci_dev *pdev)
 	imc->mci = mci;
 	return 0;
 fail3:
+	mci->pvt_info = NULL;
 	kfree(mci->ctl_name);
 fail2:
 	edac_mc_free(mci);
@@ -1269,6 +1270,7 @@ static void igen6_unregister_mcis(void)
 
 		edac_mc_del_mc(mci->pdev);
 		kfree(mci->ctl_name);
+		mci->pvt_info = NULL;
 		edac_mc_free(mci);
 		iounmap(imc->window);
 	}
-- 
2.47.0

RE: [PATCH 1/3] EDAC/igen6: Avoid segmentation fault when rmmod

[Luck, Tony]

> The segmentation fault happens because
>
> During modprobe:
> 1. In igen6_probe(), igen6_pvt will be allocated with kzalloc()
> 2. In igen6_register_mci(), mci->pvt_info will point to
>    &igen6_pvt->imc[mc]
>
> During rmmod:
> 1. In mci_release() in edac_mc.c, it will kfree(mci->pvt_info)
> 2. In igen6_remove(), it will kfree(igen6_pvt);
>
> Fix this issue by setting mci->pvt_info to NULL to avoid the double
> kfree.
>
> Fixes: 10590a9d4f23 ("EDAC/igen6: Add EDAC driver for Intel client SoCs using IBECC")
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219360
> Signed-off-by: Orange Kao <orange@aiven.io>

I've applied this patch to the ras tree. Thanks.

Patches 2 & 3 are on hold waiting for an answer to Boris' question
on whether polling mode can be applied automatically on systems
that need it. Rather than pushing the burden onto the user to use the
module parameter to select it.

-Tony

[PATCH 2/3] EDAC/igen6: Initialize edac_op_state according to the configuration data

[Orange Kao]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Currently, igen6_edac sets edac_op_state to EDAC_OPSTATE_NMI, while the
driver also supports memory errors reported from Machine Check. Initialize
edac_op_state to the correct value according to the configuration data
that the driver probed.

Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
 drivers/edac/igen6_edac.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/igen6_edac.c b/drivers/edac/igen6_edac.c
index 07dacf8c10be..fa488ba15059 100644
--- a/drivers/edac/igen6_edac.c
+++ b/drivers/edac/igen6_edac.c
@@ -1350,6 +1350,15 @@ static void unregister_err_handler(void)
 	unregister_nmi_handler(NMI_SERR, IGEN6_NMI_NAME);
 }
 
+static void opstate_set(struct res_config *cfg)
+{
+	/* Set the mode according to the configuration data. */
+	if (cfg->machine_check)
+		edac_op_state = EDAC_OPSTATE_INT;
+	else
+		edac_op_state = EDAC_OPSTATE_NMI;
+}
+
 static int igen6_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	u64 mchbar;
@@ -1367,6 +1376,8 @@ static int igen6_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		goto fail;
 
+	opstate_set(res_cfg);
+
 	for (i = 0; i < res_cfg->num_imc; i++) {
 		rc = igen6_register_mci(i, mchbar, pdev);
 		if (rc)
@@ -1450,8 +1461,6 @@ static int __init igen6_init(void)
 	if (owner && strncmp(owner, EDAC_MOD_STR, sizeof(EDAC_MOD_STR)))
 		return -EBUSY;
 
-	edac_op_state = EDAC_OPSTATE_NMI;
-
 	rc = pci_register_driver(&igen6_driver);
 	if (rc)
 		return rc;
-- 
2.47.0

[PATCH 3/3] EDAC/igen6: Add polling support

[Orange Kao]

Some PCs with Intel N100 (with PCI device 8086:461c, DID_ADL_N_SKU4)
experienced issues with error interrupts not working, even with the
following configuration in the BIOS.

    In-Band ECC Support: Enabled
    In-Band ECC Operation Mode: 2 (make all requests protected and
                                   ignore range checks)
    IBECC Error Injection Control: Inject Correctable Error on insertion
                                   counter
    Error Injection Insertion Count: 251658240 (0xf000000)

Add polling mode support for these machines to ensure that memory error
events are handled.

Signed-off-by: Orange Kao <orange@aiven.io>
---
 drivers/edac/igen6_edac.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/edac/igen6_edac.c b/drivers/edac/igen6_edac.c
index fa488ba15059..eb783c6b77f1 100644
--- a/drivers/edac/igen6_edac.c
+++ b/drivers/edac/igen6_edac.c
@@ -1170,6 +1170,20 @@ static int igen6_pci_setup(struct pci_dev *pdev, u64 *mchbar)
 	return -ENODEV;
 }
 
+static void igen6_check(struct mem_ctl_info *mci)
+{
+	struct igen6_imc *imc = mci->pvt_info;
+	u64 ecclog;
+
+	/* errsts_clear() isn't NMI-safe. Delay it in the IRQ context */
+	ecclog = ecclog_read_and_clear(imc);
+	if (!ecclog)
+		return;
+
+	if (!ecclog_gen_pool_add(imc->mc, ecclog))
+		irq_work_queue(&ecclog_irq_work);
+}
+
 static int igen6_register_mci(int mc, u64 mchbar, struct pci_dev *pdev)
 {
 	struct edac_mc_layer layers[2];
@@ -1211,6 +1225,8 @@ static int igen6_register_mci(int mc, u64 mchbar, struct pci_dev *pdev)
 	mci->edac_cap = EDAC_FLAG_SECDED;
 	mci->mod_name = EDAC_MOD_STR;
 	mci->dev_name = pci_name(pdev);
+	if (edac_op_state == EDAC_OPSTATE_POLL)
+		mci->edac_check = igen6_check;
 	mci->pvt_info = &igen6_pvt->imc[mc];
 
 	imc = mci->pvt_info;
@@ -1352,6 +1368,10 @@ static void unregister_err_handler(void)
 
 static void opstate_set(struct res_config *cfg)
 {
+	/* Only the polling mode can be set via the module parameter. */
+	if (edac_op_state == EDAC_OPSTATE_POLL)
+		return;
+
 	/* Set the mode according to the configuration data. */
 	if (cfg->machine_check)
 		edac_op_state = EDAC_OPSTATE_INT;
@@ -1483,3 +1503,6 @@ module_exit(igen6_exit);
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Qiuxu Zhuo");
 MODULE_DESCRIPTION("MC Driver for Intel client SoC using In-Band ECC");
+
+module_param(edac_op_state, int, 0444);
+MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll, Others or default=Auto detect");
-- 
2.47.0

Re: [PATCH 3/3] EDAC/igen6: Add polling support

[Borislav Petkov]

On Mon, Nov 04, 2024 at 12:40:54PM +0000, Orange Kao wrote:
> +module_param(edac_op_state, int, 0444);
> +MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll, Others or default=Auto detect");

Why is this module parameter here instead of detecting those broken machines
and enabling polling on them by default and automatically?

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH 3/3] EDAC/igen6: Add polling support

[Zhuo, Qiuxu]

> From: Borislav Petkov <bp@alien8.de>
> [...]
> On Mon, Nov 04, 2024 at 12:40:54PM +0000, Orange Kao wrote:
> > +module_param(edac_op_state, int, 0444);
> > +MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll,
> > +Others or default=Auto detect");
> 
> Why is this module parameter here instead of detecting those broken
> machines and enabling polling on them by default and automatically?

Good suggestion. Thanks, Boris. 

@Orange Kao,
As per Boris' suggestion, set the default to polling mode for those broken machines
to offload the burden from userspace.

1) A small update to your current patch, as shown below for your reference. 

static void opstate_set(struct res_config *cfg, const struct pci_device_id *ent)
{
        /*
         * Quirk: Certain SoCs' error reporting interrupts don't work.
         *        Force polling mode for them to ensure that memory error
         *        events can be handled.
         */
        if (ent->device == DID_ADL_N_SKU4) {
                edac_op_state = EDAC_OPSTATE_POLL;
                return;
        }

        /* Set the mode according to the configuration data. */
        if (cfg->machine_check)
                edac_op_state = EDAC_OPSTATE_INT;
        else
                edac_op_state = EDAC_OPSTATE_NMI;
}

2) The call site is updated accordingly:
      ...
      opstate_set(res_cfg, ent);
      ...

3) Also, the following 2 lines are no longer needed in this patch.
    
     module_param(edac_op_state, int, 0444);
     MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll, Others or default=Auto detect");

Could you try it and help resend a new version of this patch? 
Or any questions please feel free to let me know.
Thanks!

-Qiuxu