linux-kernel - Re: [PATCH 3/3] cxl/core: Add sysfs attribute get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220616203949.GB1529208@alison-desk>
Date:   Thu, 16 Jun 2022 13:39:49 -0700
From:   Alison Schofield <alison.schofield@...el.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     "Williams, Dan J" <dan.j.williams@...el.com>,
        "Weiny, Ira" <ira.weiny@...el.com>,
        "Verma, Vishal L" <vishal.l.verma@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] cxl/core: Add sysfs attribute get_poison for list
 retrieval

On Thu, Jun 16, 2022 at 08:04:12AM -0700, Jonathan Cameron wrote:
> On Tue, 14 Jun 2022 17:10:28 -0700
> alison.schofield@...el.com wrote:
> 
> > From: Alison Schofield <alison.schofield@...el.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@...el.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!