>-----Original Message----- >From: Alison Schofield <alison.schofield@xxxxxxxxx> >Sent: 20 May 2025 02:36 >To: Shiju Jose <shiju.jose@xxxxxxxxxx> >Cc: linux-cxl@xxxxxxxxxxxxxxx; dan.j.williams@xxxxxxxxx; Jonathan Cameron ><jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx; dave@xxxxxxxxxxxx; >vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx; >linux-doc@xxxxxxxxxxxxxxx; bp@xxxxxxxxx; tony.luck@xxxxxxxxx; >lenb@xxxxxxxxxx; Yazen.Ghannam@xxxxxxx; mchehab@xxxxxxxxxx; >nifan.cxl@xxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; tanxiaofei ><tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; Roberto >Sassu <roberto.sassu@xxxxxxxxxx>; kangkang.shen@xxxxxxxxxxxxx; >wanghuiqiang <wanghuiqiang@xxxxxxxxxx> >Subject: Re: [PATCH v5 0/8] cxl: support CXL memory RAS features > >On Thu, May 15, 2025 at 12:59:16PM +0100, shiju.jose@xxxxxxxxxx wrote: >> From: Shiju Jose <shiju.jose@xxxxxxxxxx> >> >> 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 >>
