Re: [PATCH 1/3] EDAC/loongson: Encode node and MC info into mc_idx

[Qunqin Zhao]

在 2026/8/3 16:25, Huacai Chen 写道:
> Hi, Qunqin,
>
> On Thu, Jul 30, 2026 at 2:45 PM Qunqin Zhao <zhaoqunqin@xxxxxxxxxxx> wrote:
> > From: Wang Jinwei <wangjinwei@xxxxxxxxxxx>
> >
> > On Loongson multi-node systems, memory controllers are distributed
> > across multiple nodes. Encode node and MC information into mc_idx
> > so that sysfs entries uniquely identify each controller.
> >
> > The driver gets mc-idx from ACPI _DSD property 'mc-idx' if firmware
> > provides it. If not, it falls back to edac_device_alloc_index() for
> > legacy firmware compatibility.
> >
> > The number of memory controllers per node (mc-per-node) is obtained
> > from firmware via ACPI _DSD property, with a default of 4 for
> > backward compatibility. The node and MC IDs are decoded from mc_idx:
> >       node = mc_idx / mc_per_node
> >       mc = mc_idx % mc_per_node
> >
> > For example, with mc-per-node=4:
> >       mc-idx 0-3   -> node0 mc0-mc3
> >       mc-idx 4-7   -> node1 mc0-mc3
> >       ...
> >       mc-idx 28-31 -> node7 mc0-mc3
> >
> > The dmesg output looks like this：
> > Before: (no _DSD mc-idx)
> > EDAC MC3: 510 CE error on MC#3Channel#0_DIMM#0 (channel:0 slot:0
> > page:0x0 offset:0x0 grain:8 syndrome:0x0)
> >
> > After: (_DSD mc-idx)
> > EDAC MC3: 510 CE error on MC#3Channel#0_DIMM#0 (channel:0 slot:0
> > page:0x0 offset:0x0 grain:8 syndrome:0x0 - node:0 mc:3)
> >
> > Cc: Yulong Wang <wangyulong@xxxxxxxxxxx>
> > Cc: Dongyan Qian <qiandongyan@xxxxxxxxxxx>
> > Cc: Chao Li <lichao@xxxxxxxxxxx>
> > Signed-off-by: Wang Jinwei <wangjinwei@xxxxxxxxxxx>
> > Signed-off-by: Qunqin Zhao <zhaoqunqin@xxxxxxxxxxx>
> > ---
> >   drivers/edac/loongson_edac.c | 38 +++++++++++++++++++++++++++++++-----
> >   1 file changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/edac/loongson_edac.c b/drivers/edac/loongson_edac.c
> > index 38745800ed..f2452e98f5 100644
> > --- a/drivers/edac/loongson_edac.c
> > +++ b/drivers/edac/loongson_edac.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/io-64-nonatomic-lo-hi.h>
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >   #include "edac_module.h"
> >
> >   #define ECC_CS_COUNT_REG       0x18
> > @@ -23,6 +24,8 @@ struct loongson_edac_pvt {
> >           * register state.
> >           */
> >          int last_ce_count;
> > +       int mc_per_node;
> I think mcs_per_node is better than mc_per_node.
>
> > +       bool mc_idx_valid;
> >   };
> >
> >   static int read_ecc(struct mem_ctl_info *mci)
> > @@ -44,7 +47,8 @@ static int read_ecc(struct mem_ctl_info *mci)
> >   static void edac_check(struct mem_ctl_info *mci)
> >   {
> >          struct loongson_edac_pvt *pvt = mci->pvt_info;
> > -       int new, add;
> > +       char other_detail[64];
> Use char other_detail[64] = {0}, then we can drop the 'else' branch below.
>
> > +       int new, add, node, mc;
> >
> >          new = read_ecc(mci);
> >          add = new - pvt->last_ce_count;
> > @@ -52,8 +56,17 @@ static void edac_check(struct mem_ctl_info *mci)
> >          if (add <= 0)
> >                  return;
> >
> > +       if (pvt->mc_idx_valid) {
> > +               node = mci->mc_idx / pvt->mc_per_node;
> > +               mc = mci->mc_idx % pvt->mc_per_node;
> > +               snprintf(other_detail, sizeof(other_detail),
> > +                               "node:%d mc:%d", node, mc);
> The snprintf() can be put in oneline.
>
> > +       } else {
> > +               other_detail[0] = '\0';
> > +       }
> > +
> >          edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, add,
> > -                            0, 0, 0, 0, 0, -1, "error", "");
> > +                            0, 0, 0, 0, 0, -1, "error", other_detail);
> >   }
> >
> >   static void dimm_config_init(struct mem_ctl_info *mci)
> > @@ -72,20 +85,26 @@ static void dimm_config_init(struct mem_ctl_info *mci)
> >          dimm->grain = 8;
> >   }
> >
> > -static void pvt_init(struct mem_ctl_info *mci, void __iomem *vbase)
> > +static void pvt_init(struct mem_ctl_info *mci, void __iomem *vbase,
> > +               bool mc_idx_valid, int mc_per_node)
> The function definition can be put in one line.
>
> >   {
> >          struct loongson_edac_pvt *pvt = mci->pvt_info;
> >
> >          pvt->ecc_base = vbase;
> >          pvt->last_ce_count = read_ecc(mci);
> > +       pvt->mc_idx_valid = mc_idx_valid;
> > +       pvt->mc_per_node = mc_per_node;
> >   }
> >
> >   static int edac_probe(struct platform_device *pdev)
> >   {
> >          struct edac_mc_layer layers[2];
> >          struct mem_ctl_info *mci;
> > +       struct device *dev = &pdev->dev;
> >          void __iomem *vbase;
> > +       u32 mc_per_node;
> >          int ret;
> > +       bool mc_idx_valid;
> Use bool mc_idx_valid = true, then we can drop the 'else' branch below.

Will fix all,

Thanks

> Huacai