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

[Zhao Qunqin]

在 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@xxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxx>
> >   L:	linux-edac@xxxxxxxxxxxxxxx
> >   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@xxxxxxxxxxxx>
> > 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).
> >   	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.

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);
> > +	}
> > +
> > +	/* 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(*pvt));
> > +	if (mci == NULL)
> > +		return -ENOMEM;
> > +
> > +	edac_dbg(0, "MC: mci = %p\n", mci);
> Nope, drop. You should never print pointers.
I will drop it in next version patch.
> > +
> > +	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 */
> > +	mci->edac_check = loongson_edac_check;
> > +
> > +	loongson_pvt_init(mci, vbase);
> > +	get_dimm_config(mci);
> > +
> > +	if (edac_mc_add_mc(mci)) {
> > +		edac_dbg(0, "MC: failed edac_mc_add_mc()\n");
> > +		edac_mc_free(mci);
> That's still a success of probe? Weird a bit.

I will add return ret if (ret =edac_mc_add_mc(mci))  in next version patch.

> > +	}
> > +	edac_op_state = EDAC_OPSTATE_POLL;
> > +
> > +	return 0;
> > +}
> > +
> > +static void loongson_edac_remove(struct platform_device *pdev)
> > +{
> > +	struct mem_ctl_info *mci = edac_mc_del_mc(&pdev->dev);
> > +
> > +	if (mci)
> > +		edac_mc_free(mci);
> > +}
> > +
> > +static const struct of_device_id loongson_edac_of_match[] = {
> > +	{ .compatible = "loongson,ls3a5000-mc-edac", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, loongson_edac_of_match);
> > +
> > +static struct platform_driver loongson_edac_driver = {
> > +	.probe		= loongson_edac_probe,
> > +	.remove		= loongson_edac_remove,
> > +	.driver		= {
> > +		.name	= "ls-mc-edac",
> > +		.of_match_table = loongson_edac_of_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(loongson_edac_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Zhao Qunqin <zhaoqunqin@xxxxxxxxxxx>\n");
> Drop \n.
I will drop it in next version patch.
> > +MODULE_DESCRIPTION("EDAC driver for loongson memory controller");
> > --
> > 2.43.0
Best regards,

Zhao Qunqin.