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

[Guenter Roeck <linux@roeck-us.net>]

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
>