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().
