linux-kernel - Re: [PATCH V10 RESEND] EDAC: Add 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] [day] [month] [year] [list]

Message-ID: <2d4e5bd0-34dc-d781-3bd6-9eb7be2c1b17@loongson.cn>
Date: Tue, 17 Dec 2024 10:25:25 +0800
From: Zhao Qunqin <zhaoqunqin@...ngson.cn>
To: Borislav Petkov <bp@...en8.de>
Cc: chenhuacai@...nel.org, linux-edac@...r.kernel.org,
 linux-kernel@...r.kernel.org, kernel@...0n.name, tony.luck@...el.com,
 james.morse@....com, mchehab@...nel.org, rric@...nel.org,
 loongarch@...ts.linux.dev, xry111@...111.site, Markus.Elfring@....de,
 Jonathan.Cameron@...wei.com, Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH V10 RESEND] EDAC: Add EDAC driver for loongson memory
 controller


在 2024/12/16 下午7:55, Borislav Petkov 写道:
> On Mon, Dec 16, 2024 at 09:33:51AM +0800, Zhao Qunqin wrote:
>> +LOONGSON EDAC DRIVER
>> +M:	Zhao Qunqin <zhaoqunqin@...ngson.cn>
>> +L:	linux-edac@...r.kernel.org
>> +S:	Maintained
>> +F:	drivers/edac/loongson_edac.c
> If you add yourself as a maintainer, I'd expect you to review and/or ack
> patches for your driver so that I can pick them up.
OK. I can review the patches for this driver.
>
>> +config EDAC_LOONGSON
>> +	tristate "Loongson Memory Controller"
>> +	depends on (LOONGARCH && ACPI) || COMPILE_TEST
> The COMPILE_TEST thing would mean that you'll make sure this driver always
> builds on other arches and it doesn't break randconfig builds of people. If it
> happens too often and no one is fixing it, I'll remove the COMPILE_TEST.
There have indeed been build errors on other arches. So i will remove it.
>> +	help
>> +	  Support for error detection and correction on the Loongson
>> +	  family memory controller. This driver reports single bit
>> +	  errors (CE) only. Loongson-3A5000/3C5000/3D5000/3A6000/3C6000
>> +	  are compatible.
>>   
>>   endif # EDAC
>> +static int read_ecc(struct mem_ctl_info *mci)
>> +{
>> +	struct loongson_edac_pvt *pvt = mci->pvt_info;
>> +	u64 ecc;
>> +	int cs;
>> +
>> +	if (!pvt->ecc_base)
> When can that even happen? You're initializing it properly in pvt_init().
Will remove this check.
>> +		return pvt->last_ce_count;
>> +
>> +	ecc = readq(pvt->ecc_base + ECC_CS_COUNT_REG);
>> +	/* cs0 -- cs3 */
>> +	cs = ecc & 0xff;
>> +	cs += (ecc >> 8) & 0xff;
>> +	cs += (ecc >> 16) & 0xff;
>> +	cs += (ecc >> 24) & 0xff;
>> +
>> +	return cs;
>> +}
>> +
>> +static void edac_check(struct mem_ctl_info *mci)
>> +{
>> +	struct loongson_edac_pvt *pvt = mci->pvt_info;
>> +	int new, add;
>> +
>> +	new = read_ecc(mci);
>> +	add = new - pvt->last_ce_count;
>> +	pvt->last_ce_count = new;
> That last_ce_count is just silly. Kill it.

Then  I  can't calculate the error count added since the last check,  cause

what record in Loongson's ECC register is  the error count  from reset of

the memory controller.

>> +	if (add <= 0)
>> +		return;
>> +
>> +	edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, add,
>> +			     0, 0, 0, 0, 0, -1, "error", "");
>> +	edac_mc_printk(mci, KERN_INFO, "add: %d", add);
> "add"? What are you adding? Error count?
>
> No need.
Will remove this printk.
>> +static int edac_probe(struct platform_device *pdev)
>> +{
>> +	struct edac_mc_layer layers[2];
>> +	struct mem_ctl_info *mci;
>> +	void __iomem *vbase;
>> +	int ret;
>> +
>> +	vbase = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(vbase))
>> +		return PTR_ERR(vbase);
>> +
>> +	/* allocate a new MC control structure */
>> +	layers[0].type = EDAC_MC_LAYER_CHANNEL;
>> +	layers[0].size = 1;
>> +	layers[0].is_virt_csrow = false;
>> +	layers[1].type = EDAC_MC_LAYER_SLOT;
>> +	layers[1].size = 1;
>> +	layers[1].is_virt_csrow = true;
>> +	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> +			    sizeof(struct loongson_edac_pvt));
>> +	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;
>> +	mci->mod_name = "loongson_edac.c";
>> +	mci->ctl_name = "loongson_edac_ctl";
>> +	mci->dev_name = "loongson_edac_dev";
>> +	mci->ctl_page_to_phys = NULL;
>> +	mci->pdev = &pdev->dev;
>> +	mci->error_desc.grain = 8;
>> +	/* Set the function pointer to an actual operation function */
> Remove that obvious comment.

OK

Thanks for taking the time to review.

Best regards

>