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

[Shiju Jose <shiju.jose@huawei.com>]

>-----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
>>