RE: [PATCH v5 0/8] cxl: support CXL memory RAS features

Shiju Jose
2025-05-20T23:44:18Z

>-----Original Message-----
>From: Alison Schofield <alison.schofield@intel.com>
>Sent: 20 May 2025 02:36
>To: Shiju Jose <shiju.jose@huawei.com>
>Cc: linux-cxl@vger.kernel.org; dan.j.williams@intel.com; Jonathan Cameron
><jonathan.cameron@huawei.com>; dave.jiang@intel.com; dave@stgolabs.net;
>vishal.l.verma@intel.com; ira.weiny@intel.com; linux-edac@vger.kernel.org;
>linux-doc@vger.kernel.org; bp@alien8.de; tony.luck@intel.com;
>lenb@kernel.org; Yazen.Ghannam@amd.com; mchehab@kernel.org;
>nifan.cxl@gmail.com; Linuxarm <linuxarm@huawei.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>
>Subject: Re: [PATCH v5 0/8] cxl: support CXL memory RAS features
>
>On Thu, May 15, 2025 at 12:59:16PM +0100, shiju.jose@huawei.com wrote:
>> From: Shiju Jose <shiju.jose@huawei.com>
>>
>> Support for CXL memory EDAC features: patrol scrub, ECS, soft-PPR and
>> memory sparing.
>
>snip
>
>>
>> Shiju Jose (8):
>> EDAC: Update documentation for the CXL memory patrol scrub control
>> feature
>> cxl: Update prototype of function get_support_feature_info()
>> cxl/edac: Add CXL memory device patrol scrub control feature
>> cxl/edac: Add CXL memory device ECS control feature
>> cxl/edac: Add support for PERFORM_MAINTENANCE command
>> cxl/edac: Support for finding memory operation attributes from the
>> current boot
>> cxl/edac: Add CXL memory device memory sparing control feature
>> cxl/edac: Add CXL memory device soft PPR control feature
>>
>> Documentation/edac/memory_repair.rst | 31 +
>> Documentation/edac/scrub.rst | 76 +
>> drivers/cxl/Kconfig | 71 +
>> drivers/cxl/core/Makefile | 1 +
>> drivers/cxl/core/core.h | 2 +
>> drivers/cxl/core/edac.c | 2082 ++++++++++++++++++++++++++
>
>Hi Shiju,
>
>The file edac.c contains a scattering of ifdef's that can be replaced with the
>IS_ENABLED() mechanism.
>
>This issue touches many patches, looks like patches 3-7 all added to edac.c.
>
>See Documentation/process/coding-style.rst "Conditional Compilation".
>You'll find a few usage example in drivers/cxl/ and of course many more all over
>the kernel.
>
>Conversely, in cxlmem.h, the patch uses IS_ENABLED() where an #ifdef is
>perfectly fine and expected. See the surrounding ifdef's in cxlmem.h
>
>I'm aware it's not a simple search and replace operation to rework this but it is
>worth doing now to make the code more readable forever, and there's also the
>benefit of allowing the compiler to check code inside the block for correctness.

Hi Alison,

Thanks for the feedback.

I replaced #ifdef with IS_ENABLED() in .c file and opposite in cxlmem.h.

Thanks,
Shiju

>
>-- Alison
>
>> drivers/cxl/core/features.c | 17 +-
>> drivers/cxl/core/mbox.c | 11 +-
>> drivers/cxl/core/memdev.c | 1 +
>> drivers/cxl/core/region.c | 10 +
>> drivers/cxl/cxl.h | 10 +
>> drivers/cxl/cxlmem.h | 36 +
>> drivers/cxl/mem.c | 4 +
>> drivers/edac/mem_repair.c | 9 +
>> include/linux/edac.h | 7 +
>> 15 files changed, 2356 insertions(+), 12 deletions(-) create mode
>> 100644 drivers/cxl/core/edac.c
>>
>> --
>> 2.43.0
>>