Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 11:42:11 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     <alison.schofield@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>
CC:     Alison Schofield <alison.schofield@intel.com>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 3/3] cxl/core: Add sysfs attribute get_poison for list
 retrieval
Message-ID: <62accb0345de2_81c5e29454@dwillia2-xfh.notmuch>
References: <cover.1655250669.git.alison.schofield@intel.com>
 <57644934bb7af8e1c692735f53c2c415a1ba16d1.1655250669.git.alison.schofield@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <57644934bb7af8e1c692735f53c2c415a1ba16d1.1655250669.git.alison.schofield@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?TMhjmHVhUKpWtuShP0+NVWlcdtOzIg4FY8k33vnHVEUL1mEOjwBMzds+FjVh?=
 =?us-ascii?Q?tEkB3ryOBlCxujMh+Pl5vnZaPoUELvVL6zc2RI3FklA/p9DwSXl0PYW4bTrT?=
 =?us-ascii?Q?dz4z/iqhkOIdQQRoui4mJaLZJCdlDbbufIhm00wrpquTltPrUqY2CyCVUNL7?=
 =?us-ascii?Q?+iMwMjf24uKBol0rc92kNoQGuEam82KBye+sm+hQQzmS1cGklHHRE6EImIKx?=
 =?us-ascii?Q?7qe6PW9hKJBV4RcW4ytNa6Ip3uBZN8DJlN/wXKGammn4Tv1+OVrVONQdtPPI?=
 =?us-ascii?Q?GyeX1pHbmGZeUm3kvlyQIA+JyrQKpyEMS9d+Kt9k+eu7A34YsjBowoN2OWZK?=
 =?us-ascii?Q?nn0Z7OV2h8Zc+CUtVifEQDUVnsAWQ8TI2r4tOfYopEgzsQAca081L1Y4/mXb?=
 =?us-ascii?Q?+z/xjy0en8PnK2N2tikSTgXzaeCm+Z8sFOvVTgU8aDbCxdsz61bkdQgWqNDr?=
 =?us-ascii?Q?Sj1F64M2FS1DG+0Kd9ie6Ft0IetmATE+98D6pBuN79MqCcjqeuucEA9+QyeF?=
 =?us-ascii?Q?W2vL6ERIXgcgYflhwhwRkMTM6WCLy1u7KvTehpIoMlxJNjVSFvvFK+yzDjHb?=
 =?us-ascii?Q?5qy4fi9UKEbZ6drMtwrftPZWBidyaYlCjn/+RUkGK0RYoGrPQ5aBt8XWpHxj?=
 =?us-ascii?Q?xy8GJ9vqH+50jhGWfFbu84Ivjdwj2Ff1XFMt0eoHYMEyzHz16t4PgixhSkER?=
 =?us-ascii?Q?+l/UcSZEc69mh1tvQIorKEXd5ry4Ek7y0WKLYge8gDVZkT9jX7EjofNf1m+k?=
 =?us-ascii?Q?6GpstjleL+Uj+JA/To7zEJc9yYPazKqDYY7lBuPYUUqALYF/5yD+S7zAm27w?=
 =?us-ascii?Q?qtinaMckaEd9+Bzb1y4Pm9bRS0i7PSHkd/26+V+E5I4jgYGU975NTKbJYb5L?=
 =?us-ascii?Q?6zEQ7MlMf8tkDpm3fJowCHZjjzkmum75kmYZDiNln29DRr9hoHIod6d/ySo1?=
 =?us-ascii?Q?vpnDy3QWqeV1ikbGrJT/qNdJESYJp7L2CKheNaZJAtslL8mhzhqcjONC5307?=
 =?us-ascii?Q?IvqCCp9kugU1C526ghowJsymcqb1pWMtSHudQYvIu4dWQmf3cTJmTS8JS1gY?=
 =?us-ascii?Q?n0Yb/8gaezGmJ2+ytSUfYAdglE48EGf5uN14SDdm7ChOrezWxdO11PyVta9P?=
 =?us-ascii?Q?P+uHVcg7Jt0G0CpuelTxmkhjj72PPVXhti4kWVyuPpI5vhLBYINbakyUUFby?=
 =?us-ascii?Q?0FGppjY3FyuI20gaxDU+qJf86qf3/zCKx1JIf3O06TvtHHY22Ug+XphuB/IW?=
 =?us-ascii?Q?DDpeE8rUN/0eZ9QtIW6GFKKLZ3+4iH+pUr84nBHYoxlqh+DfHpgRRe6ho9t4?=
 =?us-ascii?Q?aFSnykD6aG55oqxSw/YoqI/tcHNSCsFJloLscFF+vBzIJG0fYnEfnZiHX2w0?=
 =?us-ascii?Q?I8RyWSmyvbJgX1KGEF7C9HeS+GnDv7NsCw6/WULfB/9sLvQMGtvhXNR2ZsKu?=
 =?us-ascii?Q?W9mBg0Kew3SMN5Ow7nEOmA1LzBZ6vceUhg53w56IfEMLn+37x0vfck2yKSRF?=
 =?us-ascii?Q?HTO6l5Z2PYoh33RwaVuzacjl+WMlDgPA/TKr5KN0qITZmvu+2KJVxK4EBFEE?=
 =?us-ascii?Q?uhuLp94xyljivFVvMnDZX5xjIZhD6qL3aRSk/HL3K7/hgawVqS9TH/rJdR3B?=
 =?us-ascii?Q?VHDk1v81NksZ4FYihcE42QcAzydenoYFXZLXzEwYwlKCysPpFoaMOLFKBoF2?=
 =?us-ascii?Q?Z1IzOX0b0VVvhBYUg/ed39G+PqGKeA6ZOmXZQW8Dav2YVT7O0MvLKhZs2geC?=
 =?us-ascii?Q?vVK5kZtUoetUMqR+jT4F3rBuKt8ER9E=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 72d2a96b-49b9-451a-922c-08da509117d0
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jun 2022 18:42:12.7465
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: OuL/nbPG/uPrE1Me+6GDsBjwfOK2v+B02Na4oMr+LPyr5ukfUXB6GOQJEcwjad0UIx0BI/MLKoNaoCfLFWA5LmLkkdxeY27lRXCSxSQJZYI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR11MB1576
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> The sysfs attribute, get_poison, allows user space to request the
> retrieval of a CXL devices poison list for its persistent memory.

If the device supports get poison list for volatile memory, just grab
that too. With the "to be released soon" region patches userspace can
trivially translate DPA addresses to media type.

> 
> From Documentation/ABI/.../sysfs-bus-cxl
>         (WO) When a '1' is written to this attribute the memdev
>         driver retrieves the poison list from the device. The list
>         includes 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
>         trace events with the label: cxl_poison_list.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl | 13 ++++++++++
>  drivers/cxl/core/memdev.c               | 32 +++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 7c2b846521f3..9d0c3988fdd2 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -163,3 +163,16 @@ Description:
>  		memory (type-3). The 'target_type' attribute indicates the
>  		current setting which may dynamically change based on what
>  		memory regions are activated in this decode hierarchy.
> +
> +What:		/sys/bus/cxl/devices/memX/get_poison
> +Date:		June, 2022
> +KernelVersion:	v5.20
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(WO) When a '1' is written to this attribute the memdev
> +		driver retrieves the poison list from the device. The list
> +		includes 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
> +		trace events with the label: cxl_poison_list.
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f7cdcd33504a..5ef9ffaa934a 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -106,12 +106,34 @@ static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(numa_node);
>  
> +static ssize_t get_poison_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)
> +
> +{
> +	int rc;
> +
> +	if (!sysfs_streq(buf, "1")) {

kstrtobool()?

> +		dev_err(dev, "%s: unknown value: %s\n", attr->attr.name, buf);

dev_err() is overkill for sysfs errors. dev_dbg() can be nice for errors
that trigger deep within the kernel in response to a sysfs write. In
this case EINVAL return is sufficient.

> +		return -EINVAL;
> +	}
> +
> +	rc = cxl_mem_get_poison_list(dev);
> +	if (rc) {
> +		dev_err(dev, "Failed to retrieve poison list %d\n", rc);

Too chatty, dev_dbg() or delete.

> +		return rc;
> +	}
> +	return len;
> +}
> +static DEVICE_ATTR_WO(get_poison);
> +
>  static struct attribute *cxl_memdev_attributes[] = {
>  	&dev_attr_serial.attr,
>  	&dev_attr_firmware_version.attr,
>  	&dev_attr_payload_max.attr,
>  	&dev_attr_label_storage_size.attr,
>  	&dev_attr_numa_node.attr,
> +	&dev_attr_get_poison.attr,
>  	NULL,
>  };
>  
> @@ -130,6 +152,16 @@ 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;
> +
> +	if (a == &dev_attr_get_poison.attr) {
> +		struct device *dev = container_of(kobj, struct device, kobj);

Use the kobj_to_dev() helper.

> +		struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +		if (!test_bit(CXL_MEM_COMMAND_ID_GET_POISON,
> +			      cxlds->enabled_cmds))
> +			return 0;
> +	}
>  	return a->mode;
>  }
>  
> -- 
> 2.31.1
>