Re: [PATCH v7 01/10] counter: Introduce the Generic Counter interface

On Thu, Jun 21, 2018 at 05:07:08PM -0400, William Breathitt Gray wrote:

This patch introduces the Generic Counter interface for supporting
counter devices.

In the context of the Generic Counter interface, a counter is defined as
a device that reports one or more "counts" based on the state changes of
one or more "signals" as evaluated by a defined "count function."

Driver callbacks should be provided to communicate with the device: to
read and write various Signals and Counts, and to set and get the
"action mode" and "count function" for various Synapses and Counts
respectively.

To support a counter device, a driver must first allocate the available
Counter Signals via counter_signal structures. These Signals should
be stored as an array and set to the signals array member of an
allocated counter_device structure before the Counter is registered to
the system.

Counter Counts may be allocated via counter_count structures, and
respective Counter Signal associations (Synapses) made via
counter_synapse structures. Associated counter_synapse structures are
stored as an array and set to the the synapses array member of the
respective counter_count structure. These counter_count structures are
set to the counts array member of an allocated counter_device structure
before the Counter is registered to the system.

A counter device is registered to the system by passing the respective
initialized counter_device structure to the counter_register function;
similarly, the counter_unregister function unregisters the respective
Counter. The devm_counter_register and devm_counter_unregister functions
serve as device memory-managed versions of the counter_register and
counter_unregister functions respectively.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: William Breathitt Gray <redacted>
---
 MAINTAINERS                       |    7 +
 drivers/Kconfig                   |    2 +
 drivers/Makefile                  |    1 +
 drivers/counter/Kconfig           |   18 +
 drivers/counter/Makefile          |    8 +
 drivers/counter/generic-counter.c | 1519 +++++++++++++++++++++++++++++
 include/linux/counter.h           |  534 ++++++++++
 7 files changed, 2089 insertions(+)
 create mode 100644 drivers/counter/Kconfig
 create mode 100644 drivers/counter/Makefile
 create mode 100644 drivers/counter/generic-counter.c
 create mode 100644 include/linux/counter.h

First cut of review, does counter.h really have to be that big?  It
feels like some of the "internal" functions and structures could be
local to drivers/counter/ right?

+menuconfig COUNTER
+	tristate "Counter support"
+	help
+	  Provides Generic Counter interface support for counter devices.
+
+	  Counter devices are prevalent within a diverse spectrum of industries.
+	  The ubiquitous presence of these devices necessitates a common
+	  interface and standard of interaction and exposure. This driver API
+	  attempts to resolve the issue of duplicate code found among existing
+	  counter device drivers by providing a generic counter interface for
+	  consumption. The Generic Counter interface enables drivers to support
+	  and expose a common set of components and functionality present in
+	  counter devices.

No need to explain an "API" in a Kconfig help text, which is for a user
of the kernel, not for someone writing kernel code.  Odds are individual
drivers will need to select this, or are you going to depend on this
option?

Why does it matter?  :)

+
+obj-$(CONFIG_COUNTER) += counter.o
+counter-y := generic-counter.o

Why not just call your .c file, "counter.c" to keep this simple?

Please use the normal SPDX identifiers we are using, as described in the
kernel documentation.  We aren't ready to use the "new" ones just yet.

+/*
+ * Generic Counter interface
+ * Copyright (C) 2017 William Breathitt Gray

It's 2018 now :)

+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/fs.h>
+#include <linux/gfp.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+
+#include <linux/counter.h>

Why a blank line?

+
+const char *const count_direction_str[2] = {
+	[COUNT_DIRECTION_FORWARD] = "forward",
+	[COUNT_DIRECTION_BACKWARD] = "backward"
+};
+EXPORT_SYMBOL(count_direction_str);

I have to ask, for all of these, why not EXPORT_SYMBOL_GPL()?

+
+const char *const count_mode_str[4] = {
+	[COUNT_MODE_NORMAL] = "normal",
+	[COUNT_MODE_RANGE_LIMIT] = "range limit",
+	[COUNT_MODE_NON_RECYCLE] = "non-recycle",
+	[COUNT_MODE_MODULO_N] = "modulo-n"
+};
+EXPORT_SYMBOL(count_mode_str);

And why do you need to export strings?

+
+ssize_t counter_signal_enum_read(struct counter_device *counter,
+				 struct counter_signal *signal, void *priv,
+				 char *buf)
+{
+	const struct counter_signal_enum_ext *const e = priv;
+	int err;
+	size_t index;
+
+	if (!e->get)
+		return -EINVAL;

How can e->get not be set?  Shouldn't you just not have called into this
if so?

+
+	err = e->get(counter, signal, &index);
+	if (err)
+		return err;
+
+	if (index >= e->num_items)
+		return -EINVAL;
+
+	return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);

No need to do the scnprintf() crud, it's a sysfs file, a simple
sprintf() works just fine.  Yeah, it makes people nervous, and it should :)

+}
+EXPORT_SYMBOL(counter_signal_enum_read);

sysfs attribute files really should be EXPORT_SYMBOL_GPL() please.

+
+ssize_t counter_signal_enum_write(struct counter_device *counter,
+				  struct counter_signal *signal, void *priv,
+				  const char *buf, size_t len)
+{
+	const struct counter_signal_enum_ext *const e = priv;
+	ssize_t index;
+	int err;
+
+	if (!e->set)
+		return -EINVAL;

Again, how can this be true?

+
+	index = __sysfs_match_string(e->items, e->num_items, buf);
+	if (index < 0)
+		return index;
+
+	err = e->set(counter, signal, index);
+	if (err)
+		return err;
+
+	return len;
+}
+EXPORT_SYMBOL(counter_signal_enum_write);
+
+ssize_t counter_signal_enum_available_read(struct counter_device *counter,
+					   struct counter_signal *signal,
+					   void *priv, char *buf)
+{
+	const struct counter_signal_enum_ext *const e = priv;
+	size_t i;
+	size_t len = 0;
+
+	if (!e->num_items)
+		return 0;
+
+	for (i = 0; i < e->num_items; i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
+			e->items[i]);

a list of items?  In sysfs?  Are you _SURE_ you want to do that?

+
+	return len;
+}
+EXPORT_SYMBOL(counter_signal_enum_available_read);
+
+ssize_t counter_count_enum_read(struct counter_device *counter,
+				struct counter_count *count, void *priv,
+				char *buf)
+{
+	const struct counter_count_enum_ext *const e = priv;
+	int err;
+	size_t index;
+
+	if (!e->get)
+		return -EINVAL;

Same comment everywhere for this...

let's skip lower in the files...

+static int counter_attribute_create(
+	struct counter_device_attr_group *const group,
+	const char *const prefix,
+	const char *const name,
+	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
+			char *buf),
+	ssize_t (*store)(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t len),

Typedefs for these function pointers are good to have.

+	void *const component)

That's a crazy list of parameters...

+{
+	struct counter_device_attr *counter_attr;
+	struct device_attribute *dev_attr;
+	int err;
+	struct list_head *const attr_list = &group->attr_list;
+
+	/* Allocate a Counter device attribute */
+	counter_attr = kzalloc(sizeof(*counter_attr), GFP_KERNEL);
+	if (!counter_attr)
+		return -ENOMEM;
+	dev_attr = &counter_attr->dev_attr;
+
+	sysfs_attr_init(&dev_attr->attr);
+
+	/* Configure device attribute */
+	dev_attr->attr.name = kasprintf(GFP_KERNEL, "%s%s", prefix, name);
+	if (!dev_attr->attr.name) {
+		err = -ENOMEM;
+		goto err_free_counter_attr;
+	}
+	if (show) {
+		dev_attr->attr.mode |= 0444;
+		dev_attr->show = show;
+	}
+	if (store) {
+		dev_attr->attr.mode |= 0200;
+		dev_attr->store = store;
+	}
+
+	/* Store associated Counter component with attribute */
+	counter_attr->component = component;
+
+	/* Keep track of the attribute for later cleanup */
+	list_add(&counter_attr->l, attr_list);
+	group->num_attr++;

So you are dynamically creating sysfs attributes?  Why not just have one
big group and only enable them if needed?  That would make things a lot
simpler, right?

+struct signal_comp_t {

"_t"???

+	struct counter_signal	*signal;
+};
+
+static ssize_t counter_signal_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct counter_device *const counter = dev_get_drvdata(dev);
+	const struct counter_device_attr *const devattr = to_counter_attr(attr);
+	const struct signal_comp_t *const component = devattr->component;
+	struct counter_signal *const signal = component->signal;
+	int err;
+	struct signal_read_value val = { .buf = buf };
+
+	err = counter->ops->signal_read(counter, signal, &val);
+	if (err)
+		return err;
+
+	return val.len;
+}
+
+struct name_comp_t {

"_t"???

Same for the rest of this file...

Same identifier issue.

+/*
+ * Counter interface
+ * Copyright (C) 2017 William Breathitt Gray

2018!

And again, it looks like this file can be a lot smaller, but I don't see
a user of it just yet, so I don't really know...

thanks,

greg k-h