Re: [PATCH] cxl: Add committed sysfs attribute to CXL decoder

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

On Mon, 25 Sep 2023 11:05:56 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Expose the committed status of a CXL decoder. The status assists CXL CLI in
> determining as a sure way whether a region is active. Of coruse this is

Spell check.

> still best effort as it's a snapshot of the state and not atomic for the
> user region disable operation.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
One passing comment inline.

Seems reasonable to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |    7 +++++++
>  drivers/cxl/core/port.c                 |   11 +++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 087f762ebfd5..ef3fc9fe9d0d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -369,6 +369,13 @@ Description:
>  		provided it is currently idle / not bound to a driver.
>  
>  
> +What:		/sys/bus/cxl/devices/decoderX.Y/committed
> +Date:		Sep, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) Indicates whether the decoder is committed.
> +
>  What:		/sys/bus/cxl/devices/regionZ/uuid
>  Date:		May, 2022
>  KernelVersion:	v6.0
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 724be8448eb4..ab5b9bc26d56 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -277,12 +277,23 @@ static ssize_t interleave_ways_show(struct device *dev,
>  
>  static DEVICE_ATTR_RO(interleave_ways);
>  
> +static ssize_t committed_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> +
> +	return sysfs_emit(buf, "%d\n", !!(cxld->flags & CXL_DECODER_F_ENABLE));

Could use FIELD_GET() though as it's not been used for these
before that I could quickly find, fair enough to maintain
consistent style

> +}
> +
> +static DEVICE_ATTR_RO(committed);
> +
>  static struct attribute *cxl_decoder_base_attrs[] = {
>  	&dev_attr_start.attr,
>  	&dev_attr_size.attr,
>  	&dev_attr_locked.attr,
>  	&dev_attr_interleave_granularity.attr,
>  	&dev_attr_interleave_ways.attr,
> +	&dev_attr_committed.attr,
>  	NULL,
>  };
>  
> 
>