RE: [PATCH v17 05/18] cxl: Add Get Supported Features command for kernel usage

[Shiju Jose]

>-----Original Message-----
>From: Dan Williams <dan.j.williams@xxxxxxxxx>
>Sent: 06 December 2024 21:41
>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 v17 05/18] cxl: Add Get Supported Features command for
>kernel usage
>
>shiju.jose@ wrote:
>> From: Dave Jiang <dave.jiang@xxxxxxxxx>
>>
>> CXL spec r3.1 8.2.9.6.1 Get Supported Features (Opcode 0500h) The
>> command retrieve the list of supported device-specific features
>> (identified by UUID) and general information about each Feature.
>>
>> The driver will retrieve the feature entries in order to make checks
>> and provide information for the Get Feature and Set Feature command.
>> One of the main piece of information retrieved are the effects a Set
>> Feature command would have for a particular feature.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
>> Co-developed-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
>> Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
>> ---
>>  drivers/cxl/core/mbox.c      | 179 +++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxlmem.h         |  44 +++++++++
>>  drivers/cxl/pci.c            |   4 +
>>  include/cxl/mailbox.h        |   4 +
>>  include/uapi/linux/cxl_mem.h |   1 +
>>  5 files changed, 232 insertions(+)
>
>Hi Shiju,
>
>So I commented yesterday on this patch that is also duplicated in Dave's series
>have a merge order ordering plan to propose.

Hi Dan,

Thanks for the suggestions.
I tested your suggestions for CXL features commands in the fwctl series, in the EDAC CXL features setup,
as replied. 
>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index
>> 880ac1dba3cc..c5d4c7df2f99 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -67,6 +67,7 @@ static struct cxl_mem_command
>cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
>>  	CXL_CMD(SET_SHUTDOWN_STATE, 0x1, 0, 0),
>>  	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
>>  	CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0),
>> +	CXL_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD,
>0),
>
>As I mention on the CXL FWCTL alias of this path, for kernel-internal only usage
>by definition that means a CXL command id does not need to be defined.
I tried removing  these definitions for  get_supported_features, get_feature and set_feature
from cxl_mem_command[] and build and worked fine for the EDAC CXL features.

For cxl_get_supported_features() to work , I removed following check.
===========================
int cxl_get_supported_features(struct cxl_dev_state *cxlds) {
	int remain_feats, max_size, max_feats, start, rc;
[...]

	/* Get supported features is optional, need to check */
	cmd = cxl_mem_find_command(CXL_MBOX_OP_GET_SUPPORTED_FEATURES);
	if (!cmd)
		return -EOPNOTSUPP;
	if (!test_bit(cmd->info.id, cxl_mbox->enabled_cmds))
		return -EOPNOTSUPP;
[...]
}
========================== 
>
>Setting that aside, the place where CXL EDAC and CXL FWCTL series can unify is
>on the definition of the cxl_{get,set}_features() helpers proposed in this series
>for kernel-internal submission of CXL FEATURES commands. I think Dave's series
>should ingest cxl_{get,set}_features(), go in first since it does not have cross-
>subsystem entanglements, and then you can build reuse that infrastructure to
>finalize the CXL scrub implementation.
Agree. I will reuse merged features infrastructure for the EDAC CXL features.

>
>The missing piece in my mind to make cxl_{get,set}_features() usable with the
>CXL FWCTL path is likely to make it be able to support copying in / out of __user
>buffers. To me that looks like updating
>cxl_internal_send_cmd() to use copy_{to,from}_sockptr() internally so that it is
>independent of the kernel vs user buffer concern from CXL EDAC vs CXL FWCTL
>callers.
Ok.

Thanks,
Shiju