From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751798AbbLUKhl (ORCPT ); Mon, 21 Dec 2015 05:37:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49142 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751444AbbLUKhk (ORCPT ); Mon, 21 Dec 2015 05:37:40 -0500 Subject: Re: [PATCH 5/5] hwmon: (sch56xx) Drop watchdog driver data reference count callbacks To: Guenter Roeck , linux-watchdog@vger.kernel.org References: <1450645503-16661-1-git-send-email-linux@roeck-us.net> <1450645503-16661-6-git-send-email-linux@roeck-us.net> Cc: Wim Van Sebroeck , Pratyush Anand , Damien Riegel , linux-kernel@vger.kernel.org From: Hans de Goede Message-ID: <5677D66C.6090906@redhat.com> Date: Mon, 21 Dec 2015 11:37:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1450645503-16661-6-git-send-email-linux@roeck-us.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > #include > #include > -#include > #include > #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 ? 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. > } > 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 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.