Re: [PATCH v6 3/8] cxl/edac: Add CXL memory device patrol scrub control feature

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 21 May 2025 13:47:41 +0100
<shiju.jose@huawei.com> wrote:

> From: Shiju Jose <shiju.jose@huawei.com>
> 
> CXL spec 3.2 section 8.2.10.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/.
> 
> [dj: Add cxl_test inclusion of edac.o]
> [dj: Check return from cxl_feature_info() with IS_ERR]

Trivial question on these.  What do they reflect?  Some changes
Dave made on a prior version? Or changes in response to feedback
(in which case they should be below the ---)

> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>

A couple of formatting trivial things inline from the refactors
in this version. Maybe Dave can tweak them whilst applying if
nothing else comes up?

J

> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> new file mode 100644
> index 000000000000..eae99ed7c018
> --- /dev/null
> +++ b/drivers/cxl/core/edac.c
> @@ -0,0 +1,520 @@

> +static int cxl_scrub_get_attrbs(struct cxl_patrol_scrub_context *cxl_ps_ctx,
> +				u8 *cap, u16 *cycle, u8 *flags, u8 *min_cycle)
> +{
> +	struct cxl_mailbox *cxl_mbox;
> +	u8 min_scrub_cycle = U8_MAX;
> +	struct cxl_region_params *p;
> +	struct cxl_memdev *cxlmd;
> +	struct cxl_region *cxlr;
> +	int i, ret;
> +
> +	if (!cxl_ps_ctx->cxlr) {
> +		cxl_mbox = &cxl_ps_ctx->cxlmd->cxlds->cxl_mbox;
> +		return cxl_mem_scrub_get_attrbs(cxl_mbox, cap, cycle,
> +						flags, min_cycle);
> +	}
> +
> +	struct rw_semaphore *region_lock __free(rwsem_read_release) =
> +	rwsem_read_intr_acquire(&cxl_region_rwsem);

Trivial but that should be indented one tab more.

> +	if (!region_lock)
> +		return -EINTR;
> +
> +	cxlr = cxl_ps_ctx->cxlr;
> +	p = &cxlr->params;
> +
> +	for (i = 0; i < p->nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> +
> +		cxlmd = cxled_to_memdev(cxled);
> +		cxl_mbox = &cxlmd->cxlds->cxl_mbox;
> +		ret = cxl_mem_scrub_get_attrbs(cxl_mbox, cap, cycle,
> +					       flags, min_cycle);

Maybe move flags to previous line.

> +		if (ret)
> +			return ret;
> +
> +		if (min_cycle)
> +			min_scrub_cycle =
> +				min(*min_cycle, min_scrub_cycle);

No need for the line wrap any more.


> +	}
> +
> +	if (min_cycle)
> +		*min_cycle = min_scrub_cycle;
> +
> +	return 0;
> +}