Re: [PATCH v10 3/6] cxl/memdev: Add trigger_poison_list sysfs attribute

From: Alison Schofield <alison.schofield@intel.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: 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>,
	linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v10 3/6] cxl/memdev: Add trigger_poison_list sysfs attribute
Date: Fri, 31 Mar 2023 08:45:33 -0700	[thread overview]
Message-ID: <ZCcAHbnGu5c46Tg4@aschofie-mobl2> (raw)
In-Reply-To: <6e59a3d6-0b68-2d36-d25a-652167168db8@intel.com>

On Thu, Mar 30, 2023 at 03:55:46PM -0700, Dave Jiang wrote:
> 
> 
> On 3/21/23 7:12 PM, alison.schofield@intel.com 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 trace events with the label 'cxl_poison'.
> > 
> > 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(-)
> > 

snip

> > +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)

See this EFAULT.  That is why I changed table further down to
allow EFAULT to get through explicitly.

snip

> > @@ -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;
> > +
> 
> Stray blank line?

Yes.

> 
> >   	return a->mode;
> >   }
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 57a5999ddb35..5febaa3f9b04 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -145,7 +145,7 @@ struct cxl_mbox_cmd {
> >   	C(FWROLLBACK, -ENXIO, "rolled back to the previous active FW"),         \
> >   	C(FWRESET, -ENXIO, "FW failed to activate, needs cold reset"),		\
> >   	C(HANDLE, -ENXIO, "one or more Event Record Handles were invalid"),     \
> > -	C(PADDR, -ENXIO, "physical address specified is invalid"),		\
> > +	C(PADDR, -EFAULT, "physical address specified is invalid"),		\
> 
> Seems unrelated change? Does it go with previous patch?

See prior note. Need this one explicitly.

> 
snip to end.