| Message ID | 20250320180450.539-2-shiju.jose@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: support CXL memory RAS features | expand |
shiju.jose@ wrote: > From: Shiju Jose <shiju.jose@huawei.com> > > Add helper function to retrieve a feature entry from the supported > features list, if supported. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Reviewed-by: Fan Ni <fan.ni@samsung.com> > Signed-off-by: Shiju Jose <shiju.jose@huawei.com> > --- > drivers/cxl/core/core.h | 2 ++ > drivers/cxl/core/features.c | 23 +++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 1803aedb25ca..16bc717376fc 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -123,6 +123,8 @@ int cxl_ras_init(void); > void cxl_ras_exit(void); > > #ifdef CONFIG_CXL_FEATURES > +struct cxl_feat_entry *cxl_get_feature_entry(struct cxl_dev_state *cxlds, > + const uuid_t *feat_uuid); It is unfortunate that this naming choice is too similar to cxl_get_feature(). However, as I go to suggest a new name I find that this is a duplicate of get_support_feature_info() in Dave's fwctl series. Just drop this patch in favor of that.
>-----Original Message----- >From: Dan Williams <dan.j.williams@intel.com> >Sent: 26 March 2025 21:33 >To: Shiju Jose <shiju.jose@huawei.com>; linux-cxl@vger.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 >Cc: linux-edac@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; 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>; Shiju Jose <shiju.jose@huawei.com> >Subject: Re: [PATCH v2 1/8] cxl: Add helper function to retrieve a feature entry > >shiju.jose@ wrote: >> From: Shiju Jose <shiju.jose@huawei.com> >> >> Add helper function to retrieve a feature entry from the supported >> features list, if supported. >> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >> Reviewed-by: Fan Ni <fan.ni@samsung.com> >> Signed-off-by: Shiju Jose <shiju.jose@huawei.com> >> --- >> drivers/cxl/core/core.h | 2 ++ >> drivers/cxl/core/features.c | 23 +++++++++++++++++++++++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index >> 1803aedb25ca..16bc717376fc 100644 >> --- a/drivers/cxl/core/core.h >> +++ b/drivers/cxl/core/core.h >> @@ -123,6 +123,8 @@ int cxl_ras_init(void); void cxl_ras_exit(void); >> >> #ifdef CONFIG_CXL_FEATURES >> +struct cxl_feat_entry *cxl_get_feature_entry(struct cxl_dev_state *cxlds, >> + const uuid_t *feat_uuid); > >It is unfortunate that this naming choice is too similar to cxl_get_feature(). >However, as I go to suggest a new name I find that this is a duplicate of >get_support_feature_info() in Dave's fwctl series. Just drop this patch in favor of >that. Hi Dan, I am fine to use get_support_feature_info() for the EDAC features. However this function is defined as static in the fwctl series and takes struct fwctl_rpc_cxl * as input for RPC instead of uuid_t * as in cxl_get_feature_entry(). static struct cxl_feat_entry * get_support_feature_info(struct cxl_features_state *cxlfs, const struct fwctl_rpc_cxl *rpc_in) Can you suggest how to use get_support_feature_info() from within the CXL driver to retrieve a supported feature entry (for e.g an EDAC feature)? > Thanks, Shiju
Shiju Jose wrote: > >-----Original Message----- > >From: Dan Williams <dan.j.williams@intel.com> > >Sent: 26 March 2025 21:33 > >To: Shiju Jose <shiju.jose@huawei.com>; linux-cxl@vger.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 > >Cc: linux-edac@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; 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>; Shiju Jose <shiju.jose@huawei.com> > >Subject: Re: [PATCH v2 1/8] cxl: Add helper function to retrieve a feature entry > > > >shiju.jose@ wrote: > >> From: Shiju Jose <shiju.jose@huawei.com> > >> > >> Add helper function to retrieve a feature entry from the supported > >> features list, if supported. > >> > >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >> Reviewed-by: Fan Ni <fan.ni@samsung.com> > >> Signed-off-by: Shiju Jose <shiju.jose@huawei.com> > >> --- > >> drivers/cxl/core/core.h | 2 ++ > >> drivers/cxl/core/features.c | 23 +++++++++++++++++++++++ > >> 2 files changed, 25 insertions(+) > >> > >> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index > >> 1803aedb25ca..16bc717376fc 100644 > >> --- a/drivers/cxl/core/core.h > >> +++ b/drivers/cxl/core/core.h > >> @@ -123,6 +123,8 @@ int cxl_ras_init(void); void cxl_ras_exit(void); > >> > >> #ifdef CONFIG_CXL_FEATURES > >> +struct cxl_feat_entry *cxl_get_feature_entry(struct cxl_dev_state *cxlds, > >> + const uuid_t *feat_uuid); > > > >It is unfortunate that this naming choice is too similar to cxl_get_feature(). > >However, as I go to suggest a new name I find that this is a duplicate of > >get_support_feature_info() in Dave's fwctl series. Just drop this patch in favor of > >that. > > Hi Dan, > > I am fine to use get_support_feature_info() for the EDAC features. > However this function is defined as static in the fwctl series and So, remove static and make it a routine shared between core/features.c and core/edac.c. > takes struct fwctl_rpc_cxl * as input for RPC instead of uuid_t * > as in cxl_get_feature_entry(). > > static struct cxl_feat_entry * > get_support_feature_info(struct cxl_features_state *cxlfs, > const struct fwctl_rpc_cxl *rpc_in) > > Can you suggest how to use get_support_feature_info() from within the CXL driver > to retrieve a supported feature entry (for e.g an EDAC feature)? Simply refactor it into something generic that the cxl-edac and cxl-fwctl path can share: struct cxl_feat_entry * cxl_feature_info(struct cxl_features_state *cxlfs, const uuid_t *uuid) ...with the header in drivers/cxl/core/core.h where other cross-core object helpers are defined.
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index 1803aedb25ca..16bc717376fc 100644 --- a/drivers/cxl/core/core.h +++ b/drivers/cxl/core/core.h @@ -123,6 +123,8 @@ int cxl_ras_init(void); void cxl_ras_exit(void); #ifdef CONFIG_CXL_FEATURES +struct cxl_feat_entry *cxl_get_feature_entry(struct cxl_dev_state *cxlds, + const uuid_t *feat_uuid); size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid, enum cxl_get_feat_selection selection, void *feat_out, size_t feat_out_size, u16 offset, diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c index 048ba4fc3538..202c8c21930c 100644 --- a/drivers/cxl/core/features.c +++ b/drivers/cxl/core/features.c @@ -203,6 +203,29 @@ int devm_cxl_setup_features(struct cxl_dev_state *cxlds) } EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_features, "CXL"); +struct cxl_feat_entry *cxl_get_feature_entry(struct cxl_dev_state *cxlds, + const uuid_t *feat_uuid) +{ + struct cxl_features_state *cxlfs = to_cxlfs(cxlds); + struct cxl_feat_entry *feat_entry; + int count; + + if (!cxlfs || !cxlfs->entries || !cxlfs->entries->num_features) + return NULL; + + /* + * Retrieve the feature entry from the supported features list, + * if the feature is supported. + */ + feat_entry = cxlfs->entries->ent; + for (count = 0; count < cxlfs->entries->num_features; count++, feat_entry++) { + if (uuid_equal(&feat_entry->uuid, feat_uuid)) + return feat_entry; + } + + return NULL; +} + size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid, enum cxl_get_feat_selection selection, void *feat_out, size_t feat_out_size, u16 offset,