| Message ID | 20240716150336.2042-6-shiju.jose@huawei.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC: Scrub: Introduce generic EDAC RAS control feature driver + CXL/ACPI-RAS2 drivers | expand |
On Tue, Jul 16, 2024 at 04:03:29PM +0100, shiju.jose@huawei.com wrote: > From: Shiju Jose <shiju.jose@huawei.com> > > Add support for GET_FEATURE mailbox command. > > CXL spec 3.1 section 8.2.9.6 describes optional device specific features. > The settings of a feature can be retrieved using Get Feature command. > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com> > --- Minor comments inline. > drivers/cxl/core/mbox.c | 37 +++++++++++++++++++++++++++++++++++++ > drivers/cxl/cxlmem.h | 27 +++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 9b9b1d26454e..b1eeed508459 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -1351,6 +1351,43 @@ int cxl_get_supported_features(struct cxl_memdev_state *mds, > } > EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL); > > +size_t cxl_get_feature(struct cxl_memdev_state *mds, > + const uuid_t feat_uuid, void *feat_out, > + size_t feat_out_size, > + enum cxl_get_feat_selection selection) feat_uuid and selection are both payload inputs, maybe more natural to put them together before feat_out. > +{ > + size_t data_to_rd_size, size_out; > + struct cxl_mbox_get_feat_in pi; > + struct cxl_mbox_cmd mbox_cmd; > + size_t data_rcvd_size = 0; > + int rc; > + > + size_out = min(feat_out_size, mds->payload_size); > + pi.uuid = feat_uuid; > + pi.selection = selection; > + do { > + data_to_rd_size = min(feat_out_size - data_rcvd_size, mds->payload_size); > + pi.offset = cpu_to_le16(data_rcvd_size); > + pi.count = cpu_to_le16(data_to_rd_size); > + > + mbox_cmd = (struct cxl_mbox_cmd) { > + .opcode = CXL_MBOX_OP_GET_FEATURE, > + .size_in = sizeof(pi), > + .payload_in = &pi, > + .size_out = size_out, > + .payload_out = feat_out + data_rcvd_size, > + .min_out = data_to_rd_size, > + }; > + rc = cxl_internal_send_cmd(mds, &mbox_cmd); > + if (rc < 0 || mbox_cmd.size_out == 0) Is there other case when size_out will be 0 other than the feat_out_size is 0? If feat_out_size is 0, maybe we return directly, or we use while () {}, instead of do {} while. Anyway, if there is no other case that will return size_out as 0, we can avoid the check. Fan > + return 0; > + data_rcvd_size += mbox_cmd.size_out; > + } while (data_rcvd_size < feat_out_size); > + > + return data_rcvd_size; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL); > + > int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, > struct cxl_region *cxlr) > { > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index b0e1565b9d2e..25698a6fbe66 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -531,6 +531,7 @@ enum cxl_opcode { > 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_IDENTIFY = 0x4000, > CXL_MBOX_OP_GET_PARTITION_INFO = 0x4100, > CXL_MBOX_OP_SET_PARTITION_INFO = 0x4101, > @@ -757,6 +758,28 @@ struct cxl_mbox_get_supp_feats_out { > struct cxl_mbox_supp_feat_entry feat_entries[]; > } __packed; > > +/* > + * Get Feature CXL 3.1 Spec 8.2.9.6.2 > + */ > + > +/* > + * Get Feature input payload > + * CXL rev 3.1 section 8.2.9.6.2 Table 8-99 > + */ > +enum cxl_get_feat_selection { > + CXL_GET_FEAT_SEL_CURRENT_VALUE, > + CXL_GET_FEAT_SEL_DEFAULT_VALUE, > + CXL_GET_FEAT_SEL_SAVED_VALUE, > + CXL_GET_FEAT_SEL_MAX > +}; > + > +struct cxl_mbox_get_feat_in { > + uuid_t uuid; > + __le16 offset; > + __le16 count; > + u8 selection; > +} __packed; > + > /* Get Poison List CXL 3.0 Spec 8.2.9.8.4.1 */ > struct cxl_mbox_poison_in { > __le64 offset; > @@ -891,6 +914,10 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds); > int cxl_get_supported_features(struct cxl_memdev_state *mds, > u32 count, u16 start_index, > struct cxl_mbox_get_supp_feats_out *feats_out); > +size_t cxl_get_feature(struct cxl_memdev_state *mds, > + const uuid_t feat_uuid, void *feat_out, > + size_t feat_out_size, > + enum cxl_get_feat_selection selection); > int cxl_poison_state_init(struct cxl_memdev_state *mds); > int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, > struct cxl_region *cxlr); > -- > 2.34.1 >
>-----Original Message----- >From: nifan.cxl@gmail.com <nifan.cxl@gmail.com> >Sent: 17 July 2024 19:08 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: linux-edac@vger.kernel.org; linux-cxl@vger.kernel.org; linux- >acpi@vger.kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org; >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>; dave.jiang@intel.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; mike.malvestuto@intel.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: [RFC PATCH v9 05/11] cxl/mbox: Add GET_FEATURE mailbox >command > >On Tue, Jul 16, 2024 at 04:03:29PM +0100, shiju.jose@huawei.com wrote: >> From: Shiju Jose <shiju.jose@huawei.com> >> >> Add support for GET_FEATURE mailbox command. >> >> CXL spec 3.1 section 8.2.9.6 describes optional device specific features. >> The settings of a feature can be retrieved using Get Feature command. >> >> Signed-off-by: Shiju Jose <shiju.jose@huawei.com> >> --- >Minor comments inline. > >> drivers/cxl/core/mbox.c | 37 +++++++++++++++++++++++++++++++++++++ >> drivers/cxl/cxlmem.h | 27 +++++++++++++++++++++++++++ >> 2 files changed, 64 insertions(+) >> >> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index >> 9b9b1d26454e..b1eeed508459 100644 >> --- a/drivers/cxl/core/mbox.c >> +++ b/drivers/cxl/core/mbox.c >> @@ -1351,6 +1351,43 @@ int cxl_get_supported_features(struct >> cxl_memdev_state *mds, } >> EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL); >> >> +size_t cxl_get_feature(struct cxl_memdev_state *mds, >> + const uuid_t feat_uuid, void *feat_out, >> + size_t feat_out_size, >> + enum cxl_get_feat_selection selection) >feat_uuid and selection are both payload inputs, maybe more natural to put >them together before feat_out. Will do. >> +{ >> + size_t data_to_rd_size, size_out; >> + struct cxl_mbox_get_feat_in pi; >> + struct cxl_mbox_cmd mbox_cmd; >> + size_t data_rcvd_size = 0; >> + int rc; >> + >> + size_out = min(feat_out_size, mds->payload_size); >> + pi.uuid = feat_uuid; >> + pi.selection = selection; >> + do { >> + data_to_rd_size = min(feat_out_size - data_rcvd_size, mds- >>payload_size); >> + pi.offset = cpu_to_le16(data_rcvd_size); >> + pi.count = cpu_to_le16(data_to_rd_size); >> + >> + mbox_cmd = (struct cxl_mbox_cmd) { >> + .opcode = CXL_MBOX_OP_GET_FEATURE, >> + .size_in = sizeof(pi), >> + .payload_in = &pi, >> + .size_out = size_out, >> + .payload_out = feat_out + data_rcvd_size, >> + .min_out = data_to_rd_size, >> + }; >> + rc = cxl_internal_send_cmd(mds, &mbox_cmd); >> + if (rc < 0 || mbox_cmd.size_out == 0) >Is there other case when size_out will be 0 other than the feat_out_size is 0? I think size_out can be 0 depending on the implementation on the firmware or some error situation when the feat_out_size is non zero. > >If feat_out_size is 0, maybe we return directly, or we use while () {}, instead of >do {} while. I had a check for feat_out_size against min feat out size in the previous version. Will add return directly if feat_out_size is 0. >Anyway, if there is no other case that will return size_out as 0, we can avoid the >check. > >Fan >> + return 0; >> + data_rcvd_size += mbox_cmd.size_out; >> + } while (data_rcvd_size < feat_out_size); >> + >> + return data_rcvd_size; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL); >> + >> int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, >> struct cxl_region *cxlr) >> { >> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index >> b0e1565b9d2e..25698a6fbe66 100644 >> --- a/drivers/cxl/cxlmem.h >> +++ b/drivers/cxl/cxlmem.h >> @@ -531,6 +531,7 @@ enum cxl_opcode { >> 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_IDENTIFY = 0x4000, >> CXL_MBOX_OP_GET_PARTITION_INFO = 0x4100, >> CXL_MBOX_OP_SET_PARTITION_INFO = 0x4101, >> @@ -757,6 +758,28 @@ struct cxl_mbox_get_supp_feats_out { >> struct cxl_mbox_supp_feat_entry feat_entries[]; } __packed; >> >> +/* >> + * Get Feature CXL 3.1 Spec 8.2.9.6.2 */ >> + >> +/* >> + * Get Feature input payload >> + * CXL rev 3.1 section 8.2.9.6.2 Table 8-99 */ enum >> +cxl_get_feat_selection { >> + CXL_GET_FEAT_SEL_CURRENT_VALUE, >> + CXL_GET_FEAT_SEL_DEFAULT_VALUE, >> + CXL_GET_FEAT_SEL_SAVED_VALUE, >> + CXL_GET_FEAT_SEL_MAX >> +}; >> + >> +struct cxl_mbox_get_feat_in { >> + uuid_t uuid; >> + __le16 offset; >> + __le16 count; >> + u8 selection; >> +} __packed; >> + >> /* Get Poison List CXL 3.0 Spec 8.2.9.8.4.1 */ struct >> cxl_mbox_poison_in { >> __le64 offset; >> @@ -891,6 +914,10 @@ int cxl_set_timestamp(struct cxl_memdev_state >> *mds); int cxl_get_supported_features(struct cxl_memdev_state *mds, >> u32 count, u16 start_index, >> struct cxl_mbox_get_supp_feats_out *feats_out); >> +size_t cxl_get_feature(struct cxl_memdev_state *mds, >> + const uuid_t feat_uuid, void *feat_out, >> + size_t feat_out_size, >> + enum cxl_get_feat_selection selection); >> int cxl_poison_state_init(struct cxl_memdev_state *mds); int >> cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, >> struct cxl_region *cxlr); >> -- >> 2.34.1 >> Thanks, Shiju
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 9b9b1d26454e..b1eeed508459 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -1351,6 +1351,43 @@ int cxl_get_supported_features(struct cxl_memdev_state *mds, } EXPORT_SYMBOL_NS_GPL(cxl_get_supported_features, CXL); +size_t cxl_get_feature(struct cxl_memdev_state *mds, + const uuid_t feat_uuid, void *feat_out, + size_t feat_out_size, + enum cxl_get_feat_selection selection) +{ + size_t data_to_rd_size, size_out; + struct cxl_mbox_get_feat_in pi; + struct cxl_mbox_cmd mbox_cmd; + size_t data_rcvd_size = 0; + int rc; + + size_out = min(feat_out_size, mds->payload_size); + pi.uuid = feat_uuid; + pi.selection = selection; + do { + data_to_rd_size = min(feat_out_size - data_rcvd_size, mds->payload_size); + pi.offset = cpu_to_le16(data_rcvd_size); + pi.count = cpu_to_le16(data_to_rd_size); + + mbox_cmd = (struct cxl_mbox_cmd) { + .opcode = CXL_MBOX_OP_GET_FEATURE, + .size_in = sizeof(pi), + .payload_in = &pi, + .size_out = size_out, + .payload_out = feat_out + data_rcvd_size, + .min_out = data_to_rd_size, + }; + rc = cxl_internal_send_cmd(mds, &mbox_cmd); + if (rc < 0 || mbox_cmd.size_out == 0) + return 0; + data_rcvd_size += mbox_cmd.size_out; + } while (data_rcvd_size < feat_out_size); + + return data_rcvd_size; +} +EXPORT_SYMBOL_NS_GPL(cxl_get_feature, CXL); + int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, struct cxl_region *cxlr) { diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index b0e1565b9d2e..25698a6fbe66 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -531,6 +531,7 @@ enum cxl_opcode { 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_IDENTIFY = 0x4000, CXL_MBOX_OP_GET_PARTITION_INFO = 0x4100, CXL_MBOX_OP_SET_PARTITION_INFO = 0x4101, @@ -757,6 +758,28 @@ struct cxl_mbox_get_supp_feats_out { struct cxl_mbox_supp_feat_entry feat_entries[]; } __packed; +/* + * Get Feature CXL 3.1 Spec 8.2.9.6.2 + */ + +/* + * Get Feature input payload + * CXL rev 3.1 section 8.2.9.6.2 Table 8-99 + */ +enum cxl_get_feat_selection { + CXL_GET_FEAT_SEL_CURRENT_VALUE, + CXL_GET_FEAT_SEL_DEFAULT_VALUE, + CXL_GET_FEAT_SEL_SAVED_VALUE, + CXL_GET_FEAT_SEL_MAX +}; + +struct cxl_mbox_get_feat_in { + uuid_t uuid; + __le16 offset; + __le16 count; + u8 selection; +} __packed; + /* Get Poison List CXL 3.0 Spec 8.2.9.8.4.1 */ struct cxl_mbox_poison_in { __le64 offset; @@ -891,6 +914,10 @@ int cxl_set_timestamp(struct cxl_memdev_state *mds); int cxl_get_supported_features(struct cxl_memdev_state *mds, u32 count, u16 start_index, struct cxl_mbox_get_supp_feats_out *feats_out); +size_t cxl_get_feature(struct cxl_memdev_state *mds, + const uuid_t feat_uuid, void *feat_out, + size_t feat_out_size, + enum cxl_get_feat_selection selection); int cxl_poison_state_init(struct cxl_memdev_state *mds); int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len, struct cxl_region *cxlr);