Re: [PATCH v4 2/2] Loongarch: EDAC driver for loongson memory controller

[Zhao Qunqin]

在 2024/9/13 下午6:11, Borislav Petkov 写道:
> On Mon, Sep 09, 2024 at 11:21:24AM +0800, Zhao Qunqin wrote:
> > Subject: Re: [PATCH v4 2/2] Loongarch: EDAC driver for loongson memory controller
> 			EDAC/loongson: Add EDAC driver ...
>
> > Reports single bit errors (CE) only.
> >
> > Signed-off-by: Zhao Qunqin <zhaoqunqin@xxxxxxxxxxx>
> > ---
> ...
>
> > diff --git a/drivers/edac/loongson_edac.c b/drivers/edac/loongson_edac.c
> > new file mode 100644
> > index 000000000..b89d6e0e7
> > --- /dev/null
> > +++ b/drivers/edac/loongson_edac.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024 Loongson Technology Corporation Limited.
> > + */
> > +
> > +#include <linux/edac.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "edac_module.h"
> > +
> > +enum ecc_index {
> > +	ECC_SET = 0,
> > +	ECC_RESERVED,
> > +	ECC_COUNT,
> > +	ECC_CS_COUNT,
> > +	ECC_CODE,
> > +	ECC_ADDR,
> > +	ECC_DATA0,
> > +	ECC_DATA1,
> > +	ECC_DATA2,
> > +	ECC_DATA3,
> > +};
> > +
> > +struct loongson_edac_pvt {
> > +	u64 *ecc_base;
> > +	int last_ce_count;
> > +};
> > +
> > +static void loongson_update_ce_count(struct mem_ctl_info *mci,
> Drop the loongson_ prefix from all static functions.
>
> Also, align function arguments on the opening brace.
>
> > +					int chan,
> > +					int new)
> > +{
> > +	int add;
> > +	struct loongson_edac_pvt *pvt = mci->pvt_info;
> 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;
>
> > +
> > +	add = new - pvt->last_ce_count;
> > +
> > +	/* Store the new value */
> Drop all those obvious comments.
>
> > +	pvt->last_ce_count = new;
> > +
> > +	/* device resume or any other exceptions*/
> No clue what that means.

The CE value of the memory controller should only increase,

but it will return to its initial value when resuming from S3 or S4 
sleep state,

then the new ce count may be smaller than the last ce count.

> Also,  the check goes right under the assignment.
>
> > +	if (add < 0)
> > +		return;
> > +
> > +	/*updated the edac core */
> Useless comment.
>
> > +	if (add != 0) {
> 	if (!add)
> 		return;
>
> and now you can save yourself an indentation level:
>
> 	edac_mc_...(
>
> > +		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, add,
> > +					0, 0, 0,
> > +					chan, 0, -1, "error", "");
> > +		edac_mc_printk(mci, KERN_INFO, "add: %d", add);
> > +	}
> > +}
> > +
> > +static int loongson_read_ecc(struct mem_ctl_info *mci)
> > +{
> > +	u64 ecc;
> > +	int cs = 0;
> > +	struct loongson_edac_pvt *pvt = mci->pvt_info;
> > +
> > +	if (!pvt->ecc_base)
> > +		return pvt->last_ce_count;
> > +
> > +	ecc = pvt->ecc_base[ECC_CS_COUNT];
> > +	cs += ecc & 0xff;		// cs0
> > +	cs += (ecc >> 8) & 0xff;	// cs1
> > +	cs += (ecc >> 16) & 0xff;	// cs2
> > +	cs += (ecc >> 24) & 0xff;	// cs3
> No side comments pls - put them over the line.
>
> > +
> > +	return cs;
> > +}
> > +
> > +static void loongson_edac_check(struct mem_ctl_info *mci)
> > +{
> > +	loongson_update_ce_count(mci, 0, loongson_read_ecc(mci));
> Drop this silly wrapper.
>
> > +}
> > +
> > +static int get_dimm_config(struct mem_ctl_info *mci)
> > +{
> > +	u32 size, npages;
> > +	struct dimm_info *dimm;
> > +
> > +	/* size not used */
> > +	size = -1;
> > +	npages = MiB_TO_PAGES(size);
> > +
> > +	dimm = edac_get_dimm(mci, 0, 0, 0);
> > +	dimm->nr_pages = npages;
> > +	snprintf(dimm->label, sizeof(dimm->label),
> > +			"MC#%uChannel#%u_DIMM#%u",
> > +			mci->mc_idx, 0, 0);
> Align arguments on the opening brace.
>
> > +	dimm->grain = 8;
> > +
> > +	return 0;
> > +}

Will address above comments. Thanks for taking the time to review.

Best regards,

Zhao Qunqin.

>