public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Alison Schofield <alison.schofield@intel.com>
To: Gregory Price <gourry@gourry.net>
Cc: <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<kernel-team@meta.com>, <dave@stgolabs.net>,
	<jonathan.cameron@huawei.com>, <dave.jiang@intel.com>,
	<vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
	<dan.j.williams@intel.com>
Subject: Re: [PATCH 1/2] cxl/region: fix region leak when attach_target fails in cxl_add_to_region
Date: Mon, 23 Feb 2026 11:48:42 -0800	[thread overview]
Message-ID: <aZyvGnKfWI1Mku-c@aschofie-mobl2.lan> (raw)
In-Reply-To: <20260221043013.1420169-1-gourry@gourry.net>

On Fri, Feb 20, 2026 at 11:30:12PM -0500, Gregory Price wrote:
> cxl_add_to_region() ignores the return value of attach_target().  When
> attach_target() fails (e.g. cxl_port_setup_targets() returns -ENXIO),
> the auto-discovered region remains registered with its HPA resource
> consumed but never reaches COMMIT state.  Subsequent region creation
> attempts fail with -ENOSPC because the HPA range is already reserved.
> 
> Track whether this call to cxl_add_to_region() created the region, and
> call drop_region() on attach_target() failure to unregister it and
> release the HPA resource.  Pre-existing regions are left alone since
> other endpoints may already be attached.

I see you dropping this, perhaps just for the moment, because
the drop_region() you wanted to use is not available yet.

This looks a lot like 
	https://lore.kernel.org/linux-cxl/2a613604c0cdda6d9f838ae9b47ea6d936c5e4ce.1769746294.git.alison.schofield@intel.com/
	cxl/region: Unregister auto-created region when assembly fails
	When auto-created region assembly fails the region remains registered
	but disabled. The region continues to reserve its memory resource,
	preventing DAX from registering the memory.
	Unregister the region on assembly failure to release the resource.

And the review comments on that one, or at least on that thread in
general, was to leave all the broken things in place.
I didn't agree with that, and hope to see this version move ahead
when you have the drop_region you need.

-- Alison






> 
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/region.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 385be9cb44cd..276046d49f88 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3923,6 +3923,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  	struct cxl_region_context ctx;
>  	struct cxl_region_params *p;
>  	bool attach = false;
> +	bool newly_created = false;
>  	int rc;
>  
>  	ctx = (struct cxl_region_context) {
> @@ -3946,15 +3947,23 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  	mutex_lock(&cxlrd->range_lock);
>  	struct cxl_region *cxlr __free(put_cxl_region) =
>  		cxl_find_region_by_range(cxlrd, &ctx.hpa_range);
> -	if (!cxlr)
> +	if (!cxlr) {
>  		cxlr = construct_region(cxlrd, &ctx);
> +		newly_created = !IS_ERR(cxlr);
> +	}
>  	mutex_unlock(&cxlrd->range_lock);
>  
>  	rc = PTR_ERR_OR_ZERO(cxlr);
>  	if (rc)
>  		return rc;
>  
> -	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
> +	rc = attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
> +	if (rc) {
> +		/* If endpoint was just created, tear it down to release HPA */
> +		if (newly_created)
> +			drop_region(cxlrd, cxlr);
> +		return rc;
> +	}
>  
>  	scoped_guard(rwsem_read, &cxl_rwsem.region) {
>  		p = &cxlr->params;
> @@ -3972,7 +3981,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  				p->res);
>  	}
>  
> -	return rc;
> +	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
>  
> -- 
> 2.47.3
> 

  parent reply	other threads:[~2026-02-23 19:48 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-21  4:30 [PATCH 1/2] cxl/region: fix region leak when attach_target fails in cxl_add_to_region Gregory Price
2026-02-21  4:30 ` [PATCH 2/2] cxl/region: skip default driver attach for memdev with attach callbacks Gregory Price
2026-02-21  5:17 ` [PATCH 1/2] cxl/region: fix region leak when attach_target fails in cxl_add_to_region Gregory Price
2026-02-24 16:15   ` Alejandro Lucero Palau
2026-02-25  1:42     ` Gregory Price
2026-02-21 10:36 ` kernel test robot
2026-02-21 11:49 ` kernel test robot
2026-02-21 11:53 ` kernel test robot
2026-02-23 19:48 ` Alison Schofield [this message]
2026-02-23 20:15   ` Gregory Price
2026-02-24  0:18     ` Alison Schofield

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aZyvGnKfWI1Mku-c@aschofie-mobl2.lan \
    --to=alison.schofield@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=dave@stgolabs.net \
    --cc=gourry@gourry.net \
    --cc=ira.weiny@intel.com \
    --cc=jonathan.cameron@huawei.com \
    --cc=kernel-team@meta.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=vishal.l.verma@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox