[PATCH v7 1/5] counter: Internalize sysfs interface code
William Breathitt Gray
vilhelm.gray at gmail.com
Wed Jan 6 00:29:23 EST 2021
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 at gmail.com> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20210106/aea65cc4/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list