Alison Schofield wrote:
> On Mon, Mar 18, 2024 at 10:51:13AM -0700, fan wrote:
> > On Wed, Mar 13, 2024 at 09:05:17PM -0700, alison.schofi...@intel.com wrote:
> > > From: Alison Schofield <alison.schofi...@intel.com>
> > >
> > > CXL devices maintain a list of locations that are poisoned or result
> > > in poison if the addresses are accessed by the host.
> > >
> > > Per the spec (CXL 3.1 8.2.9.9.4.1), the device returns the Poison
> > > List as a set of Media Error Records that include the source of the
> > > error, the starting device physical address and length.
> > >
> > > Trigger the retrieval of the poison list by writing to the memory
> > > device sysfs attribute: trigger_poison_list. The CXL driver only
> > > offers triggering per memdev, so the trigger by region interface
> > > offered here is a convenience API that triggers a poison list
> > > retrieval for each memdev contributing to a region.
> > >
> > > int cxl_memdev_trigger_poison_list(struct cxl_memdev *memdev);
> > > int cxl_region_trigger_poison_list(struct cxl_region *region);
> > >
> > > The resulting poison records are logged as kernel trace events
> > > named 'cxl_poison'.
> > >
> > > Signed-off-by: Alison Schofield <alison.schofi...@intel.com>
> > > Reviewed-by: Dave Jiang <dave.ji...@intel.com>
> > > ---
> > > cxl/lib/libcxl.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> > > cxl/lib/libcxl.sym | 2 ++
> > > cxl/libcxl.h | 2 ++
> > > 3 files changed, 51 insertions(+)
> > >
> > > diff --git a/cxl/lib/libcxl.c b/cxl/lib/libcxl.c
> > > index ff27cdf7c44a..73db8f15c704 100644
> > > --- a/cxl/lib/libcxl.c
> > > +++ b/cxl/lib/libcxl.c
> > > @@ -1761,6 +1761,53 @@ CXL_EXPORT int
> > > cxl_memdev_disable_invalidate(struct cxl_memdev *memdev)
> > > return 0;
> > > }
> > >
> > > +CXL_EXPORT int cxl_memdev_trigger_poison_list(struct cxl_memdev *memdev)
> > > +{
> > > + struct cxl_ctx *ctx = cxl_memdev_get_ctx(memdev);
> > > + char *path = memdev->dev_buf;
> > > + int len = memdev->buf_len, rc;
> > > +
> > > + if (snprintf(path, len, "%s/trigger_poison_list",
> > > + memdev->dev_path) >= len) {
> > > + err(ctx, "%s: buffer too small\n",
> > > + cxl_memdev_get_devname(memdev));
> > > + return -ENXIO;
> > > + }
> > > + rc = sysfs_write_attr(ctx, path, "1\n");
> > > + if (rc < 0) {
> > > + fprintf(stderr,
> > > + "%s: Failed write sysfs attr trigger_poison_list\n",
> > > + cxl_memdev_get_devname(memdev));
> >
> > Should we use err() instead of fprintf here?
>
> Thanks Fan,
>
> How about this?
>
> - use fprintf if access() fails, ie device doesn't support poison list,
> - use err() for failure to actually read the poison list on a device with
> support
Why? There is no raw usage of fprintf in any of the libraries (ndctl,
daxctl, cxl) to date. If someone builds the library without logging then
it should not chat on stderr at all, and if someone redirects logging to
syslog then it also should emit messages only there and not stderr.
