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:   Thu, 16 Jun 2022 13:39:49 -0700
From:   Alison Schofield <alison.schofield@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc:     "Williams, Dan J" <dan.j.williams@intel.com>,
        "Weiny, Ira" <ira.weiny@intel.com>,
        "Verma, Vishal L" <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        "linux-kernel@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: <20220616203949.GB1529208@alison-desk>
References: <cover.1655250669.git.alison.schofield@intel.com>
 <57644934bb7af8e1c692735f53c2c415a1ba16d1.1655250669.git.alison.schofield@intel.com>
 <20220616160412.00000c34@Huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220616160412.00000c34@Huawei.com>
Precedence: bulk

On Thu, Jun 16, 2022 at 08:04:12AM -0700, Jonathan Cameron wrote:
> On Tue, 14 Jun 2022 17:10:28 -0700
> alison.schofield@intel.com 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.
> > 
> > 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>
> 
> Hi Alison,
> 
> I'm planning to throw together QEMU support for this and test
> it. In meantime a few quick comments / suggestions inline.

Thanks Jonathan.
I've tested with a test patch that returns contrived output payloads,
and will look fwd to trying out w qemu,

> 
> Thanks,
> 
> Jonathan
> 
> > ---
snip
> > +
> > +	if (!sysfs_streq(buf, "1")) {
> 
> Maybe kstrtobool?  If you do then fine to leave the documentation claiming
> it's tighter as that'll tell people who actually read it to expect to
> write a 1.
> 
Got it.

> > +		dev_err(dev, "%s: unknown value: %s\n", attr->attr.name, buf);
> 
> Feels noisy when I'd expect -EINVAL to be enough info to indicate an invalid
> parameter.
> 
Got it.

> > +		return -EINVAL;
> > +	}
> > +
> > +	rc = cxl_mem_get_poison_list(dev);
> > +	if (rc) {
> > +		dev_err(dev, "Failed to retrieve poison list %d\n", rc);
> 
> Here I'd expect the error code to returned on the write to probably be enough
> info so not sure this error print is useful either.
> 
Got it.

> > +
snip
> > +	if (a == &dev_attr_get_poison.attr) {
> > +		struct device *dev = container_of(kobj, struct device, kobj);
> > +		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))
> 			      to_cxl_memdev(dev)->enabled_cmds))
> and drop the local variable is shorter and I don't htink it loses
> any readability.
> 
Got it.

Thanks Jonathan!