| Message ID | 20240907081836.5801-18-alejandro.lucero-palau@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: add Type2 device support | expand |
On Sat, 7 Sep 2024 09:18:33 +0100 alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Creating a CXL region requires userspace intervention through the cxl > sysfs files. Type2 support should allow accelerator drivers to create > such cxl region from kernel code. > > Adding that functionality and integrating it with current support for > memory expanders. > > Based on https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/ > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Sign off doesn't make sense given Dan didn't sent the mail. Co-developed missing? Minor stuff inline, Jonathan > +static struct cxl_region *construct_region_begin(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled) > { > struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); > - struct cxl_port *port = cxlrd_to_port(cxlrd); > - struct range *hpa = &cxled->cxld.hpa_range; > struct cxl_region_params *p; > struct cxl_region *cxlr; > - struct resource *res; > - int rc; > + int err = 0; > > do { > cxlr = __create_region(cxlrd, cxled->mode, > @@ -3404,8 +3414,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY); > > if (IS_ERR(cxlr)) { > - dev_err(cxlmd->dev.parent, > - "%s:%s: %s failed assign region: %ld\n", > + dev_err(cxlmd->dev.parent, "%s:%s: %s failed assign region: %ld\n", > dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), > __func__, PTR_ERR(cxlr)); > return cxlr; > @@ -3415,19 +3424,41 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > p = &cxlr->params; > if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) { > dev_err(cxlmd->dev.parent, > - "%s:%s: %s autodiscovery interrupted\n", > + "%s:%s: %s region setup interrupted\n", > dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), > __func__); > - rc = -EBUSY; > - goto err; > + err = -EBUSY; > } > > + if (err) { > + construct_region_end(); Here I'd just release the semaphore not call this warpper as it's taken within this function I think? > + drop_region(cxlr); > + return ERR_PTR(err); > + } > + return cxlr; > +} > + > *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa), > @@ -3444,6 +3475,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > __func__, dev_name(&cxlr->dev)); > } > > + p = &cxlr->params; > p->res = res; > p->interleave_ways = cxled->cxld.interleave_ways; > p->interleave_granularity = cxled->cxld.interleave_granularity; > @@ -3451,24 +3483,106 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > > rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group()); > if (rc) > - goto err; > + goto out; > > dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n", > - dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__, > - dev_name(&cxlr->dev), p->res, p->interleave_ways, > - p->interleave_granularity); > + dev_name(&cxlmd->dev), > + dev_name(&cxled->cxld.dev), __func__, Avoid reformatting unless necessary. Hard to tell what changed if anything. > + dev_name(&cxlr->dev), p->res, > + p->interleave_ways, > + p->interleave_granularity); > > /* ...to match put_device() in cxl_add_to_region() */ > get_device(&cxlr->dev); > up_write(&cxl_region_rwsem); > > +out: > + construct_region_end(); As below. I'd have separate error handling path. Same in the other paths. > + if (rc) { > + drop_region(cxlr); > + return ERR_PTR(rc); > + } > return cxlr; > +} > > -err: > - up_write(&cxl_region_rwsem); > - devm_release_action(port->uport_dev, unregister_region, cxlr); > - return ERR_PTR(rc); > +static struct cxl_region * > +__construct_new_region(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled) > +{ > + struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld; > + struct cxl_region_params *p; > + resource_size_t size = 0; set in all paths that use it. > + struct cxl_region *cxlr; > + int rc; > + > + cxlr = construct_region_begin(cxlrd, cxled); > + if (IS_ERR(cxlr)) > + return cxlr; > + > + rc = set_interleave_ways(cxlr, 1); > + if (rc) > + goto out; > + > + rc = set_interleave_granularity(cxlr, cxld->interleave_granularity); > + if (rc) > + goto out; > + > + size = resource_size(cxled->dpa_res); > + only used once rc = alloc_hpa(cxlr, resource_size(cxled->dpa_res)); > + rc = alloc_hpa(cxlr, size); > + if (rc) > + goto out; > + > + down_read(&cxl_dpa_rwsem); > + rc = cxl_region_attach(cxlr, cxled, 0); > + up_read(&cxl_dpa_rwsem); > + > + if (rc) > + goto out; > + > + rc = cxl_region_decode_commit(cxlr); > + if (rc) > + goto out; > + > + p = &cxlr->params; > + p->state = CXL_CONFIG_COMMIT; > +out: I'd break out a separate error handling path and just duplicate the next line. > + construct_region_end(); > + if (rc) { > + drop_region(cxlr); > + return ERR_PTR(rc); > + } > + return cxlr; > +}
On 9/13/24 19:08, Jonathan Cameron wrote: > On Sat, 7 Sep 2024 09:18:33 +0100 > alejandro.lucero-palau@amd.com wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> Creating a CXL region requires userspace intervention through the cxl >> sysfs files. Type2 support should allow accelerator drivers to create >> such cxl region from kernel code. >> >> Adding that functionality and integrating it with current support for >> memory expanders. >> >> Based on https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/ >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Sign off doesn't make sense given Dan didn't sent the mail. Co-developed missing? > > > Minor stuff inline, > > Jonathan > >> +static struct cxl_region *construct_region_begin(struct cxl_root_decoder *cxlrd, >> + struct cxl_endpoint_decoder *cxled) >> { >> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); >> - struct cxl_port *port = cxlrd_to_port(cxlrd); >> - struct range *hpa = &cxled->cxld.hpa_range; >> struct cxl_region_params *p; >> struct cxl_region *cxlr; >> - struct resource *res; >> - int rc; >> + int err = 0; >> >> do { >> cxlr = __create_region(cxlrd, cxled->mode, >> @@ -3404,8 +3414,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, >> } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY); >> >> if (IS_ERR(cxlr)) { >> - dev_err(cxlmd->dev.parent, >> - "%s:%s: %s failed assign region: %ld\n", >> + dev_err(cxlmd->dev.parent, "%s:%s: %s failed assign region: %ld\n", >> dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), >> __func__, PTR_ERR(cxlr)); >> return cxlr; >> @@ -3415,19 +3424,41 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, >> p = &cxlr->params; >> if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) { >> dev_err(cxlmd->dev.parent, >> - "%s:%s: %s autodiscovery interrupted\n", >> + "%s:%s: %s region setup interrupted\n", >> dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), >> __func__); >> - rc = -EBUSY; >> - goto err; >> + err = -EBUSY; >> } >> >> + if (err) { >> + construct_region_end(); > Here I'd just release the semaphore not call this warpper as it's taken within this > function I think? No, it is taken inside construct_region_begin. It is hard to know just looking at the patch. >> + drop_region(cxlr); >> + return ERR_PTR(err); >> + } >> + return cxlr; >> +} >> + >> *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa), >> @@ -3444,6 +3475,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, >> __func__, dev_name(&cxlr->dev)); >> } >> >> + p = &cxlr->params; >> p->res = res; >> p->interleave_ways = cxled->cxld.interleave_ways; >> p->interleave_granularity = cxled->cxld.interleave_granularity; >> @@ -3451,24 +3483,106 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, >> >> rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group()); >> if (rc) >> - goto err; >> + goto out; >> >> dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n", >> - dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__, >> - dev_name(&cxlr->dev), p->res, p->interleave_ways, >> - p->interleave_granularity); >> + dev_name(&cxlmd->dev), >> + dev_name(&cxled->cxld.dev), __func__, > Avoid reformatting unless necessary. Hard to tell what changed > if anything. I'll fix it. >> + dev_name(&cxlr->dev), p->res, >> + p->interleave_ways, >> + p->interleave_granularity); >> >> /* ...to match put_device() in cxl_add_to_region() */ >> get_device(&cxlr->dev); >> up_write(&cxl_region_rwsem); >> >> +out: >> + construct_region_end(); > As below. I'd have separate error handling path. > Same in the other paths. > OK >> + if (rc) { >> + drop_region(cxlr); >> + return ERR_PTR(rc); >> + } >> return cxlr; >> +} >> >> -err: >> - up_write(&cxl_region_rwsem); >> - devm_release_action(port->uport_dev, unregister_region, cxlr); >> - return ERR_PTR(rc); >> +static struct cxl_region * >> +__construct_new_region(struct cxl_root_decoder *cxlrd, >> + struct cxl_endpoint_decoder *cxled) >> +{ >> + struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld; >> + struct cxl_region_params *p; >> + resource_size_t size = 0; > set in all paths that use it. > >> + struct cxl_region *cxlr; >> + int rc; >> + >> + cxlr = construct_region_begin(cxlrd, cxled); >> + if (IS_ERR(cxlr)) >> + return cxlr; >> + >> + rc = set_interleave_ways(cxlr, 1); >> + if (rc) >> + goto out; >> + >> + rc = set_interleave_granularity(cxlr, cxld->interleave_granularity); >> + if (rc) >> + goto out; >> + >> + size = resource_size(cxled->dpa_res); >> + > only used once > rc = alloc_hpa(cxlr, resource_size(cxled->dpa_res)); > Clever. I'll do it. >> + rc = alloc_hpa(cxlr, size); >> + if (rc) >> + goto out; >> + >> + down_read(&cxl_dpa_rwsem); >> + rc = cxl_region_attach(cxlr, cxled, 0); >> + up_read(&cxl_dpa_rwsem); >> + >> + if (rc) >> + goto out; >> + >> + rc = cxl_region_decode_commit(cxlr); >> + if (rc) >> + goto out; >> + >> + p = &cxlr->params; >> + p->state = CXL_CONFIG_COMMIT; >> +out: > I'd break out a separate error handling path and > just duplicate the next line. I'll do it. Thanks! > >> + construct_region_end(); >> + if (rc) { >> + drop_region(cxlr); >> + return ERR_PTR(rc); >> + } >> + return cxlr; >> +}
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index c6fa9e7e4909..d8c29e28e60c 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2192,7 +2192,7 @@ static int cxl_region_attach(struct cxl_region *cxlr, return 0; } -static int cxl_region_detach(struct cxl_endpoint_decoder *cxled) +int cxl_region_detach(struct cxl_endpoint_decoder *cxled) { struct cxl_port *iter, *ep_port = cxled_to_port(cxled); struct cxl_region *cxlr = cxled->cxld.region; @@ -2251,6 +2251,7 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled) put_device(&cxlr->dev); return rc; } +EXPORT_SYMBOL_NS_GPL(cxl_region_detach, CXL); void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled) { @@ -2780,6 +2781,14 @@ cxl_find_region_by_name(struct cxl_root_decoder *cxlrd, const char *name) return to_cxl_region(region_dev); } +static void drop_region(struct cxl_region *cxlr) +{ + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent); + struct cxl_port *port = cxlrd_to_port(cxlrd); + + devm_release_action(port->uport_dev, unregister_region, cxlr); +} + static ssize_t delete_region_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) @@ -3385,17 +3394,18 @@ static int match_region_by_range(struct device *dev, void *data) return rc; } -/* Establish an empty region covering the given HPA range */ -static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, - struct cxl_endpoint_decoder *cxled) +static void construct_region_end(void) +{ + up_write(&cxl_region_rwsem); +} + +static struct cxl_region *construct_region_begin(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled) { struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); - struct cxl_port *port = cxlrd_to_port(cxlrd); - struct range *hpa = &cxled->cxld.hpa_range; struct cxl_region_params *p; struct cxl_region *cxlr; - struct resource *res; - int rc; + int err = 0; do { cxlr = __create_region(cxlrd, cxled->mode, @@ -3404,8 +3414,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY); if (IS_ERR(cxlr)) { - dev_err(cxlmd->dev.parent, - "%s:%s: %s failed assign region: %ld\n", + dev_err(cxlmd->dev.parent, "%s:%s: %s failed assign region: %ld\n", dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__, PTR_ERR(cxlr)); return cxlr; @@ -3415,19 +3424,41 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, p = &cxlr->params; if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) { dev_err(cxlmd->dev.parent, - "%s:%s: %s autodiscovery interrupted\n", + "%s:%s: %s region setup interrupted\n", dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__); - rc = -EBUSY; - goto err; + err = -EBUSY; } + if (err) { + construct_region_end(); + drop_region(cxlr); + return ERR_PTR(err); + } + return cxlr; +} + +/* Establish an empty region covering the given HPA range */ +static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled) +{ + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); + struct range *hpa = &cxled->cxld.hpa_range; + struct cxl_region_params *p; + struct cxl_region *cxlr; + struct resource *res; + int rc; + + cxlr = construct_region_begin(cxlrd, cxled); + if (IS_ERR(cxlr)) + return cxlr; + set_bit(CXL_REGION_F_AUTO, &cxlr->flags); res = kmalloc(sizeof(*res), GFP_KERNEL); if (!res) { rc = -ENOMEM; - goto err; + goto out; } *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa), @@ -3444,6 +3475,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, __func__, dev_name(&cxlr->dev)); } + p = &cxlr->params; p->res = res; p->interleave_ways = cxled->cxld.interleave_ways; p->interleave_granularity = cxled->cxld.interleave_granularity; @@ -3451,24 +3483,106 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group()); if (rc) - goto err; + goto out; dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n", - dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__, - dev_name(&cxlr->dev), p->res, p->interleave_ways, - p->interleave_granularity); + dev_name(&cxlmd->dev), + dev_name(&cxled->cxld.dev), __func__, + dev_name(&cxlr->dev), p->res, + p->interleave_ways, + p->interleave_granularity); /* ...to match put_device() in cxl_add_to_region() */ get_device(&cxlr->dev); up_write(&cxl_region_rwsem); +out: + construct_region_end(); + if (rc) { + drop_region(cxlr); + return ERR_PTR(rc); + } return cxlr; +} -err: - up_write(&cxl_region_rwsem); - devm_release_action(port->uport_dev, unregister_region, cxlr); - return ERR_PTR(rc); +static struct cxl_region * +__construct_new_region(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled) +{ + struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld; + struct cxl_region_params *p; + resource_size_t size = 0; + struct cxl_region *cxlr; + int rc; + + cxlr = construct_region_begin(cxlrd, cxled); + if (IS_ERR(cxlr)) + return cxlr; + + rc = set_interleave_ways(cxlr, 1); + if (rc) + goto out; + + rc = set_interleave_granularity(cxlr, cxld->interleave_granularity); + if (rc) + goto out; + + size = resource_size(cxled->dpa_res); + + rc = alloc_hpa(cxlr, size); + if (rc) + goto out; + + down_read(&cxl_dpa_rwsem); + rc = cxl_region_attach(cxlr, cxled, 0); + up_read(&cxl_dpa_rwsem); + + if (rc) + goto out; + + rc = cxl_region_decode_commit(cxlr); + if (rc) + goto out; + + p = &cxlr->params; + p->state = CXL_CONFIG_COMMIT; +out: + construct_region_end(); + if (rc) { + drop_region(cxlr); + return ERR_PTR(rc); + } + return cxlr; +} + +/** + * cxl_create_region - Establish a region given an endpoint decoder + * @cxlrd: root decoder to allocate HPA + * @cxled: endpoint decoder with reserved DPA capacity + * + * Returns a fully formed region in the commit state and attached to the + * cxl_region driver. + */ +struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled) +{ + struct cxl_region *cxlr; + + mutex_lock(&cxlrd->range_lock); + cxlr = __construct_new_region(cxlrd, cxled); + mutex_unlock(&cxlrd->range_lock); + + if (IS_ERR(cxlr)) + return cxlr; + + if (device_attach(&cxlr->dev) <= 0) { + dev_err(&cxlr->dev, "failed to create region\n"); + drop_region(cxlr); + return ERR_PTR(-ENODEV); + } + return cxlr; } +EXPORT_SYMBOL_NS_GPL(cxl_create_region, CXL); int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 5d83e4a960ef..120e961f2e31 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -903,6 +903,7 @@ void cxl_coordinates_combine(struct access_coordinate *out, bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port); +int cxl_region_detach(struct cxl_endpoint_decoder *cxled); /* * Unit test builds overrides this to __weak, find the 'strong' version * of these symbols in tools/testing/cxl/. diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 07259840da8f..b0a66b064c73 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -878,4 +878,6 @@ void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds); struct cxl_root_decoder *cxl_get_hpa_freespace(struct cxl_port *endpoint, unsigned long flags, resource_size_t *max); +struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled); #endif /* __CXL_MEM_H__ */ diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c index 57667d753550..dd2dbfb8ba15 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -125,10 +125,19 @@ int efx_cxl_init(struct efx_nic *efx) goto err_release; } + cxl->efx_region = cxl_create_region(cxl->cxlrd, cxl->cxled); + if (!cxl->efx_region) { + pci_err(pci_dev, "CXL accel create region failed"); + rc = PTR_ERR(cxl->efx_region); + goto err_region; + } + cxl_release_endpoint(cxl->cxlmd, cxl->endpoint); return 0; +err_region: + cxl_dpa_free(efx->cxl->cxled); err_release: cxl_release_endpoint(cxl->cxlmd, cxl->endpoint); err: @@ -142,6 +151,7 @@ int efx_cxl_init(struct efx_nic *efx) void efx_cxl_exit(struct efx_nic *efx) { if (efx->cxl) { + cxl_region_detach(efx->cxl->cxled); cxl_dpa_free(efx->cxl->cxled); cxl_release_resource(efx->cxl->cxlds, CXL_ACCEL_RES_RAM); kfree(efx->cxl->cxlds); diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h index 3250342843e4..169683d75030 100644 --- a/include/linux/cxl/cxl.h +++ b/include/linux/cxl/cxl.h @@ -72,4 +72,8 @@ struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_port *endpoint, resource_size_t min, resource_size_t max); int cxl_dpa_free(struct cxl_endpoint_decoder *cxled); +struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled); + +int cxl_region_detach(struct cxl_endpoint_decoder *cxled); #endif