Re: [PATCH v14 07/14] cxl/memfeature: Add CXL memory device patrol scrub control feature

[Dave Jiang <dave.jiang@intel.com>]

On 10/29/24 10:00 AM, Shiju Jose wrote:
> 
> 
>> -----Original Message-----
>> From: Dave Jiang <dave.jiang@intel.com>
>> Sent: 29 October 2024 16:32
>> To: Shiju Jose <shiju.jose@huawei.com>; linux-edac@vger.kernel.org; linux-
>> cxl@vger.kernel.org; linux-acpi@vger.kernel.org; linux-mm@kvack.org; linux-
>> kernel@vger.kernel.org
>> Cc: bp@alien8.de; tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org;
>> mchehab@kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan
>> Cameron <jonathan.cameron@huawei.com>; gregkh@linuxfoundation.org;
>> sudeep.holla@arm.com; jassisinghbrar@gmail.com; alison.schofield@intel.com;
>> vishal.l.verma@intel.com; ira.weiny@intel.com; david@redhat.com;
>> Vilas.Sridharan@amd.com; leo.duran@amd.com; Yazen.Ghannam@amd.com;
>> rientjes@google.com; jiaqiyan@google.com; Jon.Grimm@amd.com;
>> dave.hansen@linux.intel.com; naoya.horiguchi@nec.com;
>> james.morse@arm.com; jthoughton@google.com; somasundaram.a@hpe.com;
>> erdemaktas@google.com; pgonda@google.com; duenwen@google.com;
>> gthelen@google.com; wschwartz@amperecomputing.com;
>> dferguson@amperecomputing.com; wbs@os.amperecomputing.com;
>> nifan.cxl@gmail.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>;
>> Linuxarm <linuxarm@huawei.com>
>> Subject: Re: [PATCH v14 07/14] cxl/memfeature: Add CXL memory device patrol
>> scrub control feature
>>
>>
>>
>> On 10/25/24 10:13 AM, shiju.jose@huawei.com wrote:
>>> From: Shiju Jose <shiju.jose@huawei.com>
>>>
>>> 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/.
>>>
>>> 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>
>>> ---
>>>  Documentation/edac/edac-scrub.rst |  74 ++++++
>>>  drivers/cxl/Kconfig               |  18 ++
>>>  drivers/cxl/core/Makefile         |   1 +
>>>  drivers/cxl/core/memfeature.c     | 381 ++++++++++++++++++++++++++++++
>>>  drivers/cxl/core/region.c         |   6 +
>>>  drivers/cxl/cxlmem.h              |   7 +
>>>  drivers/cxl/mem.c                 |   4 +
>>>  7 files changed, 491 insertions(+)
>>>  create mode 100644 Documentation/edac/edac-scrub.rst  create mode
>>> 100644 drivers/cxl/core/memfeature.c
>>>
>>> diff --git a/Documentation/edac/edac-scrub.rst
>>> b/Documentation/edac/edac-scrub.rst
>>> new file mode 100644
>>> index 000000000000..4aad4974b208
>>> --- /dev/null
>>> +++ b/Documentation/edac/edac-scrub.rst
>>> @@ -0,0 +1,74 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
> [...]
> 
>>> +static int cxl_mem_ps_get_attrs(struct cxl_memdev_state *mds,
>>> +				struct cxl_memdev_ps_params *params) {
>>> +	size_t rd_data_size = sizeof(struct cxl_memdev_ps_rd_attrs);
>>> +	size_t data_size;
>>> +	struct cxl_memdev_ps_rd_attrs *rd_attrs __free(kfree) =
>>> +						kmalloc(rd_data_size,
>> GFP_KERNEL);
>>> +	if (!rd_attrs)
>>> +		return -ENOMEM;
>>> +
>>> +	data_size = cxl_get_feature(mds, cxl_patrol_scrub_uuid,
>>> +				    CXL_GET_FEAT_SEL_CURRENT_VALUE,
>>> +				    rd_attrs, rd_data_size);
>>> +	if (!data_size)
>>> +		return -EIO;
>>> +
>>> +	params->scrub_cycle_changeable =
>> FIELD_GET(CXL_MEMDEV_PS_SCRUB_CYCLE_CHANGE_CAP_MASK,
>>> +						   rd_attrs->scrub_cycle_cap);
>>> +	params->enable =
>> FIELD_GET(CXL_MEMDEV_PS_FLAG_ENABLED_MASK,
>>> +				   rd_attrs->scrub_flags);
>>> +	params->scrub_cycle_hrs =
>> FIELD_GET(CXL_MEMDEV_PS_CUR_SCRUB_CYCLE_MASK,
>>> +					    rd_attrs->scrub_cycle_hrs);
>>> +	params->min_scrub_cycle_hrs =
>> FIELD_GET(CXL_MEMDEV_PS_MIN_SCRUB_CYCLE_MASK,
>>> +						rd_attrs->scrub_cycle_hrs);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cxl_ps_get_attrs(struct device *dev, void *drv_data,
>>
>> Would a union be better than a void *drv_data for all the places this is used as a
>> parameter? How many variations of this are there?
>>
>> DJ
> Hi Dave,
> 
> Can you give more info on this given this is a generic callback for the scrub control and each
> implementation will have its own context struct (for eg. struct cxl_patrol_scrub_context here
> for CXL scrub control), which in turn will be passed in and out as opaque data.

Mainly I'm just seeing a lot of calls with (void *). Just asking if we want to make it a union that contains 'struct cxl_patrol_scrub_context' and etc.

> 
> Thanks,
> Shiju
>>
>>> +			    struct cxl_memdev_ps_params *params) {
>>> +	struct cxl_patrol_scrub_context *cxl_ps_ctx = drv_data;
>>> +	struct cxl_memdev *cxlmd;
>>> +	struct cxl_dev_state *cxlds;
>>> +	struct cxl_memdev_state *mds;
>>> +	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];
>>> +
>>> +			cxlmd = cxled_to_memdev(cxled);
>>> +			cxlds = cxlmd->cxlds;
>>> +			mds = to_cxl_memdev_state(cxlds);
>>> +			ret = cxl_mem_ps_get_attrs(mds, params);
>>> +			if (ret)
>>> +				return ret;
>>> +
>>> +			if (params->min_scrub_cycle_hrs > min_scrub_cycle)
>>> +				min_scrub_cycle = params-
>>> min_scrub_cycle_hrs;
>>> +		}
>>> +		params->min_scrub_cycle_hrs = min_scrub_cycle;
>>> +		return 0;
>>> +	}
>>> +	cxlmd = cxl_ps_ctx->cxlmd;
>>> +	cxlds = cxlmd->cxlds;
>>> +	mds = to_cxl_memdev_state(cxlds);
>>> +
>>> +	return cxl_mem_ps_get_attrs(mds, params); }
>>> +
> [...]
>>
>