[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e72011454204462eb8ccf10eef56106c@huawei.com>
Date: Mon, 9 Dec 2024 14:28:16 +0000
From: Shiju Jose <shiju.jose@...wei.com>
To: Dan Williams <dan.j.williams@...el.com>, "linux-edac@...r.kernel.org"
<linux-edac@...r.kernel.org>, "linux-cxl@...r.kernel.org"
<linux-cxl@...r.kernel.org>, "linux-acpi@...r.kernel.org"
<linux-acpi@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "bp@...en8.de" <bp@...en8.de>, "tony.luck@...el.com"
<tony.luck@...el.com>, "rafael@...nel.org" <rafael@...nel.org>,
"lenb@...nel.org" <lenb@...nel.org>, "mchehab@...nel.org"
<mchehab@...nel.org>, "dave@...olabs.net" <dave@...olabs.net>, "Jonathan
Cameron" <jonathan.cameron@...wei.com>, "dave.jiang@...el.com"
<dave.jiang@...el.com>, "alison.schofield@...el.com"
<alison.schofield@...el.com>, "vishal.l.verma@...el.com"
<vishal.l.verma@...el.com>, "ira.weiny@...el.com" <ira.weiny@...el.com>,
"david@...hat.com" <david@...hat.com>, "Vilas.Sridharan@....com"
<Vilas.Sridharan@....com>, "leo.duran@....com" <leo.duran@....com>,
"Yazen.Ghannam@....com" <Yazen.Ghannam@....com>, "rientjes@...gle.com"
<rientjes@...gle.com>, "jiaqiyan@...gle.com" <jiaqiyan@...gle.com>,
"Jon.Grimm@....com" <Jon.Grimm@....com>, "dave.hansen@...ux.intel.com"
<dave.hansen@...ux.intel.com>, "naoya.horiguchi@....com"
<naoya.horiguchi@....com>, "james.morse@....com" <james.morse@....com>,
"jthoughton@...gle.com" <jthoughton@...gle.com>, "somasundaram.a@....com"
<somasundaram.a@....com>, "erdemaktas@...gle.com" <erdemaktas@...gle.com>,
"pgonda@...gle.com" <pgonda@...gle.com>, "duenwen@...gle.com"
<duenwen@...gle.com>, "gthelen@...gle.com" <gthelen@...gle.com>,
"wschwartz@...erecomputing.com" <wschwartz@...erecomputing.com>,
"dferguson@...erecomputing.com" <dferguson@...erecomputing.com>,
"wbs@...amperecomputing.com" <wbs@...amperecomputing.com>,
"nifan.cxl@...il.com" <nifan.cxl@...il.com>, tanxiaofei
<tanxiaofei@...wei.com>, "Zengtao (B)" <prime.zeng@...ilicon.com>, "Roberto
Sassu" <roberto.sassu@...wei.com>, "kangkang.shen@...urewei.com"
<kangkang.shen@...urewei.com>, wanghuiqiang <wanghuiqiang@...wei.com>,
Linuxarm <linuxarm@...wei.com>
Subject: RE: [PATCH v17 05/18] cxl: Add Get Supported Features command for
kernel usage
>-----Original Message-----
>From: Dan Williams <dan.j.williams@...el.com>
>Sent: 06 December 2024 21:41
>To: Shiju Jose <shiju.jose@...wei.com>; linux-edac@...r.kernel.org; linux-
>cxl@...r.kernel.org; linux-acpi@...r.kernel.org; linux-mm@...ck.org; linux-
>kernel@...r.kernel.org
>Cc: bp@...en8.de; tony.luck@...el.com; rafael@...nel.org; lenb@...nel.org;
>mchehab@...nel.org; dan.j.williams@...el.com; dave@...olabs.net; Jonathan
>Cameron <jonathan.cameron@...wei.com>; dave.jiang@...el.com;
>alison.schofield@...el.com; vishal.l.verma@...el.com; ira.weiny@...el.com;
>david@...hat.com; Vilas.Sridharan@....com; leo.duran@....com;
>Yazen.Ghannam@....com; rientjes@...gle.com; jiaqiyan@...gle.com;
>Jon.Grimm@....com; dave.hansen@...ux.intel.com;
>naoya.horiguchi@....com; james.morse@....com; jthoughton@...gle.com;
>somasundaram.a@....com; erdemaktas@...gle.com; pgonda@...gle.com;
>duenwen@...gle.com; gthelen@...gle.com;
>wschwartz@...erecomputing.com; dferguson@...erecomputing.com;
>wbs@...amperecomputing.com; nifan.cxl@...il.com; tanxiaofei
><tanxiaofei@...wei.com>; Zengtao (B) <prime.zeng@...ilicon.com>; Roberto
>Sassu <roberto.sassu@...wei.com>; kangkang.shen@...urewei.com;
>wanghuiqiang <wanghuiqiang@...wei.com>; Linuxarm
><linuxarm@...wei.com>; Shiju Jose <shiju.jose@...wei.com>
>Subject: Re: [PATCH v17 05/18] cxl: Add Get Supported Features command for
>kernel usage
>
>shiju.jose@ wrote:
>> From: Dave Jiang <dave.jiang@...el.com>
>>
>> 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@...wei.com>
>> Signed-off-by: Dave Jiang <dave.jiang@...el.com>
>> Co-developed-by: Shiju Jose <shiju.jose@...wei.com>
>> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
>> ---
>> 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
Powered by blists - more mailing lists