| Message ID | 20250227223816.2036-2-shiju.jose@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: support CXL memory RAS features | expand |
On Thu, 27 Feb 2025 22:38:08 +0000 <shiju.jose@huawei.com> wrote: > From: Shiju Jose <shiju.jose@huawei.com> > > Add helper function to retrieve a feature entry from the supported > features list, if supported. > I think this came from within the core cxl features series but got pulled out due to lack of users until this set? > Signed-off-by: Shiju Jose <shiju.jose@huawei.com> Either way it's a trivial and useful helper. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/core.h | 2 ++ > drivers/cxl/core/features.c | 20 ++++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 3d3b00835446..6c83f6f18122 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -120,6 +120,8 @@ int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port, > int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port); > > #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..c822fb4a8c33 100644 > --- a/drivers/cxl/core/features.c > +++ b/drivers/cxl/core/features.c > @@ -203,6 +203,26 @@ 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; > + > + /* > + * 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 ERR_PTR(-ENOENT); > +} > + > 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,
On Thu, Feb 27, 2025 at 10:38:08PM +0000, shiju.jose@huawei.com wrote: > From: Shiju Jose <shiju.jose@huawei.com> > > Add helper function to retrieve a feature entry from the supported > features list, if supported. > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com> Reviewed-by: Fan Ni <fan.ni@samsung.com> > --- > drivers/cxl/core/core.h | 2 ++ > drivers/cxl/core/features.c | 20 ++++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 3d3b00835446..6c83f6f18122 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -120,6 +120,8 @@ int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port, > int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port); > > #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..c822fb4a8c33 100644 > --- a/drivers/cxl/core/features.c > +++ b/drivers/cxl/core/features.c > @@ -203,6 +203,26 @@ 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; > + > + /* > + * 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 ERR_PTR(-ENOENT); > +} > + > 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, > -- > 2.43.0 >
On Thu, Feb 27, 2025 at 10:38:08PM +0000, shiju.jose@huawei.com wrote: > From: Shiju Jose <shiju.jose@huawei.com> > > Add helper function to retrieve a feature entry from the supported > features list, if supported. > > Signed-off-by: Shiju Jose <shiju.jose@huawei.com> > --- > drivers/cxl/core/core.h | 2 ++ > drivers/cxl/core/features.c | 20 ++++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 3d3b00835446..6c83f6f18122 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -120,6 +120,8 @@ int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port, > int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port); > > #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..c822fb4a8c33 100644 > --- a/drivers/cxl/core/features.c > +++ b/drivers/cxl/core/features.c > @@ -203,6 +203,26 @@ 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; > + > + /* > + * Retrieve the feature entry from the supported features list, > + * if the feature is supported. > + */ > + feat_entry = cxlfs->entries->ent; Do we need some NULL checking here on cxlfs, entries > + for (count = 0; count < cxlfs->entries->num_features; count++, feat_entry++) { Was num_features previously validated? > + if (uuid_equal(&feat_entry->uuid, feat_uuid)) > + return feat_entry; > + } > + > + return ERR_PTR(-ENOENT); Why not just 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, > -- > 2.43.0 >
>-----Original Message----- >From: Alison Schofield <alison.schofield@intel.com> >Sent: 07 March 2025 19:20 >To: Shiju Jose <shiju.jose@huawei.com> [...] >> +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; >> + >> + /* >> + * Retrieve the feature entry from the supported features list, >> + * if the feature is supported. >> + */ >> + feat_entry = cxlfs->entries->ent; > >Do we need some NULL checking here on cxlfs, entries Hi Alison, Thanks for the feedbacks. We had check on cxlfs before https://lore.kernel.org/all/20250122235159.2716036-5-dave.jiang@intel.com/ but removed because of the following comment. https://lore.kernel.org/all/20250124150150.GZ5556@nvidia.com/ > > >> + for (count = 0; count < cxlfs->entries->num_features; count++, >> +feat_entry++) { > >Was num_features previously validated? Not in the caller. Had check for num_features here before in cxl_get_feature_entry() as seen in the above link. > >> + if (uuid_equal(&feat_entry->uuid, feat_uuid)) >> + return feat_entry; >> + } >> + >> + return ERR_PTR(-ENOENT); > >Why not just return NULL? Will do. > > >> +} >> + >> 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, >> -- >> 2.43.0 >> Thanks, Shiju
On Mon, Mar 10, 2025 at 06:15:38PM +0000, Shiju Jose wrote: > >-----Original Message----- > >From: Alison Schofield <alison.schofield@intel.com> > >Sent: 07 March 2025 19:20 > >To: Shiju Jose <shiju.jose@huawei.com> > [...] > >> +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; > >> + > >> + /* > >> + * Retrieve the feature entry from the supported features list, > >> + * if the feature is supported. > >> + */ > >> + feat_entry = cxlfs->entries->ent; > > > >Do we need some NULL checking here on cxlfs, entries > > Hi Alison, > > Thanks for the feedbacks. > We had check on cxlfs before > https://lore.kernel.org/all/20250122235159.2716036-5-dave.jiang@intel.com/ > but removed because of the following comment. > https://lore.kernel.org/all/20250124150150.GZ5556@nvidia.com/ Hi Shiju, I have not followed all along, so yeah my questions may be a bit pesky at this point. I did see the comment linked above about how the driver must be bound at this point. I think my question is a bit different. Are each of these guaranteed not to be NULL here: to_cxlfs(cxlds) cxlfs->entries cxlfs->entries->ent If these cannot be NULL, then all good. --Alison > > > > > >> + for (count = 0; count < cxlfs->entries->num_features; count++, > >> +feat_entry++) { > > > >Was num_features previously validated? > Not in the caller. Had check for num_features here before in cxl_get_feature_entry() > as seen in the above link. > > > >> + if (uuid_equal(&feat_entry->uuid, feat_uuid)) > >> + return feat_entry; > >> + } > >> + > >> + return ERR_PTR(-ENOENT); > > > >Why not just return NULL? > Will do. > > > > > >> +} > >> + > >> 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, > >> -- > >> 2.43.0 > >> > > Thanks, > Shiju >
>-----Original Message----- >From: Alison Schofield <alison.schofield@intel.com> >Sent: 10 March 2025 20:29 >To: Shiju Jose <shiju.jose@huawei.com> >Cc: linux-cxl@vger.kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; >Jonathan Cameron <jonathan.cameron@huawei.com>; dave.jiang@intel.com; >vishal.l.verma@intel.com; ira.weiny@intel.com; david@redhat.com; >Vilas.Sridharan@amd.com; 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> >Subject: Re: [PATCH 1/8] cxl: Add helper function to retrieve a feature entry > >On Mon, Mar 10, 2025 at 06:15:38PM +0000, Shiju Jose wrote: >> >-----Original Message----- >> >From: Alison Schofield <alison.schofield@intel.com> >> >Sent: 07 March 2025 19:20 >> >To: Shiju Jose <shiju.jose@huawei.com> >> [...] >> >> +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; >> >> + >> >> + /* >> >> + * Retrieve the feature entry from the supported features list, >> >> + * if the feature is supported. >> >> + */ >> >> + feat_entry = cxlfs->entries->ent; >> > >> >Do we need some NULL checking here on cxlfs, entries >> >> Hi Alison, >> >> Thanks for the feedbacks. >> We had check on cxlfs before >> https://lore.kernel.org/all/20250122235159.2716036-5-dave.jiang@intel. >> com/ but removed because of the following comment. >> https://lore.kernel.org/all/20250124150150.GZ5556@nvidia.com/ > >Hi Shiju, Hi Alison, > >I have not followed all along, so yeah my questions may be a bit pesky at this >point. I did see the comment linked above about how the driver must be bound >at this point. I think my question is a bit different. The feedback was added to remove defensive checks present in this function. > >Are each of these guaranteed not to be NULL here: > >to_cxlfs(cxlds) to_cxlfs(cxlds) cannot be NULL if cxl_get_feature_entry() is called after devm_cxl_setup_features(), otherwise will be NULL. >cxlfs->entries >cxlfs->entries->ent Both fields will be NULL if the firmware does not support any features. > >If these cannot be NULL, then all good. > >--Alison > [...] >> Thanks, Shiju
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index 3d3b00835446..6c83f6f18122 100644 --- a/drivers/cxl/core/core.h +++ b/drivers/cxl/core/core.h @@ -120,6 +120,8 @@ int cxl_port_get_switch_dport_bandwidth(struct cxl_port *port, int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port); #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..c822fb4a8c33 100644 --- a/drivers/cxl/core/features.c +++ b/drivers/cxl/core/features.c @@ -203,6 +203,26 @@ 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; + + /* + * 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 ERR_PTR(-ENOENT); +} + 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,