Hi Hans,
On 12/21/2015 02:37 AM, Hans de Goede wrote:
Hi,
On 20-12-15 22:05, Guenter Roeck wrote:
Reference counting is now implemented in the watchdog core and no longer
required in watchdog drivers.
Signed-off-by: Guenter Roeck <li...@roeck-us.net>
---
drivers/hwmon/sch56xx-common.c | 30 ------------------------------
1 file changed, 30 deletions(-)
diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 738681983284..88dba68a9abb 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -30,7 +30,6 @@
#include <linux/watchdog.h>
#include <linux/miscdevice.h>
#include <linux/uaccess.h>
-#include <linux/kref.h>
#include <linux/slab.h>
#include "sch56xx-common.h"
@@ -67,7 +66,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once
started (default="
struct sch56xx_watchdog_data {
u16 addr;
struct mutex *io_lock;
- struct kref kref;
struct watchdog_info wdinfo;
struct watchdog_device wddev;
u8 watchdog_preset;
@@ -258,15 +256,6 @@ EXPORT_SYMBOL(sch56xx_read_virtual_reg12);
* Watchdog routines
*/
-/* Release our data struct when we're unregistered *and*
- all references to our watchdog device are released */
-static void watchdog_release_resources(struct kref *r)
-{
- struct sch56xx_watchdog_data *data =
- container_of(r, struct sch56xx_watchdog_data, kref);
- kfree(data);
You're not replacing this kfree() anywhere, so now the data
allocated by sch56xx_watchdog_register() never gets free-ed
after a sch56xx_watchdog_unregister(). If I read this patch
set correctly then after this patch-set it is safe to
immediately free the memory containing the struct watchdog_device
(and also the struct wdinfo ?) after calling
watchdog_unregister_device(), even if userspace still has the
watchdog cdev open, correct ?
Yes, this is correct for both struct watchdog_device and struct wdinfo.
In that case (continue reading below) ...
-}
-
static int watchdog_set_timeout(struct watchdog_device *wddev,
unsigned int timeout)
{
@@ -395,28 +384,12 @@ static int watchdog_stop(struct watchdog_device *wddev)
return 0;
}
-static void watchdog_ref(struct watchdog_device *wddev)
-{
- struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
-
- kref_get(&data->kref);
-}
-
-static void watchdog_unref(struct watchdog_device *wddev)
-{
- struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
-
- kref_put(&data->kref, watchdog_release_resources);
-}
-
static const struct watchdog_ops watchdog_ops = {
.owner = THIS_MODULE,
.start = watchdog_start,
.stop = watchdog_stop,
.ping = watchdog_trigger,
.set_timeout = watchdog_set_timeout,
- .ref = watchdog_ref,
- .unref = watchdog_unref,
};
struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
@@ -448,7 +421,6 @@ struct sch56xx_watchdog_data
*sch56xx_watchdog_register(struct device *parent,
data->addr = addr;
data->io_lock = io_lock;
- kref_init(&data->kref);
strlcpy(data->wdinfo.identity, "sch56xx watchdog",
sizeof(data->wdinfo.identity));
@@ -494,8 +466,6 @@ EXPORT_SYMBOL(sch56xx_watchdog_register);
void sch56xx_watchdog_unregister(struct sch56xx_watchdog_data *data)
{
watchdog_unregister_device(&data->wddev);
- kref_put(&data->kref, watchdog_release_resources);
- /* Don't touch data after this it may have been free-ed! */
These 2 lines need to be replaced by a "kfree(data);" rather then being
removed.
Good catch. Fixed.
}
EXPORT_SYMBOL(sch56xx_watchdog_unregister);
Regards,
Hans
p.s.
The patches for da9052_wdt and da9055_wdt are also interesting,
their da905#_wdt_release_resources() functions are nops,
making the entire refcounting exercise a nop.
For da9052_wdt is the case since this commit:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/watchdog/da9052_wdt.c?id=0360dffedd7bad92f174b2ce5e69e960451d2b59
And for da9055 with commit ee8c94adff9b ("watchdog: da9055: Fix invalid free
of devm_ allocated data").
Which fixes the bug it tries to fix in the wrong way, the right
way would have been to change the devm_kzalloc to a regular kzalloc,
since devm_*alloc cannot be used together with refcounting to keep
memory alive after device unregister time.
What this means is that these drivers actually have a bug wrt
freeing in use memory on driver unbind while userspace has
the watchdog cdev open, and this gets fixed by this patch-set,
it would be very good to mention this in the commit messages.
Good point. I updated the commit log.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
