Re: [RFC PATCH 2/4] cxl/region: Create attribute structure / verify

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

On Thu, 10 Jun 2021 11:57:23 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Introduce a verification mechanism for a region. Regions have complex
> configuration requirements and it is beneficial to provide a way to
> verify the constraints are met before trying to bind. Primarily it adds
> ABI to inform userspace of more detailed information about what failed
> rather than the limited choices of errno at bind time.
> 
> It's important to point out that a verified region can still fail to
> bind, but the first step in binding will be to run the same verification
> algorithm.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> 
> --
> 
> Functionally it might make sense to squash this patch in with other
> patches adding attributes. From a discussion standpoint however, it's
> nice to have this broken out as I suspect there might be some debate
> about it.

Hmm. Definitely squash it in later, as this is downright odd at the moment!

Is there precedence elsewhere for this interface approach?
I can see the advantage of it as it lets us pass through invalid states
whilst configuring but it is somewhat unusual.

Probably one for linux-api@vger.kernel.org to get more exposure to people
who care about this stuff.  I suspect there aren't that many people
on linux-cxl yet ;)

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 13 +++++++++++++
>  drivers/cxl/region.c                    | 22 +++++++++++++++++++++-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 5bcbefd4ea38..699c8514fd7b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -146,3 +146,16 @@ Contact:	linux-cxl@vger.kernel.org
>  Description:
>  		Deletes the named region. A region must be unbound from the
>  		region driver before being deleted.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/verify
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:

Doing 'what' to this file causes to this to happen?
You want to state that "Reading this file instructs..."

> +		Instructs the kernel to verify that the regionX.Y:Z is properly
> +		configured and provide more detailed information about
> +		configuration errors. A value of 0 indicates the region is
> +		properly configured and ready to bind, otherwise a negative
> +		integer is returned describing the first error found in the
> +		configuration. A verified region can still fail binding due to
> +		lack of resources.
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index 1f47bc17bd50..ea1ac848c713 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -20,11 +20,31 @@
>   * relationship between decoder and region when the region is interleaved.
>   */
>  
> -static void cxl_region_release(struct device *dev);
> +static ssize_t verify_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "0");
> +}
> +
> +static DEVICE_ATTR_RO(verify);
> +
> +static struct attribute *region_attrs[] = {
> +	&dev_attr_verify.attr,
> +	NULL,
> +};
>  
> +static const struct attribute_group region_group = {
> +	.attrs = region_attrs,
> +};
> +
> +static const struct attribute_group *region_groups[] = {
> +	&region_group,
> +};
> +
> +static void cxl_region_release(struct device *dev);
>  static const struct device_type cxl_region_type = {
>  	.name = "cxl_region",
>  	.release = cxl_region_release,
> +	.groups = region_groups,
>  };
>  
>  static struct cxl_region *to_cxl_region(struct device *dev)