| Message ID | 20250122235159.2716036-4-dave.jiang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Add CXL feature commands support via fwctl | expand |
On Wed, 22 Jan 2025 16:50:34 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Add feature commands enumeration code in order to detect and enumerate > the 3 feature related commands "get supported features", "get feature", > and "set feature". The enumeration will help determine whether the driver > can issue any of the 3 commands to the device. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/mbox.c | 41 +++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxlmem.h | 3 +++ > include/cxl/features.h | 7 +++++++ > include/cxl/mailbox.h | 1 + > 4 files changed, 52 insertions(+) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index bdb8f060f2c1..5e21ff99d70f 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -38,6 +38,21 @@ static bool cxl_raw_allow_all; > .flags = _flags, \ > } > > +#define cxl_for_each_feature_cmd(cmd) \ > + for ((cmd) = &cxl_feature_commands[0]; \ > + ((cmd) - cxl_feature_commands) < ARRAY_SIZE(cxl_feature_commands); (cmd)++) > + > +#define CXL_FEATURE_CMD(_id, sin, sout, _flags) \ > + [CXL_FEATURE_ID_##_id] = { \ > + .info = { \ > + .id = CXL_FEATURE_ID_##_id, \ > + .size_in = sin, \ > + .size_out = sout, \ > + }, \ > + .opcode = CXL_MBOX_OP_##_id, \ > + .flags = _flags, \ > + } > + > #define CXL_VARIABLE_PAYLOAD ~0U > /* > * This table defines the supported mailbox commands for the driver. This table > @@ -69,6 +84,13 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = { > CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0), > }; > > +#define CXL_FEATURE_COMMAND_ID_MAX 3 Why do we need this define separately from CXL_FEATURE_ID_MAX as I'm fairly sure they must always be the same. A [] array would work here too. > +static struct cxl_mem_command cxl_feature_commands[CXL_FEATURE_COMMAND_ID_MAX] = { > + CXL_FEATURE_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD, 0), > + CXL_FEATURE_CMD(GET_FEATURE, 0xf, CXL_VARIABLE_PAYLOAD, 0), > + CXL_FEATURE_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0), > +}; > diff --git a/include/cxl/features.h b/include/cxl/features.h > index b92da1e92780..7a8be3c621a1 100644 > --- a/include/cxl/features.h > +++ b/include/cxl/features.h > @@ -5,6 +5,13 @@ > > struct cxl_mailbox; > > +enum feature_cmds { > + CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0, > + CXL_FEATURE_ID_GET_FEATURE, > + CXL_FEATURE_ID_SET_FEATURE, > + CXL_FEATURE_ID_MAX, No comma on that one as it's meant to always be the terminator. > +};
Dave Jiang wrote: > Add feature commands enumeration code in order to detect and enumerate > the 3 feature related commands "get supported features", "get feature", > and "set feature". The enumeration will help determine whether the driver > can issue any of the 3 commands to the device. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/mbox.c | 41 +++++++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxlmem.h | 3 +++ > include/cxl/features.h | 7 +++++++ > include/cxl/mailbox.h | 1 + > 4 files changed, 52 insertions(+) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index bdb8f060f2c1..5e21ff99d70f 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -38,6 +38,21 @@ static bool cxl_raw_allow_all; > .flags = _flags, \ > } > > +#define cxl_for_each_feature_cmd(cmd) \ > + for ((cmd) = &cxl_feature_commands[0]; \ > + ((cmd) - cxl_feature_commands) < ARRAY_SIZE(cxl_feature_commands); (cmd)++) > + > +#define CXL_FEATURE_CMD(_id, sin, sout, _flags) \ > + [CXL_FEATURE_ID_##_id] = { \ > + .info = { \ > + .id = CXL_FEATURE_ID_##_id, \ > + .size_in = sin, \ > + .size_out = sout, \ > + }, \ > + .opcode = CXL_MBOX_OP_##_id, \ > + .flags = _flags, \ > + } > + > #define CXL_VARIABLE_PAYLOAD ~0U > /* > * This table defines the supported mailbox commands for the driver. This table > @@ -69,6 +84,13 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = { > CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0), > }; > > +#define CXL_FEATURE_COMMAND_ID_MAX 3 > +static struct cxl_mem_command cxl_feature_commands[CXL_FEATURE_COMMAND_ID_MAX] = { > + CXL_FEATURE_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD, 0), > + CXL_FEATURE_CMD(GET_FEATURE, 0xf, CXL_VARIABLE_PAYLOAD, 0), > + CXL_FEATURE_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0), > +}; It's not immediately clear to me what this is far. The cxl_mem_commands[] array is to put all the common work of defining new commands for the CXL ioctl path into a table. Given these are all fronted by FWCTL, does the driver need to do anything more than the opcodes are supported. In other words, I worry this approach is simply copying the organization of cxl_walk_cel() for consistency sake. I had similar feedback on the DCD series: http://lore.kernel.org/67786b76a0c5_f58f294b1@dwillia2-xfh.jf.intel.com.notmuch Just check for the opcodes you need and set a flag.
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index bdb8f060f2c1..5e21ff99d70f 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -38,6 +38,21 @@ static bool cxl_raw_allow_all; .flags = _flags, \ } +#define cxl_for_each_feature_cmd(cmd) \ + for ((cmd) = &cxl_feature_commands[0]; \ + ((cmd) - cxl_feature_commands) < ARRAY_SIZE(cxl_feature_commands); (cmd)++) + +#define CXL_FEATURE_CMD(_id, sin, sout, _flags) \ + [CXL_FEATURE_ID_##_id] = { \ + .info = { \ + .id = CXL_FEATURE_ID_##_id, \ + .size_in = sin, \ + .size_out = sout, \ + }, \ + .opcode = CXL_MBOX_OP_##_id, \ + .flags = _flags, \ + } + #define CXL_VARIABLE_PAYLOAD ~0U /* * This table defines the supported mailbox commands for the driver. This table @@ -69,6 +84,13 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = { CXL_CMD(GET_TIMESTAMP, 0, 0x8, 0), }; +#define CXL_FEATURE_COMMAND_ID_MAX 3 +static struct cxl_mem_command cxl_feature_commands[CXL_FEATURE_COMMAND_ID_MAX] = { + CXL_FEATURE_CMD(GET_SUPPORTED_FEATURES, 0x8, CXL_VARIABLE_PAYLOAD, 0), + CXL_FEATURE_CMD(GET_FEATURE, 0xf, CXL_VARIABLE_PAYLOAD, 0), + CXL_FEATURE_CMD(SET_FEATURE, CXL_VARIABLE_PAYLOAD, 0, 0), +}; + /* * Commands that RAW doesn't permit. The rationale for each: * @@ -212,6 +234,17 @@ static struct cxl_mem_command *cxl_mem_find_command(u16 opcode) return NULL; } +static struct cxl_mem_command *cxl_find_feature_command(u16 opcode) +{ + struct cxl_mem_command *c; + + cxl_for_each_feature_cmd(c) + if (c->opcode == opcode) + return c; + + return NULL; +} + static const char *cxl_mem_opcode_to_name(u16 opcode) { struct cxl_mem_command *c; @@ -734,6 +767,14 @@ static void cxl_walk_cel(struct cxl_memdev_state *mds, size_t size, u8 *cel) if (cmd) { set_bit(cmd->info.id, cxl_mbox->enabled_cmds); enabled++; + } else { + struct cxl_mem_command *fcmd = + cxl_find_feature_command(opcode); + + if (fcmd) { + set_bit(fcmd->info.id, cxl_mbox->feature_cmds); + enabled++; + } } if (cxl_is_poison_command(opcode)) { diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index a0a49809cd76..55c55685cb39 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -490,6 +490,9 @@ enum cxl_opcode { CXL_MBOX_OP_GET_LOG_CAPS = 0x0402, CXL_MBOX_OP_CLEAR_LOG = 0x0403, CXL_MBOX_OP_GET_SUP_LOG_SUBLIST = 0x0405, + CXL_MBOX_OP_GET_SUPPORTED_FEATURES = 0x0500, + CXL_MBOX_OP_GET_FEATURE = 0x0501, + CXL_MBOX_OP_SET_FEATURE = 0x0502, CXL_MBOX_OP_IDENTIFY = 0x4000, CXL_MBOX_OP_GET_PARTITION_INFO = 0x4100, CXL_MBOX_OP_SET_PARTITION_INFO = 0x4101, diff --git a/include/cxl/features.h b/include/cxl/features.h index b92da1e92780..7a8be3c621a1 100644 --- a/include/cxl/features.h +++ b/include/cxl/features.h @@ -5,6 +5,13 @@ struct cxl_mailbox; +enum feature_cmds { + CXL_FEATURE_ID_GET_SUPPORTED_FEATURES = 0, + CXL_FEATURE_ID_GET_FEATURE, + CXL_FEATURE_ID_SET_FEATURE, + CXL_FEATURE_ID_MAX, +}; + struct cxl_features { int id; struct device dev; diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h index 6caab0d406ba..263fc346aeb1 100644 --- a/include/cxl/mailbox.h +++ b/include/cxl/mailbox.h @@ -58,6 +58,7 @@ struct cxl_mailbox { struct device *host; DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX); DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX); + DECLARE_BITMAP(feature_cmds, CXL_FEATURE_ID_MAX); size_t payload_size; struct mutex mbox_mutex; /* lock to protect mailbox context */ struct rcuwait mbox_wait;
Add feature commands enumeration code in order to detect and enumerate the 3 feature related commands "get supported features", "get feature", and "set feature". The enumeration will help determine whether the driver can issue any of the 3 commands to the device. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/mbox.c | 41 +++++++++++++++++++++++++++++++++++++++++ drivers/cxl/cxlmem.h | 3 +++ include/cxl/features.h | 7 +++++++ include/cxl/mailbox.h | 1 + 4 files changed, 52 insertions(+)