RE: [PATCH v12 3/6] cxl/memdev: Add trigger_poison_list sysfs attribute

[Dan Williams <dan.j.williams@intel.com>]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> When a boolean 'true' is written to this attribute the memdev driver
> retrieves the poison list from the device. The list consists of
> addresses that are poisoned, or would result in poison if accessed,
> and the source of the poison. This attribute is only visible for
> devices supporting the capability. The retrieved errors are logged
> as kernel events when cxl_poison event tracing is enabled.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 14 ++++++++
>  drivers/cxl/core/memdev.c               | 48 +++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h                    |  5 ++-
>  drivers/cxl/mem.c                       | 36 +++++++++++++++++++
>  4 files changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 3acf2f17a73f..48ac0d911801 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -415,3 +415,17 @@ Description:
>  		1), and checks that the hardware accepts the commit request.
>  		Reading this value indicates whether the region is committed or
>  		not.
> +
> +
> +What:		/sys/bus/cxl/devices/memX/trigger_poison_list
> +Date:		April, 2023
> +KernelVersion:	v6.4
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(WO) When a boolean 'true' is written to this attribute the
> +		memdev driver retrieves the poison list from the device. The
> +		list consists of addresses that are poisoned, or would result
> +		in poison if accessed, and the source of the poison. This
> +		attribute is only visible for devices supporting the
> +		capability. The retrieved errors are logged as kernel
> +		events when cxl_poison event tracing is enabled.
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 0af8856936dc..297d87ebaca6 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -106,6 +106,53 @@ static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(numa_node);
>  
> +static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	u64 offset, length;
> +	int rc = 0;
> +
> +	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
> +	if (resource_size(&cxlds->pmem_res)) {
> +		offset = cxlds->pmem_res.start;
> +		length = resource_size(&cxlds->pmem_res);
> +		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> +		if (rc)
> +			return rc;
> +	}
> +	if (resource_size(&cxlds->ram_res)) {
> +		offset = cxlds->ram_res.start;
> +		length = resource_size(&cxlds->ram_res);
> +		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> +		/*
> +		 * Invalid Physical Address is not an error for
> +		 * volatile addresses. Device support is optional.
> +		 */
> +		if (rc == -EFAULT)
> +			rc = 0;
> +	}
> +	return rc;
> +}
> +
> +ssize_t cxl_trigger_poison_list(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)

The @attr argument is unused, it can be dropped.

> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);

The caller can do this conversion.

> +	bool trigger;
> +	ssize_t rc;
> +
> +	if (kstrtobool(buf, &trigger) || !trigger)
> +		return -EINVAL;

Hmm, the caller could to this too...

...the below seems to be the bit that the cxl_core cares about handling.

> +
> +	down_read(&cxl_dpa_rwsem);

down_read_interruptible() since this is coming from userspace.

> +	rc = cxl_get_poison_by_memdev(cxlmd);
> +	up_read(&cxl_dpa_rwsem);
> +
> +	return rc ? rc : len;

The caller can do this conversion.

> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_trigger_poison_list, CXL);
> +
>  static struct attribute *cxl_memdev_attributes[] = {
>  	&dev_attr_serial.attr,
>  	&dev_attr_firmware_version.attr,
> @@ -130,6 +177,7 @@ static umode_t cxl_memdev_visible(struct kobject *kobj, struct attribute *a,
>  {
>  	if (!IS_ENABLED(CONFIG_NUMA) && a == &dev_attr_numa_node.attr)
>  		return 0;
> +

I am surprised that Jonathan let this slide :).

>  	return a->mode;
>  }
>