[PATCH v3 0/2] cxl/region: Improve Soft Reserved resource handling

[PATCH v3 0/2] cxl/region: Improve Soft Reserved resource handling

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Changes in v3:
- dev_dbg() on failure to insert resource to a soft reserved parent (Jonathan)
  Continue, as in previous versions of this patch, to insert the resource
  to the root decoder if insert to SR parent fails or SR parent does not
  exist.
- Replace goto/unlock with guard(mutex) in remove_soft_reserved() (DaveJ)
- Use automatic resource cleanup in add_soft_reserved() (DaveJ)
- Fixups to commit log grammar (DaveJ)
 
v2: https://lore.kernel.org/linux-cxl/cover.1691176651.git.alison.schofield@intel.com/
 
Begin Cover Letter:
Make the CXL Region Driver handle these 2 observed scenarios:

1) Soft reserved resources were observed as sometimes being the parent
and sometimes being the child of a region resource. Patch 1 clears up
that inconsistency.

2) Soft reserved resources were also observed as stranded after region 
teardown, making the address space the region released unavailable for
reallocation. Patch 2 implements soft reserved resource removal.


Alison Schofield (2):
  cxl/region: Try to add a region resource to a soft reserved parent
  cxl/region: Remove a soft reserved resource at region teardown

 drivers/cxl/core/region.c | 187 ++++++++++++++++++++++++++++++++++----
 1 file changed, 168 insertions(+), 19 deletions(-)


base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
-- 
2.37.3

[PATCH v3 1/2] cxl/region: Try to add a region resource to a soft reserved parent

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

During region autodiscovery, the region driver always inserts the
region resource as a child of the root decoder, a CXL WINDOW. It
has the effect of making a soft reserved resource, with an exactly
matching address range, a child of the region resource.

It looks like this in /proc/iomem:

2080000000-29dbfffffff : CXL Window 0
  2080000000-247fffffff : region0
    2080000000-247fffffff : Soft Reserved

Search for soft reserved resources that include the region resource
and add the new region resource as a child of that found resource.
If a soft reserved resource is not found, insert to the root decoder
as usual.

With this change, it looks like this:

2080000000-29dbfffffff : CXL Window 0
  2080000000-247fffffff : Soft Reserved
    2080000000-247fffffff : region0

This odd parenting only occurs when the resources are an exact match.
When the region resource only uses part of a soft reserved resource,
the parenting appears more logical like this:

2080000000-29dbfffffff : CXL Window 0
  2080000000-287fffffff : Soft Reserved
    2080000000-247fffffff : region0

Aside from the more logical appearance, this change is in preparation
for further cleanup in region teardown. A follow-on patch intends to
find and free soft reserved resources upon region teardown.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 57 ++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e115ba382e04..5c487aab15ad 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2709,6 +2709,31 @@ static int match_region_by_range(struct device *dev, void *data)
 	return rc;
 }
 
+static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
+{
+	struct resource *parent, *new, *res = arg;
+	bool found = false;
+	int rc = 0;
+
+	parent = soft_res->parent;
+	if (!parent)
+		return 0;
+
+	/* Caller provides a copy of soft_res. Find the actual resource. */
+	for (new = parent->child; new; new = new->sibling) {
+		if (resource_contains(new, soft_res)) {
+			rc = insert_resource(new, res);
+			found = true;
+			break;
+		}
+	}
+	/* Caller handles failure to find or insert resource */
+	if (!found || rc)
+		return rc;
+
+	return 1;
+}
+
 /* 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)
@@ -2755,16 +2780,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
 				    dev_name(&cxlr->dev));
-	rc = insert_resource(cxlrd->res, res);
-	if (rc) {
-		/*
-		 * Platform-firmware may not have split resources like "System
-		 * RAM" on CXL window boundaries see cxl_region_iomem_release()
-		 */
-		dev_warn(cxlmd->dev.parent,
-			 "%s:%s: %s %s cannot insert resource\n",
-			 dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
-			 __func__, dev_name(&cxlr->dev));
+
+	/* Try inserting to a Soft Reserved parent. Fallback to root decoder */
+	rc = walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0, res->start,
+				 res->end, res, insert_resource_soft_reserved);
+	if (!rc || rc == -EBUSY)
+		dev_dbg(&cxlmd->dev,
+			"insert %pr to soft reserved parent failed rc:%d\n",
+			res, rc);
+	if (rc != 1) {
+		rc = insert_resource(cxlrd->res, res);
+		if (rc) {
+			/*
+			 * Platform-firmware may not have split resources
+			 * like "System RAM" on CXL window boundaries see
+			 * cxl_region_iomem_release()
+			 */
+			dev_warn(cxlmd->dev.parent,
+				 "%s:%s: %s %s cannot insert resource\n",
+				 dev_name(&cxlmd->dev),
+				 dev_name(&cxled->cxld.dev), __func__,
+				 dev_name(&cxlr->dev));
+		}
 	}
 
 	p->res = res;
-- 
2.37.3

[PATCH v3 2/2] cxl/region: Remove a soft reserved resource at region teardown

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

When CXL regions are created through autodiscovery their resource
may be a child of a soft reserved resource. Currently, when such a
region is destroyed, its soft reserved resource remains in place.
That means that the HPA range that the region could release is
actually unavailable for reuse.

Free the soft reserved resource on region teardown by examining
the alignment of the resources, and handling accordingly. The
two resources will be exactly aligned, partially aligned, or not
aligned at all.

|----------- "Soft Reserved" -----------|
|-------------- "Region #" -------------|
Exactly aligned. Any dangling children move up to a parent on removal.
The removal of this soft reserved seems guaranteed, however the
availability of the address range for reuse depends on complete cleanup
of the region child resources also.

|----------- "Soft Reserved" -----------|
|-------- "Region #" -------|
or
|----------- "Soft Reserved" -----------|
            |-------- "Region #" -------|
Either start or end aligns. Unlike removing a resource, which simply
moves child resources up the resource tree, adjustments fail if any
child resources map the range being truncated. So this one will fail
for dangling child resources of the region.

|---------- "Soft Reserved" ----------|
        |---- "Region #" ----|
No alignment. Freeing the resource of a region in the middle succeeds
if no child resources map the leading or trailing address space.

Reported-by: Derick Marks <derick.w.marks@intel.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 130 +++++++++++++++++++++++++++++++++++---
 1 file changed, 121 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c487aab15ad..dcf8d4ad2cf4 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -569,22 +569,134 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
 	return 0;
 }
 
+static int add_soft_reserved(struct resource *parent, resource_size_t start,
+			     resource_size_t len, unsigned long flags)
+{
+	struct resource *res __free(kfree) = kmalloc(sizeof(*res), GFP_KERNEL);
+	int rc;
+
+	if (!res)
+		return -ENOMEM;
+
+	*res = DEFINE_RES_MEM_NAMED(start, len, "Soft Reserved");
+
+	res->desc = IORES_DESC_SOFT_RESERVED;
+	res->flags = res->flags | flags;
+	rc = insert_resource(parent, res);
+	if (rc)
+		return rc;
+
+	no_free_ptr(res);
+	return 0;
+}
+
+static void remove_soft_reserved(struct cxl_region *cxlr, struct resource *soft,
+				 resource_size_t start, resource_size_t end)
+{
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	resource_size_t new_start, new_end;
+	struct resource *parent;
+	unsigned long flags;
+	int rc;
+
+	/* Prevent new usage while removing or adjusting the resource */
+	guard(mutex)(&cxlrd->range_lock);
+
+	/* Aligns at both resource start and end */
+	if (soft->start == start && soft->end == end) {
+		rc = remove_resource(soft);
+		if (rc)
+			dev_dbg(&cxlr->dev,
+				"cannot remove soft reserved resource %pr\n",
+				soft);
+		else
+			kfree(soft);
+
+		return;
+	}
+
+	/* Aligns at either resource start or end */
+	if (soft->start == start || soft->end == end) {
+		if (soft->start == start) {
+			new_start = end + 1;
+			new_end = soft->end;
+		} else {
+			new_start = soft->start;
+			new_end = start + 1;
+		}
+		rc =  adjust_resource(soft, new_start, new_end - new_start + 1);
+		if (rc)
+			dev_dbg(&cxlr->dev,
+				"cannot adjust soft reserved resource %pr\n",
+				soft);
+		return;
+	}
+
+	/*
+	 * No alignment. Attempt a 3-way split that removes the part of
+	 * the resource the region occupied, and then creates new soft
+	 * reserved resources for the leading and trailing addr space.
+	 * adjust_resource() will stop the attempt if there are any
+	 * child resources.
+	 */
+
+	/* Save the original soft reserved resource params before adjusting */
+	new_start = soft->start;
+	new_end = soft->end;
+	parent = soft->parent;
+	flags = soft->flags;
+
+	rc = adjust_resource(soft, start, end - start);
+	if (rc) {
+		dev_dbg(&cxlr->dev,
+			"cannot adjust soft reserved resource %pr\n", soft);
+		return;
+	}
+	rc = remove_resource(soft);
+	if (rc)
+		dev_warn(&cxlr->dev,
+			 "cannot remove soft reserved resource %pr\n", soft);
+
+	rc = add_soft_reserved(parent, new_start, start - new_start, flags);
+	if (rc)
+		dev_warn(&cxlr->dev,
+			 "cannot add new soft reserved resource at %pa\n",
+			 &new_start);
+
+	rc = add_soft_reserved(parent, end + 1, new_end - end, flags);
+	if (rc)
+		dev_warn(&cxlr->dev,
+			 "cannot add new soft reserved resource at %pa + 1\n",
+			 &end);
+}
+
 static void cxl_region_iomem_release(struct cxl_region *cxlr)
 {
 	struct cxl_region_params *p = &cxlr->params;
+	struct resource *parent, *res = p->res;
+	resource_size_t start, end;
 
 	if (device_is_registered(&cxlr->dev))
 		lockdep_assert_held_write(&cxl_region_rwsem);
-	if (p->res) {
-		/*
-		 * Autodiscovered regions may not have been able to insert their
-		 * resource.
-		 */
-		if (p->res->parent)
-			remove_resource(p->res);
-		kfree(p->res);
-		p->res = NULL;
+	if (!res)
+		return;
+	/*
+	 * Autodiscovered regions may not have been able to insert their
+	 * resource. If a Soft Reserved parent resource exists, try to
+	 * remove that also.
+	 */
+	if (p->res->parent) {
+		parent = p->res->parent;
+		start = p->res->start;
+		end = p->res->end;
+		remove_resource(p->res);
+		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
+		    parent->desc == IORES_DESC_SOFT_RESERVED)
+			remove_soft_reserved(cxlr, parent, start, end);
 	}
+
+	kfree(p->res);
+	p->res = NULL;
 }
 
 static int free_hpa(struct cxl_region *cxlr)
-- 
2.37.3

Re: [PATCH v3 1/2] cxl/region: Try to add a region resource to a soft reserved parent

[Dave Jiang]

On 8/21/23 11:14, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> During region autodiscovery, the region driver always inserts the
> region resource as a child of the root decoder, a CXL WINDOW. It
> has the effect of making a soft reserved resource, with an exactly
> matching address range, a child of the region resource.
> 
> It looks like this in /proc/iomem:
> 
> 2080000000-29dbfffffff : CXL Window 0
>    2080000000-247fffffff : region0
>      2080000000-247fffffff : Soft Reserved
> 
> Search for soft reserved resources that include the region resource
> and add the new region resource as a child of that found resource.
> If a soft reserved resource is not found, insert to the root decoder
> as usual.
> 
> With this change, it looks like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>    2080000000-247fffffff : Soft Reserved
>      2080000000-247fffffff : region0
> 
> This odd parenting only occurs when the resources are an exact match.
> When the region resource only uses part of a soft reserved resource,
> the parenting appears more logical like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>    2080000000-287fffffff : Soft Reserved
>      2080000000-247fffffff : region0
> 
> Aside from the more logical appearance, this change is in preparation
> for further cleanup in region teardown. A follow-on patch intends to
> find and free soft reserved resources upon region teardown.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/core/region.c | 57 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..5c487aab15ad 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2709,6 +2709,31 @@ static int match_region_by_range(struct device *dev, void *data)
>   	return rc;
>   }
>   
> +static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
> +{
> +	struct resource *parent, *new, *res = arg;
> +	bool found = false;
> +	int rc = 0;
> +
> +	parent = soft_res->parent;
> +	if (!parent)
> +		return 0;
> +
> +	/* Caller provides a copy of soft_res. Find the actual resource. */
> +	for (new = parent->child; new; new = new->sibling) {
> +		if (resource_contains(new, soft_res)) {
> +			rc = insert_resource(new, res);
> +			found = true;
> +			break;
> +		}
> +	}
> +	/* Caller handles failure to find or insert resource */
> +	if (!found || rc)
> +		return rc;
> +
> +	return 1;
> +}
> +
>   /* 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)
> @@ -2755,16 +2780,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   
>   	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
>   				    dev_name(&cxlr->dev));
> -	rc = insert_resource(cxlrd->res, res);
> -	if (rc) {
> -		/*
> -		 * Platform-firmware may not have split resources like "System
> -		 * RAM" on CXL window boundaries see cxl_region_iomem_release()
> -		 */
> -		dev_warn(cxlmd->dev.parent,
> -			 "%s:%s: %s %s cannot insert resource\n",
> -			 dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> -			 __func__, dev_name(&cxlr->dev));
> +
> +	/* Try inserting to a Soft Reserved parent. Fallback to root decoder */
> +	rc = walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0, res->start,
> +				 res->end, res, insert_resource_soft_reserved);
> +	if (!rc || rc == -EBUSY)
> +		dev_dbg(&cxlmd->dev,
> +			"insert %pr to soft reserved parent failed rc:%d\n",
> +			res, rc);
> +	if (rc != 1) {
> +		rc = insert_resource(cxlrd->res, res);
> +		if (rc) {
> +			/*
> +			 * Platform-firmware may not have split resources
> +			 * like "System RAM" on CXL window boundaries see
> +			 * cxl_region_iomem_release()
> +			 */
> +			dev_warn(cxlmd->dev.parent,
> +				 "%s:%s: %s %s cannot insert resource\n",
> +				 dev_name(&cxlmd->dev),
> +				 dev_name(&cxled->cxld.dev), __func__,
> +				 dev_name(&cxlr->dev));
> +		}
>   	}
>   
>   	p->res = res;

Re: [PATCH v3 2/2] cxl/region: Remove a soft reserved resource at region teardown

[Dave Jiang]

On 8/21/23 11:14, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> When CXL regions are created through autodiscovery their resource
> may be a child of a soft reserved resource. Currently, when such a
> region is destroyed, its soft reserved resource remains in place.
> That means that the HPA range that the region could release is
> actually unavailable for reuse.
> 
> Free the soft reserved resource on region teardown by examining
> the alignment of the resources, and handling accordingly. The
> two resources will be exactly aligned, partially aligned, or not
> aligned at all.
> 
> |----------- "Soft Reserved" -----------|
> |-------------- "Region #" -------------|
> Exactly aligned. Any dangling children move up to a parent on removal.
> The removal of this soft reserved seems guaranteed, however the
> availability of the address range for reuse depends on complete cleanup
> of the region child resources also.
> 
> |----------- "Soft Reserved" -----------|
> |-------- "Region #" -------|
> or
> |----------- "Soft Reserved" -----------|
>              |-------- "Region #" -------|
> Either start or end aligns. Unlike removing a resource, which simply
> moves child resources up the resource tree, adjustments fail if any
> child resources map the range being truncated. So this one will fail
> for dangling child resources of the region.
> 
> |---------- "Soft Reserved" ----------|
>          |---- "Region #" ----|
> No alignment. Freeing the resource of a region in the middle succeeds
> if no child resources map the leading or trailing address space.
> 
> Reported-by: Derick Marks <derick.w.marks@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/core/region.c | 130 +++++++++++++++++++++++++++++++++++---
>   1 file changed, 121 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c487aab15ad..dcf8d4ad2cf4 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -569,22 +569,134 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
>   	return 0;
>   }
>   
> +static int add_soft_reserved(struct resource *parent, resource_size_t start,
> +			     resource_size_t len, unsigned long flags)
> +{
> +	struct resource *res __free(kfree) = kmalloc(sizeof(*res), GFP_KERNEL);
> +	int rc;
> +
> +	if (!res)
> +		return -ENOMEM;
> +
> +	*res = DEFINE_RES_MEM_NAMED(start, len, "Soft Reserved");
> +
> +	res->desc = IORES_DESC_SOFT_RESERVED;
> +	res->flags = res->flags | flags;
> +	rc = insert_resource(parent, res);
> +	if (rc)
> +		return rc;
> +
> +	no_free_ptr(res);
> +	return 0;
> +}
> +
> +static void remove_soft_reserved(struct cxl_region *cxlr, struct resource *soft,
> +				 resource_size_t start, resource_size_t end)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	resource_size_t new_start, new_end;
> +	struct resource *parent;
> +	unsigned long flags;
> +	int rc;
> +
> +	/* Prevent new usage while removing or adjusting the resource */
> +	guard(mutex)(&cxlrd->range_lock);
> +
> +	/* Aligns at both resource start and end */
> +	if (soft->start == start && soft->end == end) {
> +		rc = remove_resource(soft);
> +		if (rc)
> +			dev_dbg(&cxlr->dev,
> +				"cannot remove soft reserved resource %pr\n",
> +				soft);
> +		else
> +			kfree(soft);
> +
> +		return;
> +	}
> +
> +	/* Aligns at either resource start or end */
> +	if (soft->start == start || soft->end == end) {
> +		if (soft->start == start) {
> +			new_start = end + 1;
> +			new_end = soft->end;
> +		} else {
> +			new_start = soft->start;
> +			new_end = start + 1;
> +		}
> +		rc =  adjust_resource(soft, new_start, new_end - new_start + 1);
> +		if (rc)
> +			dev_dbg(&cxlr->dev,
> +				"cannot adjust soft reserved resource %pr\n",
> +				soft);
> +		return;
> +	}
> +
> +	/*
> +	 * No alignment. Attempt a 3-way split that removes the part of
> +	 * the resource the region occupied, and then creates new soft
> +	 * reserved resources for the leading and trailing addr space.
> +	 * adjust_resource() will stop the attempt if there are any
> +	 * child resources.
> +	 */
> +
> +	/* Save the original soft reserved resource params before adjusting */
> +	new_start = soft->start;
> +	new_end = soft->end;
> +	parent = soft->parent;
> +	flags = soft->flags;
> +
> +	rc = adjust_resource(soft, start, end - start);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev,
> +			"cannot adjust soft reserved resource %pr\n", soft);
> +		return;
> +	}
> +	rc = remove_resource(soft);
> +	if (rc)
> +		dev_warn(&cxlr->dev,
> +			 "cannot remove soft reserved resource %pr\n", soft);
> +
> +	rc = add_soft_reserved(parent, new_start, start - new_start, flags);
> +	if (rc)
> +		dev_warn(&cxlr->dev,
> +			 "cannot add new soft reserved resource at %pa\n",
> +			 &new_start);
> +
> +	rc = add_soft_reserved(parent, end + 1, new_end - end, flags);
> +	if (rc)
> +		dev_warn(&cxlr->dev,
> +			 "cannot add new soft reserved resource at %pa + 1\n",
> +			 &end);
> +}
> +
>   static void cxl_region_iomem_release(struct cxl_region *cxlr)
>   {
>   	struct cxl_region_params *p = &cxlr->params;
> +	struct resource *parent, *res = p->res;
> +	resource_size_t start, end;
>   
>   	if (device_is_registered(&cxlr->dev))
>   		lockdep_assert_held_write(&cxl_region_rwsem);
> -	if (p->res) {
> -		/*
> -		 * Autodiscovered regions may not have been able to insert their
> -		 * resource.
> -		 */
> -		if (p->res->parent)
> -			remove_resource(p->res);
> -		kfree(p->res);
> -		p->res = NULL;
> +	if (!res)
> +		return;
> +	/*
> +	 * Autodiscovered regions may not have been able to insert their
> +	 * resource. If a Soft Reserved parent resource exists, try to
> +	 * remove that also.
> +	 */
> +	if (p->res->parent) {
> +		parent = p->res->parent;
> +		start = p->res->start;
> +		end = p->res->end;
> +		remove_resource(p->res);
> +		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
> +		    parent->desc == IORES_DESC_SOFT_RESERVED)
> +			remove_soft_reserved(cxlr, parent, start, end);
>   	}
> +
> +	kfree(p->res);
> +	p->res = NULL;
>   }
>   
>   static int free_hpa(struct cxl_region *cxlr)

Re: [PATCH v3 1/2] cxl/region: Try to add a region resource to a soft reserved parent

[Jonathan Cameron]

On Mon, 21 Aug 2023 11:14:36 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> During region autodiscovery, the region driver always inserts the
> region resource as a child of the root decoder, a CXL WINDOW. It
> has the effect of making a soft reserved resource, with an exactly
> matching address range, a child of the region resource.
> 
> It looks like this in /proc/iomem:
> 
> 2080000000-29dbfffffff : CXL Window 0
>   2080000000-247fffffff : region0
>     2080000000-247fffffff : Soft Reserved
> 
> Search for soft reserved resources that include the region resource
> and add the new region resource as a child of that found resource.
> If a soft reserved resource is not found, insert to the root decoder
> as usual.
> 
> With this change, it looks like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>   2080000000-247fffffff : Soft Reserved
>     2080000000-247fffffff : region0
> 
> This odd parenting only occurs when the resources are an exact match.
> When the region resource only uses part of a soft reserved resource,
> the parenting appears more logical like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>   2080000000-287fffffff : Soft Reserved
>     2080000000-247fffffff : region0
> 
> Aside from the more logical appearance, this change is in preparation
> for further cleanup in region teardown. A follow-on patch intends to
> find and free soft reserved resources upon region teardown.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
One trivial comment inline. Feel free to ignore.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c | 57 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..5c487aab15ad 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2709,6 +2709,31 @@ static int match_region_by_range(struct device *dev, void *data)
>  	return rc;
>  }
>  
> +static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
> +{
> +	struct resource *parent, *new, *res = arg;
> +	bool found = false;
> +	int rc = 0;
> +
> +	parent = soft_res->parent;
> +	if (!parent)
> +		return 0;
> +
> +	/* Caller provides a copy of soft_res. Find the actual resource. */
> +	for (new = parent->child; new; new = new->sibling) {
> +		if (resource_contains(new, soft_res)) {
> +			rc = insert_resource(new, res);
> +			found = true;
> +			break;
> +		}
> +	}
> +	/* Caller handles failure to find or insert resource */
> +	if (!found || rc)
> +		return rc;

rc is only set in the found path, so could move this check up there to
simplify flow a tiny bit.  Not that important...

> +
> +	return 1;
> +}
> +
>  /* 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)
> @@ -2755,16 +2780,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  
>  	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
>  				    dev_name(&cxlr->dev));
> -	rc = insert_resource(cxlrd->res, res);
> -	if (rc) {
> -		/*
> -		 * Platform-firmware may not have split resources like "System
> -		 * RAM" on CXL window boundaries see cxl_region_iomem_release()
> -		 */
> -		dev_warn(cxlmd->dev.parent,
> -			 "%s:%s: %s %s cannot insert resource\n",
> -			 dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> -			 __func__, dev_name(&cxlr->dev));
> +
> +	/* Try inserting to a Soft Reserved parent. Fallback to root decoder */
> +	rc = walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0, res->start,
> +				 res->end, res, insert_resource_soft_reserved);
> +	if (!rc || rc == -EBUSY)
> +		dev_dbg(&cxlmd->dev,
> +			"insert %pr to soft reserved parent failed rc:%d\n",
> +			res, rc);
> +	if (rc != 1) {
> +		rc = insert_resource(cxlrd->res, res);
> +		if (rc) {
> +			/*
> +			 * Platform-firmware may not have split resources
> +			 * like "System RAM" on CXL window boundaries see
> +			 * cxl_region_iomem_release()
> +			 */
> +			dev_warn(cxlmd->dev.parent,
> +				 "%s:%s: %s %s cannot insert resource\n",
> +				 dev_name(&cxlmd->dev),
> +				 dev_name(&cxled->cxld.dev), __func__,
> +				 dev_name(&cxlr->dev));
> +		}
>  	}
>  
>  	p->res = res;

Re: [PATCH v3 2/2] cxl/region: Remove a soft reserved resource at region teardown

[Jonathan Cameron]

On Mon, 21 Aug 2023 11:14:37 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> When CXL regions are created through autodiscovery their resource
> may be a child of a soft reserved resource. Currently, when such a
> region is destroyed, its soft reserved resource remains in place.
> That means that the HPA range that the region could release is
> actually unavailable for reuse.
> 
> Free the soft reserved resource on region teardown by examining
> the alignment of the resources, and handling accordingly. The
> two resources will be exactly aligned, partially aligned, or not
> aligned at all.
> 
> |----------- "Soft Reserved" -----------|
> |-------------- "Region #" -------------|
> Exactly aligned. Any dangling children move up to a parent on removal.
> The removal of this soft reserved seems guaranteed, however the
> availability of the address range for reuse depends on complete cleanup
> of the region child resources also.
> 
> |----------- "Soft Reserved" -----------|
> |-------- "Region #" -------|
> or
> |----------- "Soft Reserved" -----------|
>             |-------- "Region #" -------|
> Either start or end aligns. Unlike removing a resource, which simply
> moves child resources up the resource tree, adjustments fail if any
> child resources map the range being truncated. So this one will fail
> for dangling child resources of the region.
> 
> |---------- "Soft Reserved" ----------|
>         |---- "Region #" ----|
> No alignment. Freeing the resource of a region in the middle succeeds
> if no child resources map the leading or trailing address space.
> 
> Reported-by: Derick Marks <derick.w.marks@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/region.c | 130 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 121 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c487aab15ad..dcf8d4ad2cf4 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -569,22 +569,134 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
>  	return 0;
>  }
>  
> +static int add_soft_reserved(struct resource *parent, resource_size_t start,
> +			     resource_size_t len, unsigned long flags)
> +{
> +	struct resource *res __free(kfree) = kmalloc(sizeof(*res), GFP_KERNEL);
> +	int rc;
> +
> +	if (!res)
> +		return -ENOMEM;
> +
> +	*res = DEFINE_RES_MEM_NAMED(start, len, "Soft Reserved");
> +
> +	res->desc = IORES_DESC_SOFT_RESERVED;
> +	res->flags = res->flags | flags;
> +	rc = insert_resource(parent, res);
> +	if (rc)
> +		return rc;
> +
> +	no_free_ptr(res);

nitpick but I'd like a blank line here :)

> +	return 0;
> +}
> +
> +static void remove_soft_reserved(struct cxl_region *cxlr, struct resource *soft,
> +				 resource_size_t start, resource_size_t end)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	resource_size_t new_start, new_end;
> +	struct resource *parent;
> +	unsigned long flags;
> +	int rc;
> +
> +	/* Prevent new usage while removing or adjusting the resource */
> +	guard(mutex)(&cxlrd->range_lock);
> +
> +	/* Aligns at both resource start and end */
> +	if (soft->start == start && soft->end == end) {
> +		rc = remove_resource(soft);
> +		if (rc)
> +			dev_dbg(&cxlr->dev,
> +				"cannot remove soft reserved resource %pr\n",
> +				soft);
> +		else
> +			kfree(soft);
Really trivial readability comment but I'd prefer...
		if (rc) {
			dev_dbg(&cxlr->...
			        soft);
			return
		}

		kfree(soft)

		return;

So that the error path is indented and the good path not.
Maybe that's just how my brain works though so feel free to ignore this one. 

> +
> +		return;
> +	}
> +
> +	/* Aligns at either resource start or end */
> +	if (soft->start == start || soft->end == end) {
> +		if (soft->start == start) {
> +			new_start = end + 1;
> +			new_end = soft->end;
> +		} else {
> +			new_start = soft->start;
> +			new_end = start + 1;
> +		}
> +		rc =  adjust_resource(soft, new_start, new_end - new_start + 1);
> +		if (rc)
> +			dev_dbg(&cxlr->dev,
> +				"cannot adjust soft reserved resource %pr\n",
> +				soft);
> +		return;
> +	}
> +
> +	/*
> +	 * No alignment. Attempt a 3-way split that removes the part of
> +	 * the resource the region occupied, and then creates new soft
> +	 * reserved resources for the leading and trailing addr space.
> +	 * adjust_resource() will stop the attempt if there are any
> +	 * child resources.
> +	 */
> +
> +	/* Save the original soft reserved resource params before adjusting */
> +	new_start = soft->start;
> +	new_end = soft->end;
> +	parent = soft->parent;
> +	flags = soft->flags;
> +
> +	rc = adjust_resource(soft, start, end - start);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev,
> +			"cannot adjust soft reserved resource %pr\n", soft);
> +		return;
> +	}
> +	rc = remove_resource(soft);
> +	if (rc)
> +		dev_warn(&cxlr->dev,
> +			 "cannot remove soft reserved resource %pr\n", soft);

I'd like a comment on this first bit.  Why adjust a resource only to throw it
away?


> +
> +	rc = add_soft_reserved(parent, new_start, start - new_start, flags);
> +	if (rc)
> +		dev_warn(&cxlr->dev,
> +			 "cannot add new soft reserved resource at %pa\n",
> +			 &new_start);
> +
> +	rc = add_soft_reserved(parent, end + 1, new_end - end, flags);
> +	if (rc)
> +		dev_warn(&cxlr->dev,
> +			 "cannot add new soft reserved resource at %pa + 1\n",
> +			 &end);
> +}
> +
>  static void cxl_region_iomem_release(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> +	struct resource *parent, *res = p->res;
> +	resource_size_t start, end;
>  
>  	if (device_is_registered(&cxlr->dev))
>  		lockdep_assert_held_write(&cxl_region_rwsem);
> -	if (p->res) {
> -		/*
> -		 * Autodiscovered regions may not have been able to insert their
> -		 * resource.
> -		 */
> -		if (p->res->parent)
> -			remove_resource(p->res);
> -		kfree(p->res);
> -		p->res = NULL;
> +	if (!res)
> +		return;
> +	/*
> +	 * Autodiscovered regions may not have been able to insert their
> +	 * resource. If a Soft Reserved parent resource exists, try to
> +	 * remove that also.
> +	 */
> +	if (p->res->parent) {
> +		parent = p->res->parent;
> +		start = p->res->start;
> +		end = p->res->end;
> +		remove_resource(p->res);
> +		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
> +		    parent->desc == IORES_DESC_SOFT_RESERVED)
> +			remove_soft_reserved(cxlr, parent, start, end);
>  	}
> +
> +	kfree(p->res);
> +	p->res = NULL;
>  }
>  
>  static int free_hpa(struct cxl_region *cxlr)

Re: [PATCH v3 2/2] cxl/region: Remove a soft reserved resource at region teardown

[Alison Schofield]

On Fri, Aug 25, 2023 at 12:54:15PM +0100, Jonathan Cameron wrote:
> On Mon, 21 Aug 2023 11:14:37 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > When CXL regions are created through autodiscovery their resource
> > may be a child of a soft reserved resource. Currently, when such a
> > region is destroyed, its soft reserved resource remains in place.
> > That means that the HPA range that the region could release is
> > actually unavailable for reuse.
> > 
> > Free the soft reserved resource on region teardown by examining
> > the alignment of the resources, and handling accordingly. The
> > two resources will be exactly aligned, partially aligned, or not
> > aligned at all.
> > 
> > |----------- "Soft Reserved" -----------|
> > |-------------- "Region #" -------------|
> > Exactly aligned. Any dangling children move up to a parent on removal.
> > The removal of this soft reserved seems guaranteed, however the
> > availability of the address range for reuse depends on complete cleanup
> > of the region child resources also.
> > 
> > |----------- "Soft Reserved" -----------|
> > |-------- "Region #" -------|
> > or
> > |----------- "Soft Reserved" -----------|
> >             |-------- "Region #" -------|
> > Either start or end aligns. Unlike removing a resource, which simply
> > moves child resources up the resource tree, adjustments fail if any
> > child resources map the range being truncated. So this one will fail
> > for dangling child resources of the region.
> > 
> > |---------- "Soft Reserved" ----------|
> >         |---- "Region #" ----|
> > No alignment. Freeing the resource of a region in the middle succeeds
> > if no child resources map the leading or trailing address space.
> > 
> > Reported-by: Derick Marks <derick.w.marks@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 130 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 121 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5c487aab15ad..dcf8d4ad2cf4 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -569,22 +569,134 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
> >  	return 0;
> >  }
> >  
> > +static int add_soft_reserved(struct resource *parent, resource_size_t start,
> > +			     resource_size_t len, unsigned long flags)
> > +{
> > +	struct resource *res __free(kfree) = kmalloc(sizeof(*res), GFP_KERNEL);
> > +	int rc;
> > +
> > +	if (!res)
> > +		return -ENOMEM;
> > +
> > +	*res = DEFINE_RES_MEM_NAMED(start, len, "Soft Reserved");
> > +
> > +	res->desc = IORES_DESC_SOFT_RESERVED;
> > +	res->flags = res->flags | flags;
> > +	rc = insert_resource(parent, res);
> > +	if (rc)
> > +		return rc;
> > +
> > +	no_free_ptr(res);
> 
> nitpick but I'd like a blank line here :)

Got it.

> 
> > +	return 0;
> > +}
> > +
> > +static void remove_soft_reserved(struct cxl_region *cxlr, struct resource *soft,
> > +				 resource_size_t start, resource_size_t end)
> > +{
> > +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> > +	resource_size_t new_start, new_end;
> > +	struct resource *parent;
> > +	unsigned long flags;
> > +	int rc;
> > +
> > +	/* Prevent new usage while removing or adjusting the resource */
> > +	guard(mutex)(&cxlrd->range_lock);
> > +
> > +	/* Aligns at both resource start and end */
> > +	if (soft->start == start && soft->end == end) {
> > +		rc = remove_resource(soft);
> > +		if (rc)
> > +			dev_dbg(&cxlr->dev,
> > +				"cannot remove soft reserved resource %pr\n",
> > +				soft);
> > +		else
> > +			kfree(soft);
> Really trivial readability comment but I'd prefer...
> 		if (rc) {
> 			dev_dbg(&cxlr->...
> 			        soft);
> 			return
> 		}
> 
> 		kfree(soft)
> 
> 		return;
> 
> So that the error path is indented and the good path not.
> Maybe that's just how my brain works though so feel free to ignore this one. 

I'll do it. I appreciate the suggestion.

> 
> > +
> > +		return;
> > +	}
> > +
> > +	/* Aligns at either resource start or end */
> > +	if (soft->start == start || soft->end == end) {
> > +		if (soft->start == start) {
> > +			new_start = end + 1;
> > +			new_end = soft->end;
> > +		} else {
> > +			new_start = soft->start;
> > +			new_end = start + 1;
> > +		}
> > +		rc =  adjust_resource(soft, new_start, new_end - new_start + 1);
> > +		if (rc)
> > +			dev_dbg(&cxlr->dev,
> > +				"cannot adjust soft reserved resource %pr\n",
> > +				soft);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * No alignment. Attempt a 3-way split that removes the part of
> > +	 * the resource the region occupied, and then creates new soft
> > +	 * reserved resources for the leading and trailing addr space.
> > +	 * adjust_resource() will stop the attempt if there are any
> > +	 * child resources.
> > +	 */
> > +
> > +	/* Save the original soft reserved resource params before adjusting */
> > +	new_start = soft->start;
> > +	new_end = soft->end;
> > +	parent = soft->parent;
> > +	flags = soft->flags;
> > +
> > +	rc = adjust_resource(soft, start, end - start);
> > +	if (rc) {
> > +		dev_dbg(&cxlr->dev,
> > +			"cannot adjust soft reserved resource %pr\n", soft);
> > +		return;
> > +	}
> > +	rc = remove_resource(soft);
> > +	if (rc)
> > +		dev_warn(&cxlr->dev,
> > +			 "cannot remove soft reserved resource %pr\n", soft);
> 
> I'd like a comment on this first bit.  Why adjust a resource only to throw it
> away?

I intended to give the big picture in the block comment above. Scroll up
and see /* No alignment... 

Did you overlook that or do you want more explanation or something else?

Thanks,
Alison

> 
> 
> > +
> > +	rc = add_soft_reserved(parent, new_start, start - new_start, flags);
> > +	if (rc)
> > +		dev_warn(&cxlr->dev,
> > +			 "cannot add new soft reserved resource at %pa\n",
> > +			 &new_start);
> > +
> > +	rc = add_soft_reserved(parent, end + 1, new_end - end, flags);
> > +	if (rc)
> > +		dev_warn(&cxlr->dev,
> > +			 "cannot add new soft reserved resource at %pa + 1\n",
> > +			 &end);
> > +}
> > +
> >  static void cxl_region_iomem_release(struct cxl_region *cxlr)
> >  {
> >  	struct cxl_region_params *p = &cxlr->params;
> > +	struct resource *parent, *res = p->res;
> > +	resource_size_t start, end;
> >  
> >  	if (device_is_registered(&cxlr->dev))
> >  		lockdep_assert_held_write(&cxl_region_rwsem);
> > -	if (p->res) {
> > -		/*
> > -		 * Autodiscovered regions may not have been able to insert their
> > -		 * resource.
> > -		 */
> > -		if (p->res->parent)
> > -			remove_resource(p->res);
> > -		kfree(p->res);
> > -		p->res = NULL;
> > +	if (!res)
> > +		return;
> > +	/*
> > +	 * Autodiscovered regions may not have been able to insert their
> > +	 * resource. If a Soft Reserved parent resource exists, try to
> > +	 * remove that also.
> > +	 */
> > +	if (p->res->parent) {
> > +		parent = p->res->parent;
> > +		start = p->res->start;
> > +		end = p->res->end;
> > +		remove_resource(p->res);
> > +		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
> > +		    parent->desc == IORES_DESC_SOFT_RESERVED)
> > +			remove_soft_reserved(cxlr, parent, start, end);
> >  	}
> > +
> > +	kfree(p->res);
> > +	p->res = NULL;
> >  }
> >  
> >  static int free_hpa(struct cxl_region *cxlr)
> 

Re: [PATCH v3 2/2] cxl/region: Remove a soft reserved resource at region teardown

[Jonathan Cameron]

Hi Alison,

> > > +	/*
> > > +	 * No alignment. Attempt a 3-way split that removes the part of
> > > +	 * the resource the region occupied, and then creates new soft
> > > +	 * reserved resources for the leading and trailing addr space.
> > > +	 * adjust_resource() will stop the attempt if there are any
> > > +	 * child resources.
> > > +	 */
> > > +
> > > +	/* Save the original soft reserved resource params before adjusting */
> > > +	new_start = soft->start;
> > > +	new_end = soft->end;
> > > +	parent = soft->parent;
> > > +	flags = soft->flags;
> > > +
> > > +	rc = adjust_resource(soft, start, end - start);
> > > +	if (rc) {
> > > +		dev_dbg(&cxlr->dev,
> > > +			"cannot adjust soft reserved resource %pr\n", soft);
> > > +		return;
> > > +	}
> > > +	rc = remove_resource(soft);
> > > +	if (rc)
> > > +		dev_warn(&cxlr->dev,
> > > +			 "cannot remove soft reserved resource %pr\n", soft);  
> > 
> > I'd like a comment on this first bit.  Why adjust a resource only to throw it
> > away?  
> 
> I intended to give the big picture in the block comment above. Scroll up
> and see /* No alignment... 
> 
> Did you overlook that or do you want more explanation or something else?

Overlooked it as you guessed.  Makes sense though it's ugly that we need
that comment.  Maybe worth explicitly checking that there are no children
rather than using a side effect of adjust_resource()?
Might need a new generic resource_has_children() check or similar.

Jonathan
> 
> Thanks,
> Alison
> 
> > 
> >   
> > > +
> > > +	rc = add_soft_reserved(parent, new_start, start - new_start, flags);
> > > +	if (rc)
> > > +		dev_warn(&cxlr->dev,
> > > +			 "cannot add new soft reserved resource at %pa\n",
> > > +			 &new_start);
> > > +
> > > +	rc = add_soft_reserved(parent, end + 1, new_end - end, flags);
> > > +	if (rc)
> > > +		dev_warn(&cxlr->dev,
> > > +			 "cannot add new soft reserved resource at %pa + 1\n",
> > > +			 &end);
> > > +}
> > > +
> > >  static void cxl_region_iomem_release(struct cxl_region *cxlr)
> > >  {
> > >  	struct cxl_region_params *p = &cxlr->params;
> > > +	struct resource *parent, *res = p->res;
> > > +	resource_size_t start, end;
> > >  
> > >  	if (device_is_registered(&cxlr->dev))
> > >  		lockdep_assert_held_write(&cxl_region_rwsem);
> > > -	if (p->res) {
> > > -		/*
> > > -		 * Autodiscovered regions may not have been able to insert their
> > > -		 * resource.
> > > -		 */
> > > -		if (p->res->parent)
> > > -			remove_resource(p->res);
> > > -		kfree(p->res);
> > > -		p->res = NULL;
> > > +	if (!res)
> > > +		return;
> > > +	/*
> > > +	 * Autodiscovered regions may not have been able to insert their
> > > +	 * resource. If a Soft Reserved parent resource exists, try to
> > > +	 * remove that also.
> > > +	 */
> > > +	if (p->res->parent) {
> > > +		parent = p->res->parent;
> > > +		start = p->res->start;
> > > +		end = p->res->end;
> > > +		remove_resource(p->res);
> > > +		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
> > > +		    parent->desc == IORES_DESC_SOFT_RESERVED)
> > > +			remove_soft_reserved(cxlr, parent, start, end);
> > >  	}
> > > +
> > > +	kfree(p->res);
> > > +	p->res = NULL;
> > >  }
> > >  
> > >  static int free_hpa(struct cxl_region *cxlr)  
> >   

Re: [PATCH v3 2/2] cxl/region: Remove a soft reserved resource at region teardown

[Alison Schofield]

On Tue, Aug 29, 2023 at 02:17:02PM +0100, Jonathan Cameron wrote:
> 
> Hi Alison,
> 
> > > > +	/*
> > > > +	 * No alignment. Attempt a 3-way split that removes the part of
> > > > +	 * the resource the region occupied, and then creates new soft
> > > > +	 * reserved resources for the leading and trailing addr space.
> > > > +	 * adjust_resource() will stop the attempt if there are any
> > > > +	 * child resources.
> > > > +	 */
> > > > +
> > > > +	/* Save the original soft reserved resource params before adjusting */
> > > > +	new_start = soft->start;
> > > > +	new_end = soft->end;
> > > > +	parent = soft->parent;
> > > > +	flags = soft->flags;
> > > > +
> > > > +	rc = adjust_resource(soft, start, end - start);
> > > > +	if (rc) {
> > > > +		dev_dbg(&cxlr->dev,
> > > > +			"cannot adjust soft reserved resource %pr\n", soft);
> > > > +		return;
> > > > +	}
> > > > +	rc = remove_resource(soft);
> > > > +	if (rc)
> > > > +		dev_warn(&cxlr->dev,
> > > > +			 "cannot remove soft reserved resource %pr\n", soft);  
> > > 
> > > I'd like a comment on this first bit.  Why adjust a resource only to throw it
> > > away?  
> > 
> > I intended to give the big picture in the block comment above. Scroll up
> > and see /* No alignment... 
> > 
> > Did you overlook that or do you want more explanation or something else?
> 
> Overlooked it as you guessed.  Makes sense though it's ugly that we need
> that comment.  Maybe worth explicitly checking that there are no children
> rather than using a side effect of adjust_resource()?
> Might need a new generic resource_has_children() check or similar.
> 
> Jonathan

Hi Jonathan,
Thanks - agree on the explicit child check.

However, I'm reworking this Soft Reserved handling based on a chat with
Dan. New approach is to not have the CXL intersecting soft reserved
resources in iomem_resource tree. Only move them there if CXL region
assembly fails and we want to make them availabe to DAX directly.
Just wrapping my head around it.

So, both these patches go away.

Alison


> > 
> > Thanks,
> > Alison
> > 
> > > 
> > >   
> > > > +
> > > > +	rc = add_soft_reserved(parent, new_start, start - new_start, flags);
> > > > +	if (rc)
> > > > +		dev_warn(&cxlr->dev,
> > > > +			 "cannot add new soft reserved resource at %pa\n",
> > > > +			 &new_start);
> > > > +
> > > > +	rc = add_soft_reserved(parent, end + 1, new_end - end, flags);
> > > > +	if (rc)
> > > > +		dev_warn(&cxlr->dev,
> > > > +			 "cannot add new soft reserved resource at %pa + 1\n",
> > > > +			 &end);
> > > > +}
> > > > +
> > > >  static void cxl_region_iomem_release(struct cxl_region *cxlr)
> > > >  {
> > > >  	struct cxl_region_params *p = &cxlr->params;
> > > > +	struct resource *parent, *res = p->res;
> > > > +	resource_size_t start, end;
> > > >  
> > > >  	if (device_is_registered(&cxlr->dev))
> > > >  		lockdep_assert_held_write(&cxl_region_rwsem);
> > > > -	if (p->res) {
> > > > -		/*
> > > > -		 * Autodiscovered regions may not have been able to insert their
> > > > -		 * resource.
> > > > -		 */
> > > > -		if (p->res->parent)
> > > > -			remove_resource(p->res);
> > > > -		kfree(p->res);
> > > > -		p->res = NULL;
> > > > +	if (!res)
> > > > +		return;
> > > > +	/*
> > > > +	 * Autodiscovered regions may not have been able to insert their
> > > > +	 * resource. If a Soft Reserved parent resource exists, try to
> > > > +	 * remove that also.
> > > > +	 */
> > > > +	if (p->res->parent) {
> > > > +		parent = p->res->parent;
> > > > +		start = p->res->start;
> > > > +		end = p->res->end;
> > > > +		remove_resource(p->res);
> > > > +		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags) &&
> > > > +		    parent->desc == IORES_DESC_SOFT_RESERVED)
> > > > +			remove_soft_reserved(cxlr, parent, start, end);
> > > >  	}
> > > > +
> > > > +	kfree(p->res);
> > > > +	p->res = NULL;
> > > >  }
> > > >  
> > > >  static int free_hpa(struct cxl_region *cxlr)  
> > >   
>