linux-kernel - Re: [PATCH v2 2/2] Loongarch: EDAC driver for loongson memory controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cca91e87-ea98-5b16-0503-4d51fd524f24@loongson.cn>
Date: Tue, 3 Sep 2024 17:59:14 +0800
From: Zhao Qunqin <zhaoqunqin@...ngson.cn>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 chenhuacai@...nel.org, linux-edac@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, kernel@...0n.name,
 bp@...en8.de, tony.luck@...el.com, james.morse@....com, mchehab@...nel.org,
 rric@...nel.org, loongarch@...ts.linux.dev
Subject: Re: [PATCH v2 2/2] Loongarch: EDAC driver for loongson memory
 controller


在 2024/9/3 下午3:58, Krzysztof Kozlowski 写道:
> On 03/09/2024 09:24, Zhao Qunqin wrote:
>> 在 2024/9/3 下午2:47, Krzysztof Kozlowski 写道:
>>> On Tue, Sep 03, 2024 at 09:53:54AM +0800, Zhao Qunqin wrote:
>>>> Report single bit errors (CE) only.
>>>>
>>>> Signed-off-by: Zhao Qunqin <zhaoqunqin@...ngson.cn>
>>>> ---
>>>>    MAINTAINERS                  |   1 +
>>>>    arch/loongarch/Kconfig       |   1 +
>>>>    drivers/edac/Kconfig         |   8 ++
>>>>    drivers/edac/Makefile        |   1 +
>>>>    drivers/edac/ls3a5000_edac.c | 187 +++++++++++++++++++++++++++++++++++
>>>>    5 files changed, 198 insertions(+)
>>>>    create mode 100644 drivers/edac/ls3a5000_edac.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 6cc8cfc8f..b43f82279 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -13242,6 +13242,7 @@ M:	Zhao Qunqin <zhaoqunqin@...ngson.cn>
>>>>    L:	linux-edac@...r.kernel.org
>>>>    S:	Maintained
>>>>    F:	Documentation/devicetree/bindings/edac/loongson,ls3a5000-mc-edac.yaml
>>>> +F:	drivers/edac/ls3a5000_edac.c
>>>>    
>>>>    LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI)
>>>>    M:	Sathya Prakash <sathya.prakash@...adcom.com>
>>>> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
>>>> index 70f169210..348030c24 100644
>>>> --- a/arch/loongarch/Kconfig
>>>> +++ b/arch/loongarch/Kconfig
>>>> @@ -182,6 +182,7 @@ config LOONGARCH
>>>>    	select PCI_QUIRKS
>>>>    	select PERF_USE_VMALLOC
>>>>    	select RTC_LIB
>>>> +	select EDAC_SUPPORT
>>> I think you got here comment before. How did you address it?
>> I just randomly found a spot, and I will put it at the end(next version
>> patch).
> No, the comment was different. You must not select user-visible symbols.

Last version patch is:

@@ -182,6 +182,7 @@ config LOONGARCH
   	select PCI_QUIRKS
   	select PERF_USE_VMALLOC
   	select RTC_LIB
+	select EDAC_SUPPORT
+	select EDAC

I delet "select EDAC" in this version patch. Other arch's Kconfig has "select EDAC_SUPPORT", such as arch/x86/Kconfig and arch/arm64/Kconfig
.

>
>>>>    	select SPARSE_IRQ
>>>>    	select SYSCTL_ARCH_UNALIGN_ALLOW
>>>>    	select SYSCTL_ARCH_UNALIGN_NO_WARN
>>>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>>>> index 16c8de505..2d10256f0 100644
>>>> --- a/drivers/edac/Kconfig
>>>> +++ b/drivers/edac/Kconfig
>>>> @@ -573,5 +573,13 @@ config EDAC_VERSAL
>>>>    	  Support injecting both correctable and uncorrectable errors
>>>>    	  for debugging purposes.
>>>>    
>>> ...
>>>
>>>    +
>>>> +static int loongson_edac_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct resource *rs;
>>>> +	struct mem_ctl_info *mci;
>>>> +	struct edac_mc_layer layers[2];
>>>> +	struct loongson_edac_pvt *pvt;
>>>> +	u64 *vbase = NULL;
>>>> +
>>>> +	rs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	/* not return if can not find resource or resource start equals NULL */
>>> Why?
>> Because there are multiple memory controllers in the ls3x soc,
>>
>> but the ECC function of some memory controllers cannot be used.
> Then what does the driver do for such memory controllers? Your binding
> is quite clear here that above code is just bogus. It is not possible to
> have node without reg.

I will rewrite the code above.

> Please point us to your DTS and results of dtbs_check.
>
>> But in any case, a node must be created in /sys/devices/system/edac/mc/
>> through edac_mc_add_mc(mci).
>>
>> Then if the ECC function of the memory controller cannot be used, set
>> start to NULL or do not pass mem resource,
>>
>> which is equivalent to enumeration of memory controller, and the CE
>> count will always be zero.
>>
>>>> +	if (rs && rs->start) {
>>>> +		vbase = devm_ioremap_resource(&pdev->dev, rs);
>>>> +		if (IS_ERR(vbase))
>>>> +			return PTR_ERR(vbase);
>>>> +	}
>
> Best regards,
> Krzysztof