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

[Dan Williams <dan.j.williams@intel.com>]

Alison Schofield wrote:
[..]
> > > +	bool trigger;
> > > +	ssize_t rc;
> > > +
> > > +	if (kstrtobool(buf, &trigger) || !trigger)
> > > +		return -EINVAL;
> > 
> > Hmm, the caller could to this too...
> 
> Why split the work?
> 
> It seems having the caller do a little bit of work, and then pass it on,
> hurts readability. ATM, the caller does nothing, and here we do all
> the usual sysfs handling. The division of power is crisp. If it's not
> an efficiency concern, why make it less readable?

Ah interesting, I am coming at it from a Separation of Concerns
perspective.

The core functionality being exported is a method to retrieve poison
records and emit them as trace-events. Whether that is in response to
sysfs stimulus or some other stimulus is not something the core needs to
care about. An export of cxl_get_poison_by_memdev() is an API that can
have multiple users, cxl_trigger_poison_list() is an API that is
purpose-built for one user. The motivation is to have the core API be
potentially useful for multiple contexts.

One thought experiement is write a kernel-doc comment for
cxl_trigger_poison_list(). It would need to say something like "this API
is only for the cxl_mem driver to use for its sysfs attribute", while
the kernel-doc for cxl_get_poison_by_memdev() can remain generic.

So the general ask here is create new CXL core exports that can be read
and understood independent of their callers.

> > ...the below seems to be the bit that the cxl_core cares about handling.
> > 
> > > +
> > > +	down_read(&cxl_dpa_rwsem);
> > 
> > down_read_interruptible() since this is coming from userspace.
> 
> Thanks, got it!
> 
> > 
> > > +	rc = cxl_get_poison_by_memdev(cxlmd);
> > > +	up_read(&cxl_dpa_rwsem);
> > > +
> > > +	return rc ? rc : len;
> > 
> > The caller can do this conversion.
> > 
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(cxl_trigger_poison_list, CXL);
> > > +
> > >  static struct attribute *cxl_memdev_attributes[] = {
> > >  	&dev_attr_serial.attr,
> > >  	&dev_attr_firmware_version.attr,
> > > @@ -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;
> > > +
> > 
> > I am surprised that Jonathan let this slide :).
> 
> He caught it, so did DaveJ. It just won't go away.

I feel your pain, these things are sneaky.