| Message ID | 20220128002707.391076-7-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL Region driver | expand |
On Thu, Jan 27, 2022 at 4:27 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > When a region is not assigned a host physical address, Curious, is there a way to pick one in the current ABI? Not that I want one, in fact I think Linux should make it as difficult as possible to create a region with a fixed address (per the 'HPA' field of the region label) given all the problems it can cause with decoder allocation ordering. Unless and until someone identifies a solid use case for that capability it should be de-emphasized. > one is picked by > the driver. As the address will determine which CFMWS contains the > region, it's usually a better idea to let the driver make this > determination. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/region.c | 40 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c > index cc41939a2f0a..5588873dd250 100644 > --- a/drivers/cxl/region.c > +++ b/drivers/cxl/region.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* Copyright(c) 2021 Intel Corporation. All rights reserved. */ > #include <linux/platform_device.h> > +#include <linux/genalloc.h> > #include <linux/device.h> > #include <linux/module.h> > #include <linux/pci.h> > @@ -64,6 +65,20 @@ static struct cxl_port *get_root_decoder(const struct cxl_memdev *endpoint) > return NULL; > } > > +static void release_cxl_region(void *r) > +{ > + struct cxl_region *cxlr = (struct cxl_region *)r; > + struct cxl_decoder *rootd = rootd_from_region(cxlr); > + struct resource *res = &rootd->platform_res; > + resource_size_t start, size; > + > + start = cxlr->res->start; > + size = resource_size(cxlr->res); > + > + __release_region(res, start, size); > + gen_pool_free(rootd->address_space, start, size); If the need to keep the gen_pool in sync is dropped then this open-coded devm release handler can be replaced with __devm_request_region(). > +} > + > /** > * sanitize_region() - Check is region is reasonably configured > * @cxlr: The region to check > @@ -129,8 +144,29 @@ static int sanitize_region(const struct cxl_region *cxlr) > */ > static int allocate_address_space(struct cxl_region *cxlr) > { > - /* TODO */ The problem with TODOs is now I forget which context calls allocate_address_space(). If the caller was added in this patch it would be reviewable, as is, I need to go to another window to search "allocate_address_space" to recall that it is called from cxl_region_probe(). That's too late as someone defining a region should know upfront a region creation time that space has been reserved, or not. > - return 0; > + struct cxl_decoder *rootd = rootd_from_region(cxlr); > + unsigned long start; s/unsigned long/resource_size_t/? > + > + start = gen_pool_alloc(rootd->address_space, cxlr->config.size); > + if (!start) { > + dev_dbg(&cxlr->dev, "Couldn't allocate %lluM of address space", > + cxlr->config.size >> 20); > + return -ENOMEM; > + } > + > + cxlr->res = > + __request_region(&rootd->platform_res, start, cxlr->config.size, > + dev_name(&cxlr->dev), IORESOURCE_MEM); > + if (!cxlr->res) { > + dev_dbg(&cxlr->dev, "Couldn't obtain region from %s (%pR)\n", > + dev_name(&rootd->dev), &rootd->platform_res); > + gen_pool_free(rootd->address_space, start, cxlr->config.size); > + return -ENOMEM; > + } > + > + dev_dbg(&cxlr->dev, "resource %pR", cxlr->res); > + > + return devm_add_action_or_reset(&cxlr->dev, release_cxl_region, cxlr); > } > > /** > -- > 2.35.0 >
diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c index cc41939a2f0a..5588873dd250 100644 --- a/drivers/cxl/region.c +++ b/drivers/cxl/region.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2021 Intel Corporation. All rights reserved. */ #include <linux/platform_device.h> +#include <linux/genalloc.h> #include <linux/device.h> #include <linux/module.h> #include <linux/pci.h> @@ -64,6 +65,20 @@ static struct cxl_port *get_root_decoder(const struct cxl_memdev *endpoint) return NULL; } +static void release_cxl_region(void *r) +{ + struct cxl_region *cxlr = (struct cxl_region *)r; + struct cxl_decoder *rootd = rootd_from_region(cxlr); + struct resource *res = &rootd->platform_res; + resource_size_t start, size; + + start = cxlr->res->start; + size = resource_size(cxlr->res); + + __release_region(res, start, size); + gen_pool_free(rootd->address_space, start, size); +} + /** * sanitize_region() - Check is region is reasonably configured * @cxlr: The region to check @@ -129,8 +144,29 @@ static int sanitize_region(const struct cxl_region *cxlr) */ static int allocate_address_space(struct cxl_region *cxlr) { - /* TODO */ - return 0; + struct cxl_decoder *rootd = rootd_from_region(cxlr); + unsigned long start; + + start = gen_pool_alloc(rootd->address_space, cxlr->config.size); + if (!start) { + dev_dbg(&cxlr->dev, "Couldn't allocate %lluM of address space", + cxlr->config.size >> 20); + return -ENOMEM; + } + + cxlr->res = + __request_region(&rootd->platform_res, start, cxlr->config.size, + dev_name(&cxlr->dev), IORESOURCE_MEM); + if (!cxlr->res) { + dev_dbg(&cxlr->dev, "Couldn't obtain region from %s (%pR)\n", + dev_name(&rootd->dev), &rootd->platform_res); + gen_pool_free(rootd->address_space, start, cxlr->config.size); + return -ENOMEM; + } + + dev_dbg(&cxlr->dev, "resource %pR", cxlr->res); + + return devm_add_action_or_reset(&cxlr->dev, release_cxl_region, cxlr); } /**
When a region is not assigned a host physical address, one is picked by the driver. As the address will determine which CFMWS contains the region, it's usually a better idea to let the driver make this determination. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/region.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)