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

在 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.
>   	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);