Re: [PATCH v2 5/8] cxl/region: Split commit_store() into __commit() and queue_reset() helpers

[Alison Schofield <alison.schofield@intel.com>]

On Wed, Jun 18, 2025 at 10:04:13PM -0700, Dan Williams wrote:
> The complexity of dropping the lock is removed in favor of splitting commit
> operations to a helper, and leaving all the complexities of "decommit" for
> commit_store() to coordinate the different locking contexts.
> 
> The CPU cache-invalidation in the decommit path is solely handled now by
> cxl_region_decode_reset(). Previously the CPU caches were being needlessly
> flushed twice in the decommit path where the first flush had no guarantee
> that the memory would not be immediately re-dirtied.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c | 99 +++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e5e1460068d..3a77aec2c447 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -349,30 +349,42 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> -static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
> -			    const char *buf, size_t len)
> +static int queue_reset(struct cxl_region *cxlr)

How about defining both new helpers to return ssize_t type for consistency
with sysfs functions. The local 'int rc' can remain as long as the function
return type is ssize_t.

There no possible breakage here based on the actual limited return values.
Just a suggestion to follow sysfs convention.

>  {
> -	struct cxl_region *cxlr = to_cxl_region(dev);
>  	struct cxl_region_params *p = &cxlr->params;
> -	bool commit;
> -	ssize_t rc;
> +	int rc;
>  
> -	rc = kstrtobool(buf, &commit);
> +	rc = down_write_killable(&cxl_region_rwsem);
>  	if (rc)
>  		return rc;
>  
> +	/* Already in the requested state? */
> +	if (p->state < CXL_CONFIG_COMMIT)
> +		goto out;
> +
> +	p->state = CXL_CONFIG_RESET_PENDING;
> +
> +out:
> +	up_write(&cxl_region_rwsem);
> +
> +	return rc;
> +}

snip