RE: [PATCH v2 1/8] cxl: Add helper function to retrieve a feature entry

[Dan Williams]

Shiju Jose wrote:
> >-----Original Message-----
> >From: Dan Williams <dan.j.williams@xxxxxxxxx>
> >Sent: 26 March 2025 21:33
> >To: Shiju Jose <shiju.jose@xxxxxxxxxx>; linux-cxl@xxxxxxxxxxxxxxx;
> >dan.j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx; Jonathan Cameron
> ><jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx;
> >alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx;
> >david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx
> >Cc: linux-edac@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-
> >mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; bp@xxxxxxxxx;
> >tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
> >mchehab@xxxxxxxxxx; leo.duran@xxxxxxx; Yazen.Ghannam@xxxxxxx;
> >rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx; Jon.Grimm@xxxxxxx;
> >dave.hansen@xxxxxxxxxxxxxxx; naoya.horiguchi@xxxxxxx;
> >james.morse@xxxxxxx; jthoughton@xxxxxxxxxx; somasundaram.a@xxxxxxx;
> >erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx; duenwen@xxxxxxxxxx;
> >gthelen@xxxxxxxxxx; wschwartz@xxxxxxxxxxxxxxxxxxx;
> >dferguson@xxxxxxxxxxxxxxxxxxx; wbs@xxxxxxxxxxxxxxxxxxxxxx;
> >nifan.cxl@xxxxxxxxx; tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B)
> ><prime.zeng@xxxxxxxxxxxxx>; Roberto Sassu <roberto.sassu@xxxxxxxxxx>;
> >kangkang.shen@xxxxxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>;
> >Linuxarm <linuxarm@xxxxxxxxxx>; Shiju Jose <shiju.jose@xxxxxxxxxx>
> >Subject: Re: [PATCH v2 1/8] cxl: Add helper function to retrieve a feature entry
> >
> >shiju.jose@ wrote:
> >> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
> >>
> >> Add helper function to retrieve a feature entry from the supported
> >> features list, if supported.
> >>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >> Reviewed-by: Fan Ni <fan.ni@xxxxxxxxxxx>
> >> Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx>
> >> ---
> >>  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.