Re: [PATCH v7 1/5] counter: Internalize sysfs interface code

[William Breathitt Gray]

On Wed, Dec 30, 2020 at 02:37:19PM +0000, Jonathan Cameron wrote:
> On Fri, 25 Dec 2020 19:15:34 -0500
> William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote:
> 
> > This is a reimplementation of the Generic Counter driver interface.
> > There are no modifications to the Counter subsystem userspace interface,
> > so existing userspace applications should continue to run seamlessly.
> Hi William
> 
> Been a while since I looked at this series.  Its rather big and I'm lazy
> (or busy depending on who I'm talking to :)
> 
> Hmm. I'm a bit in two minds about how you should handle the huge amount of
> description here.  Some of it clearly belongs in the kernel docs (and some
> is I think put there in a later patch).  Other parts are specific to
> this series, but I'm not 100% sure this much detail is really useful in the
> git log.   Note that we now have links to the threads for all patches applied
> using b4 (which this will be) so it's fine to have really detailed stuff
> in cover letters rather than the individual patches.

I'll simplify the description here to something more succinct.

> One thing that would be handy for review, might be if you put up a tree
> somewhere with this applied and included a link.

This is such a large set of changes that having a tree to checkout for
review would be convenient. I typically push to my personal tree, so you
can check out the changes there in the counter_chrdev_v* branches:
https://gitlab.com/vilhelmgray/iio

I'll include a link to it again in the cover letter for v8 when it's
ready.

> Mind you I don't feel that strongly about it if it you do want to maintain
> it in the individual patch descriptions.
> 
> I've been a bit lazy and not cropped this down as much as I ideally should
> have done (to include only bits I'm commenting on).
> 
> Anyhow, various minor things inline but this fundamentally looks fine to me.
> 
> Jonathan
> 
> 
> > 
> > Overview
> > ========
> > 
> > The purpose of this patch is to internalize the sysfs interface code
> > among the various counter drivers into a shared module. Counter drivers
> > pass and take data natively (i.e. u8, u64, etc.) and the shared counter
> > module handles the translation between the sysfs interface.
> 
> Confusing statement.  Between the sysfs interface and what?
> Perhaps "handles the translation to/from the sysfs interface."

Looks like I cut that line short by accident; it should read: "between
the sysfs interface and the device drivers". I'll fix this up.

> > This
> > guarantees a standard userspace interface for all counter drivers, and
> > helps generalize the Generic Counter driver ABI in order to support the
> > Generic Counter chrdev interface (introduced in a subsequent patch)
> > without significant changes to the existing counter drivers.
> > 
> > A high-level view of how a count value is passed down from a counter
> > driver is exemplified by the following. The driver callbacks are first
> > registered to the Counter core component for use by the Counter
> > userspace interface components:
> > 
> >                         +----------------------------+
> > 	                | Counter device driver      |
> 
> Looks like something snuck a tab in amongst your spaces.

Ack.

> >  static int quad8_signal_read(struct counter_device *counter,
> > -	struct counter_signal *signal, enum counter_signal_value *val)
> > +			     struct counter_signal *signal,
> > +			     enum counter_signal_level *level)
> >  {
> >  	const struct quad8_iio *const priv = counter->priv;
> >  	unsigned int state;
> > @@ -633,13 +634,13 @@ static int quad8_signal_read(struct counter_device *counter,
> >  	state = inb(priv->base + QUAD8_REG_INDEX_INPUT_LEVELS)
> >  		& BIT(signal->id - 16);
> >  
> > -	*val = (state) ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
> > +	*level = (state) ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;
> 
> This bit of refactoring / renaming could have been split out as a precursor patch
> I think.  There may be other opportunities. 

Ack. I'll look around for additional changes I can pull out as precursor
patches too.

> >  
> >  	return 0;
> >  }
> >  
> >  static int quad8_count_read(struct counter_device *counter,
> > -	struct counter_count *count, unsigned long *val)
> > +			    struct counter_count *count, u64 *val)
> 
> Could the type change for val have been done as a precursor?

I don't think we can pull this one out as a precursor unfortunately.
Since unsigned long is passed in as pointer, we could get a type
mismatch if we're on a 32-bit system. For this to work, it requires the
u64 change in struct counter_ops and subsequent dependent code, so we'll
have to keep it as part of this patch for now.

> > @@ -785,18 +782,21 @@ static int quad8_function_set(struct counter_device *counter,
> >  		*quadrature_mode = 1;
> >  
> >  		switch (function) {
> > -		case QUAD8_COUNT_FUNCTION_QUADRATURE_X1:
> > +		case COUNTER_FUNCTION_QUADRATURE_X1_A:
> >  			*scale = 0;
> >  			mode_cfg |= QUAD8_CMR_QUADRATURE_X1;
> >  			break;
> > -		case QUAD8_COUNT_FUNCTION_QUADRATURE_X2:
> > +		case COUNTER_FUNCTION_QUADRATURE_X2_A:
> >  			*scale = 1;
> >  			mode_cfg |= QUAD8_CMR_QUADRATURE_X2;
> >  			break;
> > -		case QUAD8_COUNT_FUNCTION_QUADRATURE_X4:
> > +		case COUNTER_FUNCTION_QUADRATURE_X4:
> >  			*scale = 2;
> >  			mode_cfg |= QUAD8_CMR_QUADRATURE_X4;
> >  			break;
> > +		default:
> > +			mutex_unlock(&priv->lock);
> > +			return -EINVAL;
> 
> This looks like a sensible precaution / cleanup but could have been
> done separately to the rest of the patch I think?

Ack.

> > @@ -1229,30 +1194,28 @@ static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
> >  
> >  	mutex_unlock(&priv->lock);
> >  
> > -	return len;
> > +	return -EINVAL;
> 
> ?  That looks like the good exit path to me.

You're right, this should be a return 0.

> > +/**
> > + * counter_register - register Counter to the system
> > + * @counter:	pointer to Counter to register
> > + *
> > + * This function registers a Counter to the system. A sysfs "counter" directory
> > + * will be created and populated with sysfs attributes correlating with the
> > + * Counter Signals, Synapses, and Counts respectively.
> 
> Where easy to do it's worth documenting return values.

Ack.

> > +static void devm_counter_unregister(struct device *dev, void *res)
> > +{
> > +	counter_unregister(*(struct counter_device **)res);
> 
> Rename this. It looks like it's a generic way of unwinding
> devm_counter_register which it is definitely not...

Ack.

> > +/**
> > + * struct counter_attribute - Counter sysfs attribute
> > + * @dev_attr:	device attribute for sysfs
> > + * @l:		node to add Counter attribute to attribute group list
> > + * @comp:	Counter component callbacks and data
> > + * @scope:	Counter scope of the attribute
> > + * @parent:	pointer to the parent component
> > + */
> > +struct counter_attribute {
> > +	struct device_attribute dev_attr;
> > +	struct list_head l;
> > +
> > +	struct counter_comp comp;
> > +	__u8 scope;
> 
> Why not an enum?

This should be enum; I missed it from the previous revision.

> > +	switch (a->comp.type) {
> > +	case COUNTER_COMP_FUNCTION:
> > +		return sprintf(buf, "%s\n", counter_function_str[data]);
> > +	case COUNTER_COMP_SIGNAL_LEVEL:
> > +		return sprintf(buf, "%s\n", counter_signal_value_str[data]);
> > +	case COUNTER_COMP_SYNAPSE_ACTION:
> > +		return sprintf(buf, "%s\n", counter_synapse_action_str[data]);
> > +	case COUNTER_COMP_ENUM:
> > +		return sprintf(buf, "%s\n", avail->strs[data]);
> > +	case COUNTER_COMP_COUNT_DIRECTION:
> > +		return sprintf(buf, "%s\n", counter_count_direction_str[data]);
> > +	case COUNTER_COMP_COUNT_MODE:
> > +		return sprintf(buf, "%s\n", counter_count_mode_str[data]);
> > +	default:
> 
> Perhaps move the below return sprintf() up here?

Ack.

> > +		break;
> > +	}
> > +
> > +	return sprintf(buf, "%u\n", (unsigned int)data);
> > +}
> > +
> > +static int find_in_string_array(u32 *const enum_item, const u32 *const enums,
> > +				const size_t num_enums, const char *const buf,
> > +				const char *const string_array[])
> 
> Please avoid defining such generically named functions.  High chance of a clash
> with something that turns up in generic headers sometime in the future.

Ack.

> > +static ssize_t enums_available_show(const u32 *const enums,
> > +				    const size_t num_enums,
> > +				    const char *const strs[], char *buf)
> > +{
> > +	size_t len = 0;
> > +	size_t index;
> > +
> > +	for (index = 0; index < num_enums; index++)
> > +		len += sprintf(buf + len, "%s\n", strs[enums[index]]);
> 
> Probably better to add protections on overrunning the buffer to this.
> Sure it won't actually happen but that may not be obvious to someone reading
> this code in future.
> 
> Look at new sysfs_emit * family for this.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2efc459d06f1630001e3984854848a5647086232

Ack.

> > +static ssize_t counter_comp_available_show(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   char *buf)
> > +{
> > +	const struct counter_attribute *const a = to_counter_attribute(attr);
> > +	const struct counter_count *const count = a->parent;
> > +	const struct counter_synapse *const synapse = a->comp.priv;
> > +	const struct counter_available *const avail = a->comp.priv;
> > +
> > +	switch (a->comp.type) {
> > +	case COUNTER_COMP_FUNCTION:
> > +		return enums_available_show(count->functions_list,
> > +					    count->num_functions,
> > +					    counter_function_str, buf);
> > +	case COUNTER_COMP_SYNAPSE_ACTION:
> > +		return enums_available_show(synapse->actions_list,
> > +					    synapse->num_actions,
> > +					    counter_synapse_action_str, buf);
> > +	case COUNTER_COMP_ENUM:
> > +		return strs_available_show(avail, buf);
> > +	case COUNTER_COMP_COUNT_MODE:
> > +		return enums_available_show(avail->enums, avail->num_items,
> > +					    counter_count_mode_str, buf);
> > +	default:
> > +		break;
> 
> Might as well return -EINVAL; here

Ack.

> > +	/* Store list node */
> > +	list_add(&counter_attr->l, &group->attr_list);
> > +	group->num_attr++;
> > +
> > +	switch (comp->type) {
> > +	case COUNTER_COMP_FUNCTION:
> > +	case COUNTER_COMP_SYNAPSE_ACTION:
> > +	case COUNTER_COMP_ENUM:
> > +	case COUNTER_COMP_COUNT_MODE:
> > +		return counter_avail_attr_create(dev, group, comp, parent);
> > +	default:
> > +		break;
> 
> return 0 in here.  Also add a comment on why it isn't an error.

Ack.

> > +static int counter_sysfs_synapses_add(struct counter_device *const counter,
> > +	struct counter_attribute_group *const group,
> > +	struct counter_count *const count)
> > +{
> > +	const __u8 scope = COUNTER_SCOPE_COUNT;
> > +	struct device *const dev = &counter->dev;
> > +	size_t i;
> > +	struct counter_synapse *synapse;
> > +	size_t id;
> > +	struct counter_comp comp;
> > +	int err;
> > +
> > +	/* Add each Synapse */
> > +	for (i = 0; i < count->num_synapses; i++) {
> Could reduce scope and make code a bit more readable by
> pulling
> 
> struct counter_synapse *synapse;
> struct counter_comp comp;
> size_t id;
> 
> and maybe other things in here.  Makes it clear their scope
> is just within this loop.

Ack.

> >  /**
> >   * struct counter_synapse - Counter Synapse node
> > - * @action:		index of current action mode
> >   * @actions_list:	array of available action modes
> >   * @num_actions:	number of action modes specified in @actions_list
> > - * @signal:		pointer to associated signal
> > + * @signal:		pointer to the associated Signal
> 
> Might have been nice to pull the cases that were purely capitalization out as
> a separate patch immediately following this one. There aren't
> a huge number, but from a review point of view it's a noop patch
> so doesn't need the reviewer to be awake :)

Ack.

William Breathitt Gray

Attachment: signature.asc
Description: PGP signature