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

[Qunqin Zhao]

在 2026/9/30 08:34, Borislav Petkov 写道:
> On Wed, Sep 02, 2026 at 04:48:25PM +0800, Qunqin Zhao wrote:
>> @@ -23,6 +24,8 @@ struct loongson_edac_pvt {
>>  	 * register state.
>>  	 */
>>  	int last_ce_count;
>> +	u32 mcs_per_node;
>> +	bool mc_idx_valid;
> Why do you need a silly bool if you have valid indices in any case?
>
> You either get it from edac_device_alloc_index() and in that case, the node
> number is always 0 and when you get it from ACPI, you get the right thing
> anyway?
>
> IOW, you init mc_idx properly in the probe function and then you're all set.
The indices are unique, but they are not physically meaningful.

`edac_device_alloc_index()` returns a global atomic counter:

```c
int edac_device_alloc_index(void)
{
    static atomic_t device_indexes = ATOMIC_INIT(0);
    return atomic_inc_return(&device_indexes) - 1;
}
```

It only guarantees uniqueness; it does not encode any node or MC
information. When firmware lacks the "mc-idx" _DSD property on a
multi-node system and the probes run concurrently on different CPUs,
There is no guarantee that `alloc_index()` values map to physical
MC order. Decoding `mc_idx / mc_per_node` and `mc_idx % mc_per_node`
from this counter can report a wrong location.
>>  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)
> In a prepatch, please remove the "edac_" prefix from all your driver-local
> definitions like edac_probe() or loongson_edac_pvt. The edac_ namespace
> belongs to the EDAC core functions. Since your functions are static, you can
> simply call them probe(), check() etc.
Will fix.
>>  	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;
> ...
>
>>  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;
>>  	int ret;
>> +	bool mc_idx_valid = true;

If this bool variable serves a real purpose, I'll place it between the u32 and int.


Thanks,

Qunqin.

> The EDAC tree preferred ordering of variable declarations at the
> beginning of a function is reverse fir tree order::
>
> 	struct long_struct_name *descriptive_name;
> 	unsigned long foo, bar;
> 	unsigned int tmp;
> 	int ret;
>
> The above is faster to parse than the reverse ordering::
>
> 	int ret;
> 	unsigned int tmp;
> 	unsigned long foo, bar;
> 	struct long_struct_name *descriptive_name;
>
> And even more so than random ordering::
>
> 	unsigned long foo, bar;
> 	int ret;
> 	struct long_struct_name *descriptive_name;
> 	unsigned int tmp;
>
> Thx.
>