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

[Huacai Chen]

On Thu, Aug 6, 2026 at 9:36 AM Qunqin Zhao <zhaoqunqin@xxxxxxxxxxx> wrote:
>
>
> 在 2026/8/5 21:43, Huacai Chen 写道:
> > Hi, Qunqin,
> >
> > On Wed, Aug 5, 2026 at 5:09 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 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 (mcs-per-node) is obtained
> >> from firmware via ACPI _DSD property, with a default of 4 for backward
> >> compatibility. Validate that the firmware-provided value is non-zero to
> >> prevent division by zero. The node and MC IDs are decoded from mc_idx:
> >>      node = mc_idx / mcs_per_node
> >>      mc = mc_idx % mcs_per_node
> >>
> >> 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 | 35 ++++++++++++++++++++++++++++++-----
> >>   1 file changed, 30 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/edac/loongson_edac.c b/drivers/edac/loongson_edac.c
> >> index 38745800ed..abfc1e60e0 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 mcs_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] = {0};
> >> +       int new, add, node, mc;
> >>
> >>          new = read_ecc(mci);
> >>          add = new - pvt->last_ce_count;
> >> @@ -52,8 +56,14 @@ static void edac_check(struct mem_ctl_info *mci)
> >>          if (add <= 0)
> >>                  return;
> >>
> >> +       if (pvt->mc_idx_valid) {
> >> +               node = mci->mc_idx / pvt->mcs_per_node;
> >> +               mc = mci->mc_idx % pvt->mcs_per_node;
> >> +               snprintf(other_detail, sizeof(other_detail), "node:%d mc:%d", node, mc);
> >> +       }
> >> +
> >>          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 +82,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 mcs_per_node)
> >>   {
> >>          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->mcs_per_node = mcs_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 mcs_per_node, mc_idx_u32;
> > Why do we need mc_idx_u32? The code in V1 doesn't work?
>
> V1 works. This is purely to fix the issue reported by sashiko:
>
> "The mc_idx field is defined as an int in struct mem_ctl_info, but
> device_property_read_u32() expects a u32 *. Should this be read into a
> temporary u32 variable first?"
>
> Should we ignore it?
I think we should ignore it, because:
1, it is impossible that mc_idx exceeds 2^31;
2, if mc_idx really exceeds 2^31, use mc_idx_u32 is still wrong when
assign it to mci->mc_idx.

Huacai

>
> >
> >>          int ret;
> >> +       bool mc_idx_valid = true;
> >>
> >>          vbase = devm_platform_ioremap_resource(pdev, 0);
> >>          if (IS_ERR(vbase))
> >> @@ -102,7 +118,6 @@ static int edac_probe(struct platform_device *pdev)
> >>          if (mci == NULL)
> >>                  return -ENOMEM;
> >>
> >> -       mci->mc_idx = edac_device_alloc_index();
> >>          mci->mtype_cap = MEM_FLAG_RDDR4;
> >>          mci->edac_ctl_cap = EDAC_FLAG_NONE;
> >>          mci->edac_cap = EDAC_FLAG_NONE;
> >> @@ -114,7 +129,17 @@ static int edac_probe(struct platform_device *pdev)
> >>          mci->error_desc.grain = 8;
> >>          mci->edac_check = edac_check;
> >>
> >> -       pvt_init(mci, vbase);
> >> +       if (device_property_read_u32(dev, "mc-idx", &mc_idx_u32)) {
> >> +               mci->mc_idx = edac_device_alloc_index();
> >> +               mc_idx_valid = false;
> >> +       } else {
> >> +               mci->mc_idx = mc_idx_u32;
> >> +       }
> >> +
> >> +       if (device_property_read_u32(dev, "mc-per-node", &mcs_per_node) || mcs_per_node == 0)
> > The property should also be mcs-per-node.
>
> OK
>
> Thanks.
>
> > Huacai
> >
> >> +               mcs_per_node = 4;
> >> +
> >> +       pvt_init(mci, vbase, mc_idx_valid, mcs_per_node);
> >>          dimm_config_init(mci);
> >>
> >>          ret = edac_mc_add_mc(mci);
> >> --
> >> 2.47.3
> >>
> >>
>