Re: [RFC PATCH v2 19/28] cxl/region: Introduce concept of region configuration

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 22 Oct 2021 11:37:00 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> The region creation APIs create a vacant region. Configuring the region
> works in the same way as similar subsystems such as devdax. Sysfs attrs
> will be provided to allow userspace to configure the region.  Finally
> once all configuration is complete, userspace may activate the region.
> 
> Introduced here are the most basic attributes needed to configure a
> region. Details of these attribute are described in the ABI
> Documentation.
> 
> A example is provided below:
> 
> /sys/bus/cxl/devices/region0.0:0
> ├── interleave_granularity
> ├── interleave_ways
> ├── offset
> ├── size
> ├── subsystem -> ../../../../../../bus/cxl
> ├── target0
> ├── uevent
> └── uuid
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---

Not a proper review, but just one thing noticed whilst messing around with this.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 588f0ca65bb2..3b0d74d4dd6c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3,9 +3,12 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/sizes.h>
>  #include <linux/slab.h>
> +#include <linux/uuid.h>
>  #include <linux/idr.h>
>  #include <region.h>
> +#include <cxlmem.h>
>  #include <cxl.h>
>  
> +
> +static size_t set_targetN(struct cxl_region *region, const char *buf, int n,
> +			  size_t len)
> +{
> +	struct device *memdev_dev;
> +	struct cxl_memdev *cxlmd;
> +
> +	device_lock(&region->dev);
> +
> +	if (len == 1 || region->targets[n])
> +		remove_target(region, n);
> +
> +	/* Remove target special case */
> +	if (len == 1) {
> +		device_unlock(&region->dev);
> +		return len;
> +	}
> +
> +	memdev_dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf);
> +	if (!memdev_dev)

device_unlock()?

> +		return -ENOENT;
> +
> +	/* reference to memdev held until target is unset or region goes away */
> +
> +	cxlmd = to_cxl_memdev(memdev_dev);
> +	region->targets[n] = cxlmd;
> +
> +	device_unlock(&region->dev);
> +
> +	return len;
> +}

J