Messages in this thread | | | Subject | Re: [PATCH v5 1/5] counter: Internalize sysfs interface code | From | David Lechner <> | Date | Tue, 20 Oct 2020 10:38:43 -0500 |
| |
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().
| |