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

[William Breathitt Gray <vilhelm.gray@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7970 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]