Re: [PATCH 5/5] hwmon: (sch56xx) Drop watchdog driver data reference count callbacks

[Hans de Goede <hdegoede@redhat.com>]

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 <linux@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 ?

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.