| Message ID | 20250211095349.981096-12-rrichter@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Address translation support, part 1: Cleanups and refactoring | expand |
On Tue, 11 Feb 2025 10:53:41 +0100 Robert Richter <rrichter@amd.com> wrote: > Before adding an endpoint to a region, the endpoint is initialized > first. Move that part to a new function > cxl_endpoint_decoder_initialize(). The function is in preparation of > adding more parameters that need to be determined in a setup. > > The split also helps better separating the code. After initialization > the addition of an endpoint may fail with an error code and all the > data would need to be reverted to not leave the endpoint in an > undefined state. With separate functions the init part can succeed > even if the endpoint cannot be added. > > Function naming follows the style of device_register() etc. Thus, > rename function cxl_add_to_region() to > cxl_endpoint_decoder_register(). Hi Robert, Superficially I'd expect a call of that name to be registering the device for the decoder. i.e. being the thing that makes /sys/bus/cxl/devices/decoder3.2 appear. This register naming is based on the other two being initalize and add, but they aren't initializing and adding the endpoint decode device. Hence I don't think those names work either. > > Signed-off-by: Robert Richter <rrichter@amd.com> > Reviewed-by: Gregory Price <gourry@gourry.net> > Tested-by: Gregory Price <gourry@gourry.net> > --- > drivers/cxl/core/region.c | 36 ++++++++++++++++++++++++++++-------- > drivers/cxl/cxl.h | 6 ++++-- > drivers/cxl/port.c | 9 +++++---- > 3 files changed, 37 insertions(+), 14 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 9ce0282c0042..fb43e154c7b9 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3345,7 +3345,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > dev_name(&cxlr->dev), p->res, p->interleave_ways, > p->interleave_granularity); > > - /* ...to match put_device() in cxl_add_to_region() */ > + /* ...to match put_device() in cxl_endpoint_decoder_add() */ > get_device(&cxlr->dev); > up_write(&cxl_region_rwsem); > > @@ -3357,19 +3357,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > return ERR_PTR(rc); > } > > -int cxl_add_to_region(struct cxl_endpoint_decoder *cxled) > +static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled) So far this looks like it should be called something like cxl_endpoint_decoder_init_region_decoder() or something like that. The cxled is already intialized more generally and the cxled->cxld.dev is registered. > { > - struct range *hpa = &cxled->cxld.hpa_range; > struct cxl_root_decoder *cxlrd; > - struct cxl_region_params *p; > - struct cxl_region *cxlr; > - bool attach = false; > - int rc; > > cxlrd = cxl_find_root_decoder(cxled); > if (!cxlrd) > return -ENXIO; > > + cxled->cxlrd = cxlrd; > + > + return 0; > +} > + > +static int cxl_endpoint_decoder_add(struct cxl_endpoint_decoder *cxled) It's not adding what I'd expect such a function to add. Rather it is performing an association with a region. > +{ > + struct range *hpa = &cxled->cxld.hpa_range; > + struct cxl_root_decoder *cxlrd = cxled->cxlrd; > + struct cxl_region_params *p; > + struct cxl_region *cxlr; > + bool attach = false; > + int rc; > + > /* > * Ensure that if multiple threads race to construct_region() for @hpa > * one does the construction and the others add to that. > @@ -3406,7 +3415,18 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled) > > return rc; > } > -EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL"); > + > +int cxl_endpoint_decoder_register(struct cxl_endpoint_decoder *cxled) > +{ > + int rc; > + > + rc = cxl_endpoint_decoder_initialize(cxled); > + if (rc) > + return rc; > + > + return cxl_endpoint_decoder_add(cxled); > +} > +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_register, "CXL");
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 9ce0282c0042..fb43e154c7b9 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -3345,7 +3345,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, dev_name(&cxlr->dev), p->res, p->interleave_ways, p->interleave_granularity); - /* ...to match put_device() in cxl_add_to_region() */ + /* ...to match put_device() in cxl_endpoint_decoder_add() */ get_device(&cxlr->dev); up_write(&cxl_region_rwsem); @@ -3357,19 +3357,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, return ERR_PTR(rc); } -int cxl_add_to_region(struct cxl_endpoint_decoder *cxled) +static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled) { - struct range *hpa = &cxled->cxld.hpa_range; struct cxl_root_decoder *cxlrd; - struct cxl_region_params *p; - struct cxl_region *cxlr; - bool attach = false; - int rc; cxlrd = cxl_find_root_decoder(cxled); if (!cxlrd) return -ENXIO; + cxled->cxlrd = cxlrd; + + return 0; +} + +static int cxl_endpoint_decoder_add(struct cxl_endpoint_decoder *cxled) +{ + struct range *hpa = &cxled->cxld.hpa_range; + struct cxl_root_decoder *cxlrd = cxled->cxlrd; + struct cxl_region_params *p; + struct cxl_region *cxlr; + bool attach = false; + int rc; + /* * Ensure that if multiple threads race to construct_region() for @hpa * one does the construction and the others add to that. @@ -3406,7 +3415,18 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled) return rc; } -EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL"); + +int cxl_endpoint_decoder_register(struct cxl_endpoint_decoder *cxled) +{ + int rc; + + rc = cxl_endpoint_decoder_initialize(cxled); + if (rc) + return rc; + + return cxl_endpoint_decoder_add(cxled); +} +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_register, "CXL"); static int is_system_ram(struct resource *res, void *arg) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 85dfc8df0a80..50e7d878bb6f 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -392,6 +392,7 @@ enum cxl_decoder_state { */ struct cxl_endpoint_decoder { struct cxl_decoder cxld; + struct cxl_root_decoder *cxlrd; struct resource *dpa_res; resource_size_t skip; enum cxl_decoder_state state; @@ -854,7 +855,7 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_port *port); #ifdef CONFIG_CXL_REGION bool is_cxl_pmem_region(struct device *dev); struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev); -int cxl_add_to_region(struct cxl_endpoint_decoder *cxled); +int cxl_endpoint_decoder_register(struct cxl_endpoint_decoder *cxled); struct cxl_dax_region *to_cxl_dax_region(struct device *dev); #else static inline bool is_cxl_pmem_region(struct device *dev) @@ -865,7 +866,8 @@ static inline struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev) { return NULL; } -static inline int cxl_add_to_region(struct cxl_endpoint_decoder *cxled) +static inline int +cxl_endpoint_decoder_register(struct cxl_endpoint_decoder *cxled) { return 0; } diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index 74587a403e3d..36e487366c7a 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -46,13 +46,14 @@ static int discover_region(struct device *dev, void *unused) return 0; /* - * Region enumeration is opportunistic, if this add-event fails, + * Region enumeration is opportunistic, ignore errors and * continue to the next endpoint decoder. */ - rc = cxl_add_to_region(cxled); + rc = cxl_endpoint_decoder_register(cxled); if (rc) - dev_dbg(dev, "failed to add to region: %#llx-%#llx\n", - cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end); + dev_warn(cxled->cxld.dev.parent, + "failed to register %s: %d\n", + dev_name(&cxled->cxld.dev), rc); return 0; }