| Message ID | cover.1601170670.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce the Counter character device interface | expand |
On 9/26/20 9:18 PM, William Breathitt Gray wrote: > The following are some questions I have about this patchset: > > 1. Should standard Counter component data types be defined as u8 or u32? > > Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL > have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and > COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the > Counter subsystem code as u8 data types. > > If u32 is used for these values instead, C enum structures could be > used by driver authors to implicit cast these values via the driver > callback parameters; userspace would still use u32 with no issue. > > In theory this can work because GCC will treat enums are having a > 32-bit size; but I worry about the possibility of build targets that > have -fshort-enums enabled, resulting in enums having a size less > than 32 bits. Would this be a problem? We shouldn't have to worry about userspace programs using -fshort-enums since that would break all kernel interfaces that use enums, not just these - so no one should be using that compiler flag. So I am in favor of using strongly typed enums with u32 as the "generic" enum member type. > > 2. Should I have reserved members in the userspace structures? > > The structures in include/uapi/linux/counter.h are available to > userspace applications. Should I reserve space in these structures > for future additions and usage? Will endianess and packing be a > concern here? > Since there doesn't seem to be a large number of counter devices this probably isn't critical. Are there any aspects of counter devices in general that couldn't be described with the proposed structures? For example, could there be components more than two levels deep (i.e. it would need id, parent id and grandparent id to describe fully)?
On 9/26/20 9:18 PM, William Breathitt Gray wrote: > This is a reimplementation of the Generic Counter driver interface. I'll follow up if I find any problems while testing but here are some comments I had from looking over the patch. > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > new file mode 100644 > index 000000000000..987c6e8277eb > --- /dev/null > +++ b/drivers/counter/counter-core.c > +/** > + * 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. > + */ > +int counter_register(struct counter_device *const counter) > +{ > + struct device *const dev = &counter->dev; > + int err; > + > + /* Acquire unique ID */ > + counter->id = ida_simple_get(&counter_ida, 0, 0, GFP_KERNEL); > + if (counter->id < 0) > + return counter->id; > + > + /* Configure device structure for Counter */ > + dev->type = &counter_device_type; > + dev->bus = &counter_bus_type; > + if (counter->parent) { > + dev->parent = counter->parent; > + dev->of_node = counter->parent->of_node; > + } > + dev_set_name(dev, "counter%d", counter->id); > + device_initialize(dev);> + dev_set_drvdata(dev, counter); > + > + /* Add Counter sysfs attributes */ > + err = counter_sysfs_add(counter); > + if (err) > + goto err_free_id; > + > + /* Add device to system */ > + err = device_add(dev); > + if (err) { > + put_device(dev); > + goto err_free_id; > + } > + > + return 0; > + > +err_free_id: > + /* get_device/put_device combo used to free managed resources */ > + get_device(dev); > + put_device(dev); I've never seen this in a driver before, so it makes me think this is not the "right way" to do this. After device_initialize() is called, we already should have a reference to dev, so only device_put() is needed. > + ida_simple_remove(&counter_ida, counter->id); In the case of error after device_initialize() is called, won't this result in ida_simple_remove() being called twice, once here and once in the release callback? > + return err; > +} > +EXPORT_SYMBOL_GPL(counter_register); > + > +/** > + * counter_unregister - unregister Counter from the system > + * @counter: pointer to Counter to unregister > + * > + * The Counter is unregistered from the system; all allocated memory is freed. > + */ > +void counter_unregister(struct counter_device *const counter) > +{ > + if (!counter) > + return; > + > + device_unregister(&counter->dev); > +} > +EXPORT_SYMBOL_GPL(counter_unregister); > + > +static void devm_counter_unreg(struct device *dev, void *res) To be consistent, it would be nice to spell out unregister. > +{ > + counter_unregister(*(struct counter_device **)res); > +} > + > diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c > new file mode 100644 > index 000000000000..e66ed99dd5ea > --- /dev/null > +++ b/drivers/counter/counter-sysfs.c > +/** > + * counter_sysfs_add - Adds Counter sysfs attributes to the device structure > + * @counter: Pointer to the Counter device structure > + * > + * Counter sysfs attributes are created and added to the respective device > + * structure for later registration to the system. Resource-managed memory > + * allocation is performed by this function, and this memory should be freed > + * when no longer needed (automatically by a device_unregister call, or > + * manually by a devres_release_all call). > + */ > +int counter_sysfs_add(struct counter_device *const counter) > +{ > + struct device *const dev = &counter->dev; > + const size_t num_groups = counter->num_signals + counter->num_counts + > + 1; It is OK to go past 80 columns, especially for just for a few characters. > + struct counter_attribute_group *groups; > + size_t i, j; > + int err; > + struct attribute_group *group; > + struct counter_attribute *p; > + > + /* Allocate space for attribute groups (signals, counts, and ext) */ > + groups = devm_kcalloc(dev, num_groups, sizeof(*groups), GFP_KERNEL); > + if (!groups) > + return -ENOMEM; > + > + /* Initialize attribute lists */ > + for (i = 0; i < num_groups; i++) > + INIT_LIST_HEAD(&groups[i].attr_list); > + > + /* Register Counter device attributes */ > + err = counter_device_register(counter, groups); This function name is a bit misleading. At first I though we were registering a new counter device (struct device). Maybe counter_sysfs_create_attrs() would be a better name? (I wouldn't mind having all functions in this file having a "counter_sysfs_" prefix for that matter.) > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index 1ff07faef27f..938085dead80 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -406,7 +414,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > > priv->counter.name = dev_name(dev); > priv->counter.parent = dev; > - priv->counter.ops = &ti_eqep_counter_ops; > + priv->counter.parent = &ti_eqep_counter_ops; > priv->counter.counts = ti_eqep_counts; > priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts); > priv->counter.signals = ti_eqep_signals; This looks like an unintentional change and causes a compile error. > diff --git a/include/linux/counter.h b/include/linux/counter.h > index 9dbd5df4cd34..132bfecca5c3 100644 > --- a/include/linux/counter.h > +++ b/include/linux/counter.h > @@ -6,417 +6,195 @@ > #ifndef _COUNTER_H_ > #define _COUNTER_H_ > > -#include <linux/counter_enum.h> > #include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/list.h> struct list_head is defined in linux/types.h. Is there something else we are using from linux/list.h in this file? > #include <linux/types.h> > It would be helpful to have kernel doc comments on everything in this file.
On 9/26/20 9:18 PM, William Breathitt Gray wrote: > +static ssize_t counter_comp_u8_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + const struct counter_attribute *const a = to_counter_attribute(attr); > + struct counter_device *const counter = dev_get_drvdata(dev); > + struct counter_count *const count = a->parent; > + struct counter_synapse *const synapse = a->comp.priv; > + const struct counter_available *const avail = a->comp.priv; > + int err; > + bool bool_data; > + u8 data; > + > + switch (a->comp.type) { > + case COUNTER_COMP_BOOL: > + err = kstrtobool(buf, &bool_data); > + data = bool_data; > + break; > + case COUNTER_COMP_FUNCTION: > + err = find_in_string_array(&data, count->functions_list, > + count->num_functions, buf, > + counter_function_str); > + break; > + case COUNTER_COMP_SYNAPSE_ACTION: > + err = find_in_string_array(&data, synapse->actions_list, > + synapse->num_actions, buf, > + counter_synapse_action_str); > + break; > + case COUNTER_COMP_ENUM: > + err = __sysfs_match_string(avail->strs, avail->num_items, buf); > + data = err; > + break; > + case COUNTER_COMP_COUNT_MODE: > + err = find_in_string_array(&data, avail->enums, > + avail->num_items, buf, > + counter_count_mode_str); > + break; > + default: > + err = kstrtou8(buf, 0, &data); > + break; > + } In this function, return values are not always errors. So it would make sense to call the err variable ret instead and check for ret < 0 below. Setting enums to a value with index > 0 fails currently because of this. > + if (err) > + return err; > + > + switch (a->scope) { > + case COUNTER_SCOPE_DEVICE: > + err = a->comp.device_u8_write(counter, data); > + break; > + case COUNTER_SCOPE_SIGNAL: > + err = a->comp.signal_u8_write(counter, a->parent, data); > + break; > + case COUNTER_SCOPE_COUNT: > + if (a->comp.type == COUNTER_COMP_SYNAPSE_ACTION) > + err = a->comp.action_write(counter, count, synapse, > + data); > + else > + err = a->comp.count_u8_write(counter, count, data); > + break; > + } > + if (err) > + return err; > + > + return len; > +}
On Mon, Oct 12, 2020 at 07:35:11PM -0500, David Lechner wrote: > On 9/26/20 9:18 PM, William Breathitt Gray wrote: > > The following are some questions I have about this patchset: > > > > 1. Should standard Counter component data types be defined as u8 or u32? > > > > Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL > > have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and > > COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the > > Counter subsystem code as u8 data types. > > > > If u32 is used for these values instead, C enum structures could be > > used by driver authors to implicit cast these values via the driver > > callback parameters; userspace would still use u32 with no issue. > > > > In theory this can work because GCC will treat enums are having a > > 32-bit size; but I worry about the possibility of build targets that > > have -fshort-enums enabled, resulting in enums having a size less > > than 32 bits. Would this be a problem? > > We shouldn't have to worry about userspace programs using -fshort-enums > since that would break all kernel interfaces that use enums, not just > these - so no one should be using that compiler flag. > > So I am in favor of using strongly typed enums with u32 as the > "generic" enum member type. That reasoning pacifies my worries. I'll test out strongly typed enums in the next revision of this patchset. > > > > 2. Should I have reserved members in the userspace structures? > > > > The structures in include/uapi/linux/counter.h are available to > > userspace applications. Should I reserve space in these structures > > for future additions and usage? Will endianess and packing be a > > concern here? > > > Since there doesn't seem to be a large number of counter devices > this probably isn't critical. Are there any aspects of counter > devices in general that couldn't be described with the proposed > structures? For example, could there be components more than two > levels deep (i.e. it would need id, parent id and grandparent id > to describe fully)? I can't think of any devices that would require more depth, so we probably don't need any additional members added to the userspace structures. The current structure should be flexible enough for future expansion, and any additional functionality we come across can be handled by implementing new extension types as we have for the sysfs attributes. William Breathitt Gray
On Mon, Oct 12, 2020 at 09:15:00PM -0500, David Lechner wrote: > On 9/26/20 9:18 PM, William Breathitt Gray wrote: > > This is a reimplementation of the Generic Counter driver interface. > > I'll follow up if I find any problems while testing but here are some > comments I had from looking over the patch. > > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > > new file mode 100644 > > index 000000000000..987c6e8277eb > > --- /dev/null > > +++ b/drivers/counter/counter-core.c > > > > +/** > > + * 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. > > + */ > > +int counter_register(struct counter_device *const counter) > > +{ > > + struct device *const dev = &counter->dev; > > + int err; > > + > > + /* Acquire unique ID */ > > + counter->id = ida_simple_get(&counter_ida, 0, 0, GFP_KERNEL); > > + if (counter->id < 0) > > + return counter->id; > > + > > + /* Configure device structure for Counter */ > > + dev->type = &counter_device_type; > > + dev->bus = &counter_bus_type; > > + if (counter->parent) { > > + dev->parent = counter->parent; > > + dev->of_node = counter->parent->of_node; > > + } > > + dev_set_name(dev, "counter%d", counter->id); > > + device_initialize(dev);> + dev_set_drvdata(dev, counter); > > + > > + /* Add Counter sysfs attributes */ > > + err = counter_sysfs_add(counter); > > + if (err) > > + goto err_free_id; > > + > > + /* Add device to system */ > > + err = device_add(dev); > > + if (err) { > > + put_device(dev); > > + goto err_free_id; > > + } > > + > > + return 0; > > + > > +err_free_id: > > + /* get_device/put_device combo used to free managed resources */ > > + get_device(dev); > > + put_device(dev); > > I've never seen this in a driver before, so it makes me think this is > not the "right way" to do this. After device_initialize() is called, we > already should have a reference to dev, so only device_put() is needed. I do admit this feels very hacky, but I'm not sure how to handle this more elegantly. The problem is that device_initialize() is not enough by itself -- it just initializes the structure, while device_add() increments the reference count via a call to get_device(). So let's assume counter_sysfs_add() fails and we go to err_free_id before device_add() is called; if we ignore get_device(), the reference count will be 0 at this point. We then call put_device(), which calls kobject_put(), kref_put(), and eventually refcount_dec_and_test(). The refcount_dec_and_test() function returns 0 at this point because the reference count can't be decremented further (refcount is already 0), so release() is never called and we end up leaking all the memory we allocated in counter_sysfs_add(). Is my logic correct here? > > + ida_simple_remove(&counter_ida, counter->id); > > In the case of error after device_initialize() is called, won't this > result in ida_simple_remove() being called twice, once here and once in > the release callback? I hadn't considered that. I suppose the ida_simple_remove here is not necessary because we will always call ida_simple_remove() in counter_device_release() when we call put_device(). I'll remove this ida_simple_remove() from counter_register() then. > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(counter_register); > > + > > +/** > > + * counter_unregister - unregister Counter from the system > > + * @counter: pointer to Counter to unregister > > + * > > + * The Counter is unregistered from the system; all allocated memory is freed. > > + */ > > +void counter_unregister(struct counter_device *const counter) > > +{ > > + if (!counter) > > + return; > > + > > + device_unregister(&counter->dev); > > +} > > +EXPORT_SYMBOL_GPL(counter_unregister); > > + > > +static void devm_counter_unreg(struct device *dev, void *res) > > To be consistent, it would be nice to spell out unregister. Ack. > > +{ > > + counter_unregister(*(struct counter_device **)res); > > +} > > + > > > diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c > > new file mode 100644 > > index 000000000000..e66ed99dd5ea > > --- /dev/null > > +++ b/drivers/counter/counter-sysfs.c > > > +/** > > + * counter_sysfs_add - Adds Counter sysfs attributes to the device structure > > + * @counter: Pointer to the Counter device structure > > + * > > + * Counter sysfs attributes are created and added to the respective device > > + * structure for later registration to the system. Resource-managed memory > > + * allocation is performed by this function, and this memory should be freed > > + * when no longer needed (automatically by a device_unregister call, or > > + * manually by a devres_release_all call). > > + */ > > +int counter_sysfs_add(struct counter_device *const counter) > > +{ > > + struct device *const dev = &counter->dev; > > + const size_t num_groups = counter->num_signals + counter->num_counts + > > + 1; > > It is OK to go past 80 columns, especially for just for a few characters. Ack. > > + struct counter_attribute_group *groups; > > + size_t i, j; > > + int err; > > + struct attribute_group *group; > > + struct counter_attribute *p; > > + > > + /* Allocate space for attribute groups (signals, counts, and ext) */ > > + groups = devm_kcalloc(dev, num_groups, sizeof(*groups), GFP_KERNEL); > > + if (!groups) > > + return -ENOMEM; > > + > > + /* Initialize attribute lists */ > > + for (i = 0; i < num_groups; i++) > > + INIT_LIST_HEAD(&groups[i].attr_list); > > + > > + /* Register Counter device attributes */ > > + err = counter_device_register(counter, groups); > > This function name is a bit misleading. At first I though we were registering > a new counter device (struct device). Maybe counter_sysfs_create_attrs() > would be a better name? (I wouldn't mind having all functions in this > file having a "counter_sysfs_" prefix for that matter.) Good point, I'll rename these to make it clearer. > > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > > index 1ff07faef27f..938085dead80 100644 > > --- a/drivers/counter/ti-eqep.c > > +++ b/drivers/counter/ti-eqep.c > > > > @@ -406,7 +414,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > > > > priv->counter.name = dev_name(dev); > > priv->counter.parent = dev; > > - priv->counter.ops = &ti_eqep_counter_ops; > > + priv->counter.parent = &ti_eqep_counter_ops; > > priv->counter.counts = ti_eqep_counts; > > priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts); > > priv->counter.signals = ti_eqep_signals; > > This looks like an unintentional change and causes a compile error. Yeah, it was an unintentional change. I'll fix this. :-) > > diff --git a/include/linux/counter.h b/include/linux/counter.h > > index 9dbd5df4cd34..132bfecca5c3 100644 > > --- a/include/linux/counter.h > > +++ b/include/linux/counter.h > > @@ -6,417 +6,195 @@ > > #ifndef _COUNTER_H_ > > #define _COUNTER_H_ > > > > -#include <linux/counter_enum.h> > > #include <linux/device.h> > > +#include <linux/kernel.h> > > +#include <linux/list.h> > > struct list_head is defined in linux/types.h. Is there something else > we are using from linux/list.h in this file? I think this was left over from when I had list_add() in this file in an earlier version; I'll remove this header now since it doesn't look necessary. > > #include <linux/types.h> > > > > > It would be helpful to have kernel doc comments on everything in this file. Ack. William Breathitt Gray
On Wed, Oct 14, 2020 at 08:38:40PM -0500, David Lechner wrote: > On 9/26/20 9:18 PM, William Breathitt Gray wrote: > > +static ssize_t counter_comp_u8_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + const struct counter_attribute *const a = to_counter_attribute(attr); > > + struct counter_device *const counter = dev_get_drvdata(dev); > > + struct counter_count *const count = a->parent; > > + struct counter_synapse *const synapse = a->comp.priv; > > + const struct counter_available *const avail = a->comp.priv; > > + int err; > > + bool bool_data; > > + u8 data; > > + > > + switch (a->comp.type) { > > + case COUNTER_COMP_BOOL: > > + err = kstrtobool(buf, &bool_data); > > + data = bool_data; > > + break; > > + case COUNTER_COMP_FUNCTION: > > + err = find_in_string_array(&data, count->functions_list, > > + count->num_functions, buf, > > + counter_function_str); > > + break; > > + case COUNTER_COMP_SYNAPSE_ACTION: > > + err = find_in_string_array(&data, synapse->actions_list, > > + synapse->num_actions, buf, > > + counter_synapse_action_str); > > + break; > > + case COUNTER_COMP_ENUM: > > + err = __sysfs_match_string(avail->strs, avail->num_items, buf); > > + data = err; > > + break; > > + case COUNTER_COMP_COUNT_MODE: > > + err = find_in_string_array(&data, avail->enums, > > + avail->num_items, buf, > > + counter_count_mode_str); > > + break; > > + default: > > + err = kstrtou8(buf, 0, &data); > > + break; > > + } > > In this function, return values are not always errors. So it would make > sense to call the err variable ret instead and check for ret < 0 below. > > Setting enums to a value with index > 0 fails currently because of this. Thank you for pointing this out, I'll fix this. William Breathitt Gray > > + if (err) > > + return err; > > + > > + switch (a->scope) { > > + case COUNTER_SCOPE_DEVICE: > > + err = a->comp.device_u8_write(counter, data); > > + break; > > + case COUNTER_SCOPE_SIGNAL: > > + err = a->comp.signal_u8_write(counter, a->parent, data); > > + break; > > + case COUNTER_SCOPE_COUNT: > > + if (a->comp.type == COUNTER_COMP_SYNAPSE_ACTION) > > + err = a->comp.action_write(counter, count, synapse, > > + data); > > + else > > + err = a->comp.count_u8_write(counter, count, data); > > + break; > > + } > > + if (err) > > + return err; > > + > > + return len; > > +} >
On 10/18/20 9:49 AM, William Breathitt Gray wrote: > On Mon, Oct 12, 2020 at 09:15:00PM -0500, David Lechner wrote: >> On 9/26/20 9:18 PM, William Breathitt Gray wrote: >>> This is a reimplementation of the Generic Counter driver interface. >> >> I'll follow up if I find any problems while testing but here are some >> comments I had from looking over the patch. >> >>> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c >>> new file mode 100644 >>> index 000000000000..987c6e8277eb >>> --- /dev/null >>> +++ b/drivers/counter/counter-core.c >> >> >>> +/** >>> + * 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. >>> + */ >>> +int counter_register(struct counter_device *const counter) >>> +{ >>> + struct device *const dev = &counter->dev; >>> + int err; >>> + >>> + /* Acquire unique ID */ >>> + counter->id = ida_simple_get(&counter_ida, 0, 0, GFP_KERNEL); >>> + if (counter->id < 0) >>> + return counter->id; >>> + >>> + /* Configure device structure for Counter */ >>> + dev->type = &counter_device_type; >>> + dev->bus = &counter_bus_type; >>> + if (counter->parent) { >>> + dev->parent = counter->parent; >>> + dev->of_node = counter->parent->of_node; >>> + } >>> + dev_set_name(dev, "counter%d", counter->id); >>> + device_initialize(dev);> + dev_set_drvdata(dev, counter); >>> + >>> + /* Add Counter sysfs attributes */ >>> + err = counter_sysfs_add(counter); >>> + if (err) >>> + goto err_free_id; >>> + >>> + /* Add device to system */ >>> + err = device_add(dev); >>> + if (err) { >>> + put_device(dev); >>> + goto err_free_id; >>> + } >>> + >>> + return 0; >>> + >>> +err_free_id: >>> + /* get_device/put_device combo used to free managed resources */ >>> + get_device(dev); >>> + put_device(dev); >> >> I've never seen this in a driver before, so it makes me think this is >> not the "right way" to do this. After device_initialize() is called, we >> already should have a reference to dev, so only device_put() is needed. > > I do admit this feels very hacky, but I'm not sure how to handle this > more elegantly. The problem is that device_initialize() is not enough by > itself -- it just initializes the structure, while device_add() > increments the reference count via a call to get_device(). add_device() also releases this reference by calling put_device() in all return paths. The reference count is initialized to 1 in device_initialize(). device_initialize > kobject_init > kobject_init_internal > kref_init > > So let's assume counter_sysfs_add() fails and we go to err_free_id > before device_add() is called; if we ignore get_device(), the reference > count will be 0 at this point. We then call put_device(), which calls > kobject_put(), kref_put(), and eventually refcount_dec_and_test(). > > The refcount_dec_and_test() function returns 0 at this point because the > reference count can't be decremented further (refcount is already 0), so > release() is never called and we end up leaking all the memory we > allocated in counter_sysfs_add(). > > Is my logic correct here? Not quite. I think you missed a few things I mentioned above. So we never have a ref count of 0 until the very last call to put_device().
On Tue, Oct 20, 2020 at 10:38:43AM -0500, David Lechner wrote: > On 10/18/20 9:49 AM, William Breathitt Gray wrote: > > On Mon, Oct 12, 2020 at 09:15:00PM -0500, David Lechner wrote: > >> On 9/26/20 9:18 PM, William Breathitt Gray wrote: > >>> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > >>> new file mode 100644 > >>> index 000000000000..987c6e8277eb > >>> --- /dev/null > >>> +++ b/drivers/counter/counter-core.c > >> > >> > >>> +/** > >>> + * 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. > >>> + */ > >>> +int counter_register(struct counter_device *const counter) > >>> +{ > >>> + struct device *const dev = &counter->dev; > >>> + int err; > >>> + > >>> + /* Acquire unique ID */ > >>> + counter->id = ida_simple_get(&counter_ida, 0, 0, GFP_KERNEL); > >>> + if (counter->id < 0) > >>> + return counter->id; > >>> + > >>> + /* Configure device structure for Counter */ > >>> + dev->type = &counter_device_type; > >>> + dev->bus = &counter_bus_type; > >>> + if (counter->parent) { > >>> + dev->parent = counter->parent; > >>> + dev->of_node = counter->parent->of_node; > >>> + } > >>> + dev_set_name(dev, "counter%d", counter->id); > >>> + device_initialize(dev);> + dev_set_drvdata(dev, counter); > >>> + > >>> + /* Add Counter sysfs attributes */ > >>> + err = counter_sysfs_add(counter); > >>> + if (err) > >>> + goto err_free_id; > >>> + > >>> + /* Add device to system */ > >>> + err = device_add(dev); > >>> + if (err) { > >>> + put_device(dev); > >>> + goto err_free_id; > >>> + } > >>> + > >>> + return 0; > >>> + > >>> +err_free_id: > >>> + /* get_device/put_device combo used to free managed resources */ > >>> + get_device(dev); > >>> + put_device(dev); > >> > >> I've never seen this in a driver before, so it makes me think this is > >> not the "right way" to do this. After device_initialize() is called, we > >> already should have a reference to dev, so only device_put() is needed. > > > > I do admit this feels very hacky, but I'm not sure how to handle this > > more elegantly. The problem is that device_initialize() is not enough by > > itself -- it just initializes the structure, while device_add() > > increments the reference count via a call to get_device(). > > add_device() also releases this reference by calling put_device() in all > return paths. The reference count is initialized to 1 in device_initialize(). > > device_initialize > kobject_init > kobject_init_internal > kref_init You're right, I completely overlooked this: kref_init() initializes the reference count to 1. Therefore, the get_device() call is incorrect and should be be removed. > > > > So let's assume counter_sysfs_add() fails and we go to err_free_id > > before device_add() is called; if we ignore get_device(), the reference > > count will be 0 at this point. We then call put_device(), which calls > > kobject_put(), kref_put(), and eventually refcount_dec_and_test(). > > > > The refcount_dec_and_test() function returns 0 at this point because the > > reference count can't be decremented further (refcount is already 0), so > > release() is never called and we end up leaking all the memory we > > allocated in counter_sysfs_add(). > > > > Is my logic correct here? > > Not quite. I think you missed a few things I mentioned above. So we never > have a ref count of 0 until the very last call to put_device(). Ack. William Breathitt Gray
Changes in v5: - Fixed typographical errors in documentation and comments - Updated flow charts in documentation for clarity - Moved uapi header to be part of the character device intro patch - Fix git squash mistake in 104-quad-8.c; remove redundant changes - Fix git merge mistake in 104-quad-8.c; fix locking race condition - Minor code cleanup for clarity; adjust whitespace/flow - Use put_device if device_add fails - Document sysfs structures - Rename "owner" symbols to "scope"; more apt name - Use resource-managed devm_* allocation functions - Rename *_free functions to *_remove; following common convention - Rename COUNTER_DATA* to COUNTER_COMP*; more obvious meaning - Rename various symbol and define names for clarity - Bring back static ops function; more secure to have static const - Rename counter_available union members to "enums" and "strs" - Implement COUNTER_EVENT* constants; event types are now standard - Implement atomic Counter watches swap; no more racy event config Over the past couple years we have noticed some shortcomings with the Counter sysfs interface. Although useful in the majority of situations, there are certain use-cases where interacting through sysfs attributes can become cumbersome and inefficient. A desire to support more advanced functionality such as timestamps, multi-axes positioning tables, and other such latency-sensitive applications, has motivated a reevaluation of the Counter subsystem. I believe a character device interface will be helpful for this more niche area of counter device use. To quell any concerns from the offset: this patchset makes no changes to the existing Counter sysfs userspace interface -- existing userspace applications will continue to work with no modifications necessary. I request that driver maintainers please test their applications to verify that this is true, and report any discrepancies if they arise. However, this patchset does contain a major reimplementation of the Counter subsystem core and driver API. A reimplementation was necessary in order to separate the sysfs code from the counter device drivers and internalize it as a dedicated component of the core Counter subsystem module. A minor benefit from all of this is that the sysfs interface is now ensured a certain amount of consistency because the translation is performed outside of individual counter device drivers. Essentially, the reimplementation has enabled counter device drivers to pass and handle data as native C datatypes now rather than the sysfs strings from before. 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 | +----------------------------+ | Processes data from device | +----------------------------+ | ------------------- / driver callbacks / ------------------- | V +----------------------+ | Counter core | +----------------------+ | Routes device driver | | callbacks to the | | userspace interfaces | +----------------------+ | ------------------- / driver callbacks / ------------------- | +---------------+---------------+ | | V V +--------------------+ +---------------------+ | Counter sysfs | | Counter chrdev | +--------------------+ +---------------------+ | Translates to the | | Translates to the | | standard Counter | | standard Counter | | sysfs output | | character device | +--------------------+ +---------------------+ Thereafter, data can be transferred directly between the Counter device driver and Counter userspace interface: ---------------------- / Counter device \ +----------------------+ | Count register: 0x28 | +----------------------+ | ----------------- / raw count data / ----------------- | V +----------------------------+ | Counter device driver | +----------------------------+ | Processes data from device | |----------------------------| | Type: u64 | | Value: 42 | +----------------------------+ | ---------- / u64 / ---------- | +---------------+---------------+ | | V V +--------------------+ +---------------------+ | Counter sysfs | | Counter chrdev | +--------------------+ +---------------------+ | Translates to the | | Translates to the | | standard Counter | | standard Counter | | sysfs output | | character device | |--------------------| |---------------------| | Type: const char * | | Type: u64 | | Value: "42" | | Value: 42 | +--------------------+ +---------------------+ | | --------------- ----------------------- / const char * / / struct counter_event / --------------- ----------------------- | | | V | +-----------+ | | read | | +-----------+ | \ Count: 42 / | ----------- | V +--------------------------------------------------+ | `/sys/bus/counter/devices/counterX/countY/count` | +--------------------------------------------------+ \ Count: "42" / -------------------------------------------------- Counter device data is exposed through standard character device read operations. Device data is gathered when a Counter event is pushed by the respective Counter device driver. Configuration is handled via ioctl operations on the respective Counter character device node. The following are some questions I have about this patchset: 1. Should standard Counter component data types be defined as u8 or u32? Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the Counter subsystem code as u8 data types. If u32 is used for these values instead, C enum structures could be used by driver authors to implicit cast these values via the driver callback parameters; userspace would still use u32 with no issue. In theory this can work because GCC will treat enums are having a 32-bit size; but I worry about the possibility of build targets that have -fshort-enums enabled, resulting in enums having a size less than 32 bits. Would this be a problem? 2. Should I have reserved members in the userspace structures? The structures in include/uapi/linux/counter.h are available to userspace applications. Should I reserve space in these structures for future additions and usage? Will endianess and packing be a concern here? William Breathitt Gray (5): counter: Internalize sysfs interface code docs: counter: Update to reflect sysfs internalization counter: Add character device interface docs: counter: Document character device interface counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8 Documentation/ABI/testing/sysfs-bus-counter | 18 + .../ABI/testing/sysfs-bus-counter-104-quad-8 | 32 + Documentation/driver-api/generic-counter.rst | 408 ++++- .../userspace-api/ioctl/ioctl-number.rst | 1 + MAINTAINERS | 2 +- drivers/counter/104-quad-8.c | 775 +++++---- drivers/counter/Kconfig | 6 +- drivers/counter/Makefile | 1 + drivers/counter/counter-chrdev.c | 451 +++++ drivers/counter/counter-chrdev.h | 16 + drivers/counter/counter-core.c | 190 +++ drivers/counter/counter-sysfs.c | 862 ++++++++++ drivers/counter/counter-sysfs.h | 13 + drivers/counter/counter.c | 1496 ----------------- drivers/counter/ftm-quaddec.c | 60 +- drivers/counter/microchip-tcb-capture.c | 100 +- drivers/counter/stm32-lptimer-cnt.c | 175 +- drivers/counter/stm32-timer-cnt.c | 145 +- drivers/counter/ti-eqep.c | 226 +-- include/linux/counter.h | 618 +++---- include/linux/counter_enum.h | 45 - include/uapi/linux/counter.h | 99 ++ 22 files changed, 3053 insertions(+), 2686 deletions(-) create mode 100644 drivers/counter/counter-chrdev.c create mode 100644 drivers/counter/counter-chrdev.h create mode 100644 drivers/counter/counter-core.c create mode 100644 drivers/counter/counter-sysfs.c create mode 100644 drivers/counter/counter-sysfs.h delete mode 100644 drivers/counter/counter.c delete mode 100644 include/linux/counter_enum.h create mode 100644 include/uapi/linux/counter.h