Re: [PATCH v3 3/3] EDAC/loongson: Add CS4-7 timeout filter for cross-node access

On Fri, Aug 14, 2026 at 3:33 PM Qunqin Zhao <[email protected]> wrote:
>
>
> 在 2026/8/12 14:41, Qunqin Zhao 写道:
>
>
> > On Loongson 3C6000 multi-node systems, when the EDAC driver running
> > on one node accesses the memory controller registers of another node,
> > the read operation may timeout and return random values. This causes
> > false CE error reports because the random values in CS0-3 (low 32 bits)
> > are misinterpreted as valid ECC error counts.
> >
> > For cross-node accesses, check CS4-7 (high 32 bits) of the ECC
> > count register. If the valid-cs-bits firmware property is set and
> > the high bits are non-zero, it indicates a timeout or random value,
> > so skip the report and return the previous count.
> >
> > To avoid undefined behavior on 8-CS systems where valid_cs_bits=64,
> > guard the shift with a bounds check (valid_cs_bits < 64).
> >
> > Signed-off-by: Wusheng <[email protected]>
> > Signed-off-by: Wang Jinwei <[email protected]>
> > Signed-off-by: Qunqin Zhao <[email protected]>
> > ---
> >   drivers/edac/loongson_edac.c | 25 ++++++++++++++++++++-----
> >   1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/edac/loongson_edac.c b/drivers/edac/loongson_edac.c
> > index a3776a8501..eb36b708e6 100644
> > --- a/drivers/edac/loongson_edac.c
> > +++ b/drivers/edac/loongson_edac.c
> > @@ -27,6 +27,7 @@ struct loongson_edac_pvt {
> >        */
> >       int last_ce_count;
> >       int mcs_per_node;
> > +     int valid_cs_bits;
>
> Thanks for the pointer. I've reviewed the Sashiko report and address each
> finding below.
>
> [Severity: Medium]
>
>
>  > The valid-cs-bits device property is parsed as a u32, but it is stored
>  > here as a signed int.
>  >
>  > If the device tree specifies a value >= 0x80000000, it would be cast to a
>  > negative integer. This could bypass the bounds check in read_ecc()
> where it
>  > checks pvt->valid_cs_bits < 64, resulting in a right-shift by a negative
>  > amount.
>  >
>  > Could this be declared as a u32 instead to avoid undefined behavior?
>
> I'll change `int valid_cs_bits` to `u32` in the struct and
> in the `pvt_init()` parameter.
If you really want to do this, please do the same changes for mcs_per_node.

Huacai

> >       bool mc_idx_valid;
> >   };
> >
> > @@ -37,11 +38,21 @@ static int read_ecc(struct mem_ctl_info *mci)
> >       int cs;
> >
> >       ecc = readq(pvt->ecc_base + ECC_CS_COUNT_REG);
> > -     /* cs0 -- cs3 */
> > +     /* Discard the read value if any invalid bit is set to 1 */
> > +     if (pvt->valid_cs_bits < 64 && (ecc >> pvt->valid_cs_bits)) {
> > +             edac_mc_printk(mci, KERN_DEBUG, "ECC read invalid, skip: 0x%llx\n", ecc);
> > +             return pvt->last_ce_count;
> > +     }
> [Severity: Medium]
>
>  > If the hardware read times out during probe, this will return the
>  > uninitialized pvt->last_ce_count (which is 0 since pvt_info is
>  > zero-allocated).
>  >
>  > Since pvt_init() uses this to set the baseline error count:
>  >
>  > drivers/edac/loongson_edac.c:pvt_init() {
>  >     ...
>  >     pvt->last_ce_count = read_ecc(mci);
>  > }
>  >
>  > Does this incorrectly set the baseline count to 0 if a timeout occurs
> on the
>  > first read? If so, on the next successful poll, the entire historical
> error
>  > count since system reset would be subtracted by 0 and falsely reported as
>  > newly occurred errors.
>
> This cannot be fixed in software:
>
> 1. Waiting for a "correct" read during probe could hang the kernel.
> 2. A bounded retry cannot guarantee correctness - all reads within the
> window
>     could still return bogus values.
> 3. The only thing software can do is print a debug message when a suspicious
>     value is detected, which this patch already does.
> > +
> > +     /* cs0 -- cs7 */
> >       cs = ecc & 0xff;
> >       cs += (ecc >> 8) & 0xff;
> >       cs += (ecc >> 16) & 0xff;
> >       cs += (ecc >> 24) & 0xff;
> > +     cs += (ecc >> 32) & 0xff;
> > +     cs += (ecc >> 40) & 0xff;
> > +     cs += (ecc >> 48) & 0xff;
> > +     cs += (ecc >> 56) & 0xff;
> >
> >       return cs;
> >   }
> > @@ -111,14 +122,15 @@ static void dimm_config_init(struct mem_ctl_info *mci)
> >   }
> >
> >   static void pvt_init(struct mem_ctl_info *mci, void __iomem *vbase,
> > -                  bool mc_idx_valid, int mcs_per_node)
> > +                  bool mc_idx_valid, int mcs_per_node, int valid_cs_bits)
> >   {
> >       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;
> > +     pvt->valid_cs_bits = valid_cs_bits;
> > +     pvt->last_ce_count = read_ecc(mci);
> >   }
> >
> >   static int edac_probe(struct platform_device *pdev)
> > @@ -127,7 +139,7 @@ static int edac_probe(struct platform_device *pdev)
> >       struct mem_ctl_info *mci;
> >       struct device *dev = &pdev->dev;
> >       void __iomem *vbase;
> > -     u32 mcs_per_node;
> > +     u32 mcs_per_node, valid_cs_bits;
> >       int ret;
> >       bool mc_idx_valid = true;
> >
> > @@ -165,7 +177,10 @@ static int edac_probe(struct platform_device *pdev)
> >       if (device_property_read_u32(dev, "mcs-per-node", &mcs_per_node) || mcs_per_node == 0)
> >               mcs_per_node = 4;
> >
> > -     pvt_init(mci, vbase, mc_idx_valid, mcs_per_node);
> > +     if (device_property_read_u32(dev, "valid-cs-bits", &valid_cs_bits))
> > +             valid_cs_bits = 32;
> [Severity: High]
>
>  > On older 8-CS systems lacking the valid-cs-bits firmware property, this
>  > defaults to 32.
>  >
>  > If a correctable error occurs on CS4-CS7 on these systems, the
> hardware ECC
>  > register will populate the upper 32 bits. The timeout filter in
> read_ecc()
>  > will incorrectly interpret this as a timeout and discard the read.
>  >
>  > Because the hardware ECC register is cumulative and cannot be
> cleared, the
>  > upper bits will remain non-zero permanently. Will this cause every
> subsequent
>  > poll to be discarded, effectively deadlocking EDAC error reporting
> for all
>  > chip selects on that node?
>
> This concern does not apply for two reasons:
>
> 1. There are no 8-CS systems at this point, so backward compatibility with
>     8-CS firmware is not something we need to account for. The
> `valid-cs-bits`
>     default of 32 matches the 4-CS hardware that the current firmware
> targets.
>
> 2. On 8-CS systems, all 64 bits of the register are valid counter bits, so
>     software cannot distinguish a correct read from a bogus one. Determining
>     whether a read is valid on such systems requires a hardware
> mechanism; no
>     software-only check can do it.
>
> Please let me know if this addresses your concerns; otherwise I'll send a v4
> with the Finding 1 fix.
>
> Thanks,
> Qunqin
> > +
> > +     pvt_init(mci, vbase, mc_idx_valid, mcs_per_node, valid_cs_bits);
> >       dimm_config_init(mci);
> >
> >       ret = edac_mc_add_mc(mci);
>