Re: [PATCH v18 04/19] EDAC: Add memory repair control feature

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Tue, 14 Jan 2025 14:30:53 +0000
Shiju Jose <shiju.jose@huawei.com> escreveu:

> >-----Original Message-----
> >From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> >Sent: 14 January 2025 13:47
> >To: Shiju Jose <shiju.jose@huawei.com>
> >Cc: linux-edac@vger.kernel.org; linux-cxl@vger.kernel.org; linux-
> >acpi@vger.kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org;
> >bp@alien8.de; tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org;
> >mchehab@kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan
> >Cameron <jonathan.cameron@huawei.com>; dave.jiang@intel.com;
> >alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com;
> >david@redhat.com; Vilas.Sridharan@amd.com; leo.duran@amd.com;
> >Yazen.Ghannam@amd.com; rientjes@google.com; jiaqiyan@google.com;
> >Jon.Grimm@amd.com; dave.hansen@linux.intel.com;
> >naoya.horiguchi@nec.com; james.morse@arm.com; jthoughton@google.com;
> >somasundaram.a@hpe.com; erdemaktas@google.com; pgonda@google.com;
> >duenwen@google.com; gthelen@google.com;
> >wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
> >wbs@os.amperecomputing.com; nifan.cxl@gmail.com; tanxiaofei
> ><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Roberto
> >Sassu <roberto.sassu@huawei.com>; kangkang.shen@futurewei.com;
> >wanghuiqiang <wanghuiqiang@huawei.com>; Linuxarm
> ><linuxarm@huawei.com>
> >Subject: Re: [PATCH v18 04/19] EDAC: Add memory repair control feature
> >
> >Em Mon, 6 Jan 2025 12:10:00 +0000
> ><shiju.jose@huawei.com> escreveu:
> >  
> >> +What:		/sys/bus/edac/devices/<dev-
> >name>/mem_repairX/repair_function
> >> +Date:		Jan 2025
> >> +KernelVersion:	6.14
> >> +Contact:	linux-edac@vger.kernel.org
> >> +Description:
> >> +		(RO) Memory repair function type. For eg. post package repair,
> >> +		memory sparing etc.
> >> +		EDAC_SOFT_PPR - Soft post package repair
> >> +		EDAC_HARD_PPR - Hard post package repair
> >> +		EDAC_CACHELINE_MEM_SPARING - Cacheline memory sparing
> >> +		EDAC_ROW_MEM_SPARING - Row memory sparing
> >> +		EDAC_BANK_MEM_SPARING - Bank memory sparing
> >> +		EDAC_RANK_MEM_SPARING - Rank memory sparing
> >> +		All other values are reserved.
> >> +
> >> +What:		/sys/bus/edac/devices/<dev-
> >name>/mem_repairX/persist_mode
> >> +Date:		Jan 2025
> >> +KernelVersion:	6.14
> >> +Contact:	linux-edac@vger.kernel.org
> >> +Description:
> >> +		(RW) Read/Write the current persist repair mode set for a
> >> +		repair function. Persist repair modes supported in the
> >> +		device, based on the memory repair function is temporary
> >> +		or permanent and is lost with a power cycle.
> >> +		EDAC_MEM_REPAIR_SOFT - Soft repair function (temporary  
> >repair).  
> >> +		EDAC_MEM_REPAIR_HARD - Hard memory repair function  
> >(permanent repair).  
> >> +		All other values are reserved.
> >> +  
> >
> >After re-reading some things, I suspect that the above can be simplified a little
> >bit by folding soft/hard PPR into a single element at /repair_function, and letting
> >it clearer that persist_mode is valid only for PPR (I think this is the case, right?),
> >e.g. something like:  
> persist_mode is valid for memory sparing features(atleast in CXL) as well.
> In the case of CXL memory sparing, host has option to request either soft or hard sparing
> in a flag when issue a memory sparing operation.

Ok.

> 
> >
> >	What:		/sys/bus/edac/devices/<dev-  
> >name>/mem_repairX/repair_function  
> >	...
> >	Description:
> >			(RO) Memory repair function type. For e.g. post
> >package repair,
> >			memory sparing etc. Valid values are:
> >
> >			- ppr - post package repair.
> >			  Please define its mode via
> >			  /sys/bus/edac/devices/<dev-  
> >name>/mem_repairX/persist_mode  
> >			- cacheline-sparing - Cacheline memory sparing
> >			- row-sparing - Row memory sparing
> >			- bank-sparing - Bank memory sparing
> >			- rank-sparing - Rank memory sparing
> >			- All other values are reserved.
> >
> >and define persist_mode in a different way:  
> Note: For return as decoded strings instead of raw value,  I need to add some extra callback function/s
> in the edac/memory_repair.c  for these attributes  and which will reduce the current level of optimization done to
> minimize the code size.

You're already using a callback at EDAC_MEM_REPAIR_ATTR_SHOW macro.
So, no need for any change at the current code, except for the type
used at the EDAC_MEM_REPAIR_ATTR_SHOW() call.

Something similar to this (not tested) would work:

    int get_repair_function(struct device *dev, void *drv_data, const char **val)
    {
	unsigned int type;

	// Some logic to get repair type from *drv_data, storing into "unsigned int type"

	const char *repair_type[] = {
		[EDAC_SOFT_PPR] = "ppr",
		[EDAC_HARD_PPR] = "ppr",
		[EDAC_CACHELINE_MEM_SPARING] = "cacheline-sparing",
		...
	}

	if (type < ARRAY_SIZE(repair_type)) {
		*val = repair_type(type);
		return 0;
	}

	return -EINVAL;
    }

    EDAC_MEM_REPAIR_ATTR_SHOW(repair_function, get_repair_function, const char *, "%s\n");

Thanks,
Mauro