[watchdog-next] watchdog: kill unref/ref ops

[watchdog-next] watchdog: kill unref/ref ops

[Tomas Winkler]

ref/unref ops are not called at all so even marked them as deprecated
is misleading, we need to just drop the API.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 include/linux/watchdog.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 850af04fe0c7..aaabd4703b46 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -47,8 +47,6 @@ struct watchdog_ops {
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
 	unsigned int (*get_timeleft)(struct watchdog_device *);
 	int (*restart)(struct watchdog_device *);
-	void (*ref)(struct watchdog_device *) __deprecated;
-	void (*unref)(struct watchdog_device *) __deprecated;
 	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
-- 
2.4.3

Re: [watchdog-next] watchdog: kill unref/ref ops

[Guenter Roeck]

On 01/03/2016 03:32 AM, Tomas Winkler wrote:
> ref/unref ops are not called at all so even marked them as deprecated
> is misleading, we need to just drop the API.
>
One the other side, the functions are now no-ops, so they don't hurt either.
Only reason for not dropping the functions as part of my series was that
the mei watchdog code was starting to implement them, and I didn't want
to create dependencies for that code. Maybe my thinking was too conservative.

> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

... of course that is no concern anymore since this patch is from its
maintainer.

Acked-by: Guenter Roeck <linux@roeck-us.net>

Thanks!
Guenter

> ---
>   include/linux/watchdog.h | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 850af04fe0c7..aaabd4703b46 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -47,8 +47,6 @@ struct watchdog_ops {
>   	int (*set_timeout)(struct watchdog_device *, unsigned int);
>   	unsigned int (*get_timeleft)(struct watchdog_device *);
>   	int (*restart)(struct watchdog_device *);
> -	void (*ref)(struct watchdog_device *) __deprecated;
> -	void (*unref)(struct watchdog_device *) __deprecated;
>   	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>   };
>
>

RE: [watchdog-next] watchdog: kill unref/ref ops

[Winkler, Tomas]

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: Sunday, January 03, 2016 17:48
> To: Winkler, Tomas; Wim Van Sebroeck
> Cc: linux-watchdog@vger.kernel.org
> Subject: Re: [watchdog-next] watchdog: kill unref/ref ops
> 
> On 01/03/2016 03:32 AM, Tomas Winkler wrote:
> > ref/unref ops are not called at all so even marked them as deprecated
> > is misleading, we need to just drop the API.
> >
> One the other side, the functions are now no-ops, so they don't hurt either.
> Only reason for not dropping the functions as part of my series was that
> the mei watchdog code was starting to implement them, and I didn't want
> to create dependencies for that code. Maybe my thinking was too conservative.
> 
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> ... of course that is no concern anymore since this patch is from its
> maintainer.
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>

Now I'm looking into the current code and one thing I do not understand , if the watchdog_core_data is holding
a pointer to wdd, who is responsible for wdd and who is holding the reference counter on that pointer.
If the underlying driver will release the wdd (for any reason) your code will read junk. 
I think wdd should not be a pointer here.

/*
 * struct watchdog_core_data - watchdog core internal data
 * @kref:       Reference count.
 * @cdev:       The watchdog's Character device.
 * @wdd:        Pointer to watchdog device.
 * @lock:       Lock for watchdog core.
 * @status:     Watchdog core internal status bits.
 */
struct watchdog_core_data {
        struct kref kref;
        struct cdev cdev;
        struct watchdog_device *wdd;
        struct mutex lock;
        unsigned long status;           /* Internal status bits */
#define _WDOG_DEV_OPEN          0       /* Opened ? */
#define _WDOG_ALLOW_RELEASE     1       /* Did we receive the magic char ? */
};

Thanks
Tomas 

Re: [watchdog-next] watchdog: kill unref/ref ops

[Guenter Roeck]

On 01/04/2016 03:38 PM, Winkler, Tomas wrote:
>
>
>> -----Original Message-----
>> From: Guenter Roeck [mailto:linux@roeck-us.net]
>> Sent: Sunday, January 03, 2016 17:48
>> To: Winkler, Tomas; Wim Van Sebroeck
>> Cc: linux-watchdog@vger.kernel.org
>> Subject: Re: [watchdog-next] watchdog: kill unref/ref ops
>>
>> On 01/03/2016 03:32 AM, Tomas Winkler wrote:
>>> ref/unref ops are not called at all so even marked them as deprecated
>>> is misleading, we need to just drop the API.
>>>
>> One the other side, the functions are now no-ops, so they don't hurt either.
>> Only reason for not dropping the functions as part of my series was that
>> the mei watchdog code was starting to implement them, and I didn't want
>> to create dependencies for that code. Maybe my thinking was too conservative.
>>
>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>>
>> ... of course that is no concern anymore since this patch is from its
>> maintainer.
>>
>> Acked-by: Guenter Roeck <linux@roeck-us.net>
>
> Now I'm looking into the current code and one thing I do not understand , if the watchdog_core_data is holding
> a pointer to wdd, who is responsible for wdd and who is holding the reference counter on that pointer.
> If the underlying driver will release the wdd (for any reason) your code will read junk.
> I think wdd should not be a pointer here.
>
The pointer is cleared when the watchdog device is removed (in watchdog_cdev_unregister,
which is called from watchdog_dev_unregister), and the cleared pointer is used by
the watchdog core as indication that the driver is no longer accessible.
This replaces the WDOG_UNREGISTERED flag.

Guenter

> /*
>   * struct watchdog_core_data - watchdog core internal data
>   * @kref:       Reference count.
>   * @cdev:       The watchdog's Character device.
>   * @wdd:        Pointer to watchdog device.
>   * @lock:       Lock for watchdog core.
>   * @status:     Watchdog core internal status bits.
>   */
> struct watchdog_core_data {
>          struct kref kref;
>          struct cdev cdev;
>          struct watchdog_device *wdd;
>          struct mutex lock;
>          unsigned long status;           /* Internal status bits */
> #define _WDOG_DEV_OPEN          0       /* Opened ? */
> #define _WDOG_ALLOW_RELEASE     1       /* Did we receive the magic char ? */
> };
>
> Thanks
> Tomas
>

Re: [watchdog-next] watchdog: kill unref/ref ops

[Wim Van Sebroeck]

Hi Tomas,

> ref/unref ops are not called at all so even marked them as deprecated
> is misleading, we need to just drop the API.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  include/linux/watchdog.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 850af04fe0c7..aaabd4703b46 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -47,8 +47,6 @@ struct watchdog_ops {
>  	int (*set_timeout)(struct watchdog_device *, unsigned int);
>  	unsigned int (*get_timeleft)(struct watchdog_device *);
>  	int (*restart)(struct watchdog_device *);
> -	void (*ref)(struct watchdog_device *) __deprecated;
> -	void (*unref)(struct watchdog_device *) __deprecated;
>  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>  };
>  
> -- 
> 2.4.3
> 

This patch has been added to linux-watchdog-next.

Kind regards,
Wim.