Shiju Jose wrote:
> Hi Dan,
>
> Thanks for the comments.
>
> Please find reply inline.
>
> Thanks,
> Shiju
> >-----Original Message-----
> >From: Dan Williams <dan.j.williams@xxxxxxxxx>
> >Sent: 24 January 2025 20:39
> >To: Shiju Jose <shiju.jose@xxxxxxxxxx>; linux-edac@xxxxxxxxxxxxxxx; linux-
> >cxl@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; linux-
> >kernel@xxxxxxxxxxxxxxx
> >Cc: bp@xxxxxxxxx; tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
> >mchehab@xxxxxxxxxx; dan.j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx; Jonathan
> >Cameron <jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx;
> >alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx;
> >david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; leo.duran@xxxxxxx;
> >Yazen.Ghannam@xxxxxxx; rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx;
> >Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx;
> >naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx;
> >somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx;
> >duenwen@xxxxxxxxxx; gthelen@xxxxxxxxxx;
> >wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx;
> >wbs@xxxxxxxxxxxxxxxxxxxxxx; nifan.cxl@xxxxxxxxx; tanxiaofei
> ><tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; Roberto
> >Sassu <roberto.sassu@xxxxxxxxxx>; kangkang.shen@xxxxxxxxxxxxx;
> >wanghuiqiang <wanghuiqiang@xxxxxxxxxx>; Linuxarm
> ><linuxarm@xxxxxxxxxx>; Shiju Jose <shiju.jose@xxxxxxxxxx>
> >Subject: Re: [PATCH v18 15/19] cxl/memfeature: Add CXL memory device patrol
> >scrub control feature
> >
> >shiju.jose@ wrote:
> >> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
> >>
> >> CXL spec 3.1 section 8.2.9.9.11.1 describes the device patrol scrub
> >> control feature. The device patrol scrub proactively locates and makes
> >> corrections to errors in regular cycle.
> >>
> >> Allow specifying the number of hours within which the patrol scrub
> >> must be completed, subject to minimum and maximum limits reported by the
> >device.
> >> Also allow disabling scrub allowing trade-off error rates against
> >> performance.
> >>
> >> Add support for patrol scrub control on CXL memory devices.
> >> Register with the EDAC device driver, which retrieves the scrub
> >> attribute descriptors from EDAC scrub and exposes the sysfs scrub
> >> control attributes to userspace. For example, scrub control for the
> >> CXL memory device "cxl_mem0" is exposed in
> >/sys/bus/edac/devices/cxl_mem0/scrubX/.
> >>
> >> Additionally, add support for region-based CXL memory patrol scrub control.
> >> CXL memory regions may be interleaved across one or more CXL memory
> >> devices. For example, region-based scrub control for "cxl_region1" is
> >> exposed in /sys/bus/edac/devices/cxl_region1/scrubX/.
> >>
> >> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> >> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >> Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
> >> ---
> >> Documentation/edac/scrub.rst | 66 ++++++
> >> drivers/cxl/Kconfig | 17 ++
> >> drivers/cxl/core/Makefile | 1 +
> >> drivers/cxl/core/memfeature.c | 392
> >++++++++++++++++++++++++++++++++++
> >> drivers/cxl/core/region.c | 6 +
> >> drivers/cxl/cxlmem.h | 7 +
> >> drivers/cxl/mem.c | 5 +
> >> include/cxl/features.h | 16 ++
> >> 8 files changed, 510 insertions(+)
> >> create mode 100644 drivers/cxl/core/memfeature.c diff --git
> >> a/Documentation/edac/scrub.rst b/Documentation/edac/scrub.rst index
> >> f86645c7f0af..80e986c57885 100644
> >> --- a/Documentation/edac/scrub.rst
> >> +++ b/Documentation/edac/scrub.rst
[..]
> >
> >What is this content-free blob of cat and echo statements? Please write actual
> >documentation with theory of operation, clarification of assumptions, rationale
> >for defaults, guidance on changing defaults...
>
> Jonathan already replied.
I disagree that any of that is useful to include without rationale, and
if the rationale is already somewhere else then delete the multiple
lines of showing how 'cat' and 'echo' work with sysfs.
[..]
> >> + depends on CXL_MEM
> >
> >Similar comment, and this also goes away if all of this just moves into the new
> >cxl_features driver.
>
> Agree with Jonathan told in reply. These are RAS specific features for CXL memory devices and
> thus added in memfeature.c
Apoligies for this comment, I had meant to delete it along with some
other commentary along this theme after thinking it through.
I am now advocating that Dave drop his cxl_features driver altogether
and mirror your approach. I.e. EDAC is registered from existing CXL
drivers, and FWCTL can be registered against a cxl_memdev just like the
fw_upload ABI.
There was a concern that CXL needed a separate FWCTL driver in case
distributions wanted to have a policy against FWCTL, but given CXL
already has CONFIG_CXL_MEM_RAW_COMMANDS at compile-time and a wide array
of CXL bus devices, a cxl_features device is an awkward fit.
[..]
> >> +static int cxl_ps_get_attrs(struct cxl_patrol_scrub_context *cxl_ps_ctx,
> >> + struct cxl_memdev_ps_params *params) {
> >> + struct cxl_memdev *cxlmd;
> >> + u16 min_scrub_cycle = 0;
> >> + int i, ret;
> >> +
> >> + if (cxl_ps_ctx->cxlr) {
> >> + struct cxl_region *cxlr = cxl_ps_ctx->cxlr;
> >> + struct cxl_region_params *p = &cxlr->params;
> >> +
> >> + for (i = p->interleave_ways - 1; i >= 0; i--) {
> >> + struct cxl_endpoint_decoder *cxled = p->targets[i];
> >
> >It looks like this is called directly as a callback from EDAC. Where is the locking
> >that keeps cxl_ps_ctx->cxlr valid, or p->targets content stable?
> Jonathan already replied.
I could not find that comment? I *think* it's ok because when the region
is in the probe state changes will not be made to this list, but it
would be useful to at least have commentary to that effect. Protect
against someone copying this code in isolation and not consider the
context.
[..]
> >> +
> >> +int cxl_mem_ras_features_init(struct cxl_memdev *cxlmd, struct
> >> +cxl_region *cxlr)
> >
> >Please separate this into a memdev helper and a region helper. It is silly to have
> >two arguments to a function where one is expected to be NULL at all times, and
> >then have an if else statement inside that to effectively turn it back into 2 code
> >paths.
> >
> >If there is code to be shared amongst those, make *that* the shared helper.
> I added single function cxl_mem_ras_features_init() for both memdev and region based
> scrubbing to reduce code size as there were feedbacks try reduce code size.
"Succint" and "concise" does not necessarily mean less lines. I would
greatly prefer a few more lines if it mines not outsourcing complexity
to the calling context. Readable code means I do not need to wonder
what:
cxl_mem_ras_features_init(NULL, cxlr)
...means. I can just read devm_cxl_region_edac_register(cxlr), and know
exactly what is happening without needing to lose my train of thought to
go read what semantics cxl_mem_ras_features_init() is implementing.
Note that all the other _init() calls in drivers/cxl/ (outside of
module_init callbacks), are just purely init work, not object
registration. Please keep that local style.
> >> +{
> >> + struct edac_dev_feature ras_features[CXL_DEV_NUM_RAS_FEATURES];
> >> + char cxl_dev_name[CXL_DEV_NAME_LEN];
> >> + int num_ras_features = 0;
> >> + u8 scrub_inst = 0;
> >> + int rc;
> >> +
> >> + rc = cxl_memdev_scrub_init(cxlmd, cxlr,
> >&ras_features[num_ras_features],
> >> + scrub_inst);
> >> + if (rc < 0)
> >> + return rc;
> >> +
> >> + scrub_inst++;
> >> + num_ras_features++;
> >> +
> >> + if (cxlr)
> >> + snprintf(cxl_dev_name, sizeof(cxl_dev_name),
> >> + "cxl_region%d", cxlr->id);
> >
> >Why not pass dev_name(&cxlr->dev) directly?
> Jonathan already replied.
That was purely with the cxl_mem observation, cxlr can be passed
directly.
> >
> >> + else
> >> + snprintf(cxl_dev_name, sizeof(cxl_dev_name),
> >> + "%s_%s", "cxl", dev_name(&cxlmd->dev));
> >
> >Can a "cxl" directory be created so that the raw name can be used?
In fact we already do something similar for CONFIG_HMEM_REPORTING (i.e.
an "access%d" device to create a nameed directory of attributes) so it
is a question for Boris if he wants to tolerate a parent "cxl" device to
parent all CXL objects in EDAC.
> >
> >> +
> >> + return edac_dev_register(&cxlmd->dev, cxl_dev_name, NULL,
> >> + num_ras_features, ras_features);
> >
> >I'm so confused... a few lines down in this patch we have:
> >
> > rc = cxl_mem_ras_features_init(NULL, cxlr);
> >
> >...so how can this call to edac_dev_register() unconditionally de-reference
> >@cxlmd?
> Thanks for spotting this. It is a bug, need to fix.
[..]
> >> +EXPORT_SYMBOL_NS_GPL(cxl_mem_ras_features_init, "CXL");
> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> >> index b98b1ccffd1c..c2be70cd87f8 100644
> >> --- a/drivers/cxl/core/region.c
> >> +++ b/drivers/cxl/core/region.c
> >> @@ -3449,6 +3449,12 @@ static int cxl_region_probe(struct device *dev)
> >> p->res->start, p->res->end, cxlr,
> >> is_system_ram) > 0)
> >> return 0;
> >> +
> >> + rc = cxl_mem_ras_features_init(NULL, cxlr);
> >> + if (rc)
> >> + dev_warn(&cxlr->dev, "CXL RAS features init for
> >region_id=%d failed\n",
> >> + cxlr->id);
> >
> >There is more to RAS than EDAC memory scrub so this message is misleading. It
> >is also unnecessary because the driver continues to load and the admin, if they
> >care, will notice that the EDAC attributes are missing.
> This message was added for the debugging purpose in CXL driver. I will change to dev_dbg().
...but also stop calling this functionality with the blanket term "RAS".
It is "EDAC scrub and repair extensions to all the other RAS
functionality the CXL subsystem handles directly", name it accordingly.
