linux-kernel - Re: [PATCH v1 08/20] cxl/region: Rework memregion id allocation and move it to register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250721150613.315209-1-joshua.hahnjy@gmail.com>
Date: Mon, 21 Jul 2025 08:06:11 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Robert Richter <rrichter@....com>
Cc: Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Ira Weiny <ira.weiny@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Gregory Price <gourry@...rry.net>,
	"Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>,
	Terry Bowman <terry.bowman@....com>
Subject: Re: [PATCH v1 08/20] cxl/region: Rework memregion id allocation and move it to register_region()

On Tue, 15 Jul 2025 21:11:31 +0200 Robert Richter <rrichter@....com> wrote:

> Make the atomic_cmpxchg() loop an inner loop of register_region().
> Simplify calling of __create_region() by modifying the @port_id
> function argument to accept a value of -1 to indicates that an
> available memregion id should be assigned to the region.
> 
> Signed-off-by: Robert Richter <rrichter@....com>
> ---

Hello Robert,

Thank you again for this patchset!

>  drivers/cxl/core/region.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 57ee758bdece..34ffd726859e 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2504,18 +2504,34 @@ static int register_region(struct cxl_region *cxlr, int id)
>  {
>  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
>  	struct device *dev = &cxlr->dev;
> -	int rc;
> +	int old, match, rc;
>  
>  	rc = memregion_alloc(GFP_KERNEL);
>  	if (rc < 0)
>  		return rc;
>  
> -	if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
> +	if (id < 0)
> +		match = atomic_read(&cxlrd->region_id);
> +	else
> +		match = id;
> +
> +	for (; match >= 0;) {

Is there a reason we use a for loop with no initialization or update step?
(would a while loop suffice?)

> +		old = atomic_cmpxchg(&cxlrd->region_id, match, rc);
> +		if (old == match)
> +			break;
> +		if (id >= 0)
> +			break;
> +		match = old;
> +	}

Also, if I understand correctly, there seem to be 2 ways this loop is used.
There's the loop for when id < 0, in which case the loop will iterate at most
twice, and the loop for when id >= 0, in which case the loop will always
iterate once. The two break statements also seem to be unique to each use case.

Would it make sense to avoid using the while loop? I think that the following
code achieves the same functionality as the code above (untested), but
does not create an illusion of being able to be stuck in an infinite loop.

match = id < 0 ? atomic_read(&cxlrd->region_id) : id;

old = atomic_cmpxchg(&cxlrd->region_id, match, rc);
if (id < 0 && match != old)  {
	match = atomic_cmpxchg(&cxlrd->region_id, old, rc);
}


Of course, please let me know if this is incorrect, or you feel that this is
even more difficult to read : -) I think that the error handling below
should also behave the same way as before.

> +
> +	if (match < 0 || match != old) {
>  		memregion_free(rc);
> +		if (match < 0)
> +			return -ENXIO;
>  		return -EBUSY;
>  	}
>  
> -	cxlr->id = id;
> +	cxlr->id = old;
>  
>  	rc = dev_set_name(dev, "region%d", cxlr->id);
>  	if (rc)

[...snip...]

Thank you again for the patch! Have a great day!
Joshua

Sent using hkml (https://github.com/sjp38/hackermail)