subject:"\[PATCH 2\/5\] watchdog\: Separate and maintain variables based on variable lifetime"

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Guenter Roeck


On 12/22/2015 02:05 PM, Tomas Winkler wrote:



 > Do you see a situation where holding the lock between calls into the driver
 > might be a problem ?

I don't think u are holding the lock now in watchdog_unregister when 
WDOG_UNREGISTERED was dropped.


the lock is held while clearing the pointers:

mutex_lock(_data->lock);
wd_data->wdd = NULL;
wdd->wd_data = NULL;
mutex_unlock(_data->lock);

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Guenter Roeck


On 12/22/2015 11:28 AM, Damien Riegel wrote:

On Tue, Dec 22, 2015 at 08:22:40AM -0800, Guenter Roeck wrote:

On 12/22/2015 08:09 AM, Damien Riegel wrote:

On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:

On 12/21/2015 09:28 AM, Damien Riegel wrote:

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.


It should not have caused a panic to start with, but the ref/unref functions
for the most part were either not or wrongly implemented. Not really
surprising - it took me a while to understand the problem.


I tested on a driver which did not implement ref/unref. When ping is
called, it tries to dereference a freed 'struct watchdog_device' in
watchdog_get_drvdata, leading to a panic.


Yes, that will happen. Problem here is that the driver is buggy -
pretty much all drivers which dynamically allocate struct watchdog_device
have this problem.

This is the ultimate reason for coming up with this patch.



[ ... ]



  /*
+ * struct _watchdog_device - watchdog core internal data


Think it should be /**. Anyway, I find it confusing to have both
_watchdog_device and watchdog_device, but I can't think of a better
name right now.


I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
since it is only used there. No '**', though, because it is not a published
API, but just an internal data structure.

I also renamed the matching variable name to 'wd_data' (from '_wdd').


Okay. Also, why didn't you use the explicit type for 'wdd_data' in
'struct watchdog_device' instead of a void*?



This is to hide the data type, since the structure is not exported
to drivers.

I could pre-declare the structure with 'struct watchdog_data;', but
then I'd have to use a different structure name (watchdog_cdev_data,
maybe, or watchdog_core_data) to make it less generic. Any opinion ?
Would that be better / preferred ? I am 50/50 about it.


My personal preference would be to be explicit. That makes code
nagivation easier and it might help the compiler catch some mistakes.
Plus, as you moved the structure definition in watchdog_dev.c, it is
very clear that drivers aren't supposed to use it.



Ok, makes sense. I'll do that.

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Damien Riegel

On Tue, Dec 22, 2015 at 08:22:40AM -0800, Guenter Roeck wrote:
> On 12/22/2015 08:09 AM, Damien Riegel wrote:
> >On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
> >>On 12/21/2015 09:28 AM, Damien Riegel wrote:
> >>>On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> All variables required by the watchdog core to manage a watchdog are
> currently stored in struct watchdog_device. The lifetime of those
> variables is determined by the watchdog driver. However, the lifetime
> of variables used by the watchdog core differs from the lifetime of
> struct watchdog_device. To remedy this situation, watchdog drivers
> can implement ref and unref callbacks, to be used by the watchdog
> core to lock struct watchdog_device in memory.
> 
> While this solves the immediate problem, it depends on watchdog drivers
> to actually implement the ref/unref callbacks. This is error prone,
> often not implemented in the first place, or not implemented correctly.
> 
> To solve the problem without requiring driver support, split the variables
> in struct watchdog_device into two data structures - one for variables
> associated with the watchdog driver, one for variables associated with
> the watchdog core. With this approach, the watchdog core can keep track
> of its variable lifetime and no longer depends on ref/unref callbacks
> in the driver. As a side effect, some of the variables originally in
> struct watchdog_driver are now private to the watchdog core and no longer
> visible in watchdog drivers.
> 
> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> used and marked as deprecated.
> >>>
> >>>Two comments below. It's great to see that unbinding a driver no longer
> >>>triggers a kernel panic.
> >>>
> >>It should not have caused a panic to start with, but the ref/unref functions
> >>for the most part were either not or wrongly implemented. Not really
> >>surprising - it took me a while to understand the problem.
> >
> >I tested on a driver which did not implement ref/unref. When ping is
> >called, it tries to dereference a freed 'struct watchdog_device' in
> >watchdog_get_drvdata, leading to a panic.
> >
> Yes, that will happen. Problem here is that the driver is buggy -
> pretty much all drivers which dynamically allocate struct watchdog_device
> have this problem.
> 
> This is the ultimate reason for coming up with this patch.
> 
> >>
> >>[ ... ]
> >>
> 
>   /*
> + * struct _watchdog_device - watchdog core internal data
> >>>
> >>>Think it should be /**. Anyway, I find it confusing to have both
> >>>_watchdog_device and watchdog_device, but I can't think of a better
> >>>name right now.
> >>
> >>I renamed the data structure to watchdog_data and moved it into 
> >>watchdog_dev.c
> >>since it is only used there. No '**', though, because it is not a published
> >>API, but just an internal data structure.
> >>
> >>I also renamed the matching variable name to 'wd_data' (from '_wdd').
> >
> >Okay. Also, why didn't you use the explicit type for 'wdd_data' in
> >'struct watchdog_device' instead of a void*?
> >
> 
> This is to hide the data type, since the structure is not exported
> to drivers.
> 
> I could pre-declare the structure with 'struct watchdog_data;', but
> then I'd have to use a different structure name (watchdog_cdev_data,
> maybe, or watchdog_core_data) to make it less generic. Any opinion ?
> Would that be better / preferred ? I am 50/50 about it.

My personal preference would be to be explicit. That makes code
nagivation easier and it might help the compiler catch some mistakes.
Plus, as you moved the structure definition in watchdog_dev.c, it is
very clear that drivers aren't supposed to use it.

Thanks,
Damien
--
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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Guenter Roeck


On 12/22/2015 08:09 AM, Damien Riegel wrote:

On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:

On 12/21/2015 09:28 AM, Damien Riegel wrote:

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.


It should not have caused a panic to start with, but the ref/unref functions
for the most part were either not or wrongly implemented. Not really
surprising - it took me a while to understand the problem.


I tested on a driver which did not implement ref/unref. When ping is
called, it tries to dereference a freed 'struct watchdog_device' in
watchdog_get_drvdata, leading to a panic.


Yes, that will happen. Problem here is that the driver is buggy -
pretty much all drivers which dynamically allocate struct watchdog_device
have this problem.

This is the ultimate reason for coming up with this patch.



[ ... ]



  /*
+ * struct _watchdog_device - watchdog core internal data


Think it should be /**. Anyway, I find it confusing to have both
_watchdog_device and watchdog_device, but I can't think of a better
name right now.


I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
since it is only used there. No '**', though, because it is not a published
API, but just an internal data structure.

I also renamed the matching variable name to 'wd_data' (from '_wdd').


Okay. Also, why didn't you use the explicit type for 'wdd_data' in
'struct watchdog_device' instead of a void*?



This is to hide the data type, since the structure is not exported
to drivers.

I could pre-declare the structure with 'struct watchdog_data;', but
then I'd have to use a different structure name (watchdog_cdev_data,
maybe, or watchdog_core_data) to make it less generic. Any opinion ?
Would that be better / preferred ? I am 50/50 about it.

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Damien Riegel

On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
> On 12/21/2015 09:28 AM, Damien Riegel wrote:
> >On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> >>All variables required by the watchdog core to manage a watchdog are
> >>currently stored in struct watchdog_device. The lifetime of those
> >>variables is determined by the watchdog driver. However, the lifetime
> >>of variables used by the watchdog core differs from the lifetime of
> >>struct watchdog_device. To remedy this situation, watchdog drivers
> >>can implement ref and unref callbacks, to be used by the watchdog
> >>core to lock struct watchdog_device in memory.
> >>
> >>While this solves the immediate problem, it depends on watchdog drivers
> >>to actually implement the ref/unref callbacks. This is error prone,
> >>often not implemented in the first place, or not implemented correctly.
> >>
> >>To solve the problem without requiring driver support, split the variables
> >>in struct watchdog_device into two data structures - one for variables
> >>associated with the watchdog driver, one for variables associated with
> >>the watchdog core. With this approach, the watchdog core can keep track
> >>of its variable lifetime and no longer depends on ref/unref callbacks
> >>in the driver. As a side effect, some of the variables originally in
> >>struct watchdog_driver are now private to the watchdog core and no longer
> >>visible in watchdog drivers.
> >>
> >>The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> >>used and marked as deprecated.
> >
> >Two comments below. It's great to see that unbinding a driver no longer
> >triggers a kernel panic.
> >
> It should not have caused a panic to start with, but the ref/unref functions
> for the most part were either not or wrongly implemented. Not really
> surprising - it took me a while to understand the problem.

I tested on a driver which did not implement ref/unref. When ping is
called, it tries to dereference a freed 'struct watchdog_device' in
watchdog_get_drvdata, leading to a panic. 

> 
> [ ... ]
> 
> >>
> >>  /*
> >>+ * struct _watchdog_device - watchdog core internal data
> >
> >Think it should be /**. Anyway, I find it confusing to have both
> >_watchdog_device and watchdog_device, but I can't think of a better
> >name right now.
> 
> I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
> since it is only used there. No '**', though, because it is not a published
> API, but just an internal data structure.
> 
> I also renamed the matching variable name to 'wd_data' (from '_wdd').

Okay. Also, why didn't you use the explicit type for 'wdd_data' in
'struct watchdog_device' instead of a void*?

> 
> >>
> >>  static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> >>  {
> >>-   mutex_lock(>lock);
> >>-   set_bit(WDOG_UNREGISTERED, >status);
> >>-   mutex_unlock(>lock);
> >>+   struct _watchdog_device *_wdd = wdd->wdd_data;
> >>
> >>-   cdev_del(>cdev);
> >>+   cdev_del(&_wdd->cdev);
> >>if (wdd->id == 0) {
> >>misc_deregister(_miscdev);
> >>-   old_wdd = NULL;
> >>+   _old_wdd = NULL;
> >>}
> >>+
> >>+   if (watchdog_active(wdd))
> >>+   pr_crit("watchdog%d: watchdog still running!\n", wdd->id);
> >
> >As it is now safe to unbind and rebind a driver, it means that a
> >watchdog driver probe function can now be called with a running
> >watchdog. Some drivers handle this situation, but I think that most of
> >them expect the watchdog to be off at this point.
> >
> No semantics change, though, and no change in behavior. Drivers _should_
> handle that situation today. Sure, many don't, but that is a different issue.

All right, that's what confused me. It was, and still will be, driver
responsiblity to handle this situation.

> 
> I'll address handling an already-running watchdog by the watchdog core until
> the character device is opened in a separate patch set, but we'll have to have
> this series accepted before I re-introduce that. Even with that, it will still
> be the driver's responsibility to detect and report that/if a watchdog is
> already running.
> 
> Thanks,
> Guenter
> 

Thanks,
Damien
--
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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Damien Riegel

On Tue, Dec 22, 2015 at 08:22:40AM -0800, Guenter Roeck wrote:
> On 12/22/2015 08:09 AM, Damien Riegel wrote:
> >On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
> >>On 12/21/2015 09:28 AM, Damien Riegel wrote:
> >>>On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> All variables required by the watchdog core to manage a watchdog are
> currently stored in struct watchdog_device. The lifetime of those
> variables is determined by the watchdog driver. However, the lifetime
> of variables used by the watchdog core differs from the lifetime of
> struct watchdog_device. To remedy this situation, watchdog drivers
> can implement ref and unref callbacks, to be used by the watchdog
> core to lock struct watchdog_device in memory.
> 
> While this solves the immediate problem, it depends on watchdog drivers
> to actually implement the ref/unref callbacks. This is error prone,
> often not implemented in the first place, or not implemented correctly.
> 
> To solve the problem without requiring driver support, split the variables
> in struct watchdog_device into two data structures - one for variables
> associated with the watchdog driver, one for variables associated with
> the watchdog core. With this approach, the watchdog core can keep track
> of its variable lifetime and no longer depends on ref/unref callbacks
> in the driver. As a side effect, some of the variables originally in
> struct watchdog_driver are now private to the watchdog core and no longer
> visible in watchdog drivers.
> 
> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> used and marked as deprecated.
> >>>
> >>>Two comments below. It's great to see that unbinding a driver no longer
> >>>triggers a kernel panic.
> >>>
> >>It should not have caused a panic to start with, but the ref/unref functions
> >>for the most part were either not or wrongly implemented. Not really
> >>surprising - it took me a while to understand the problem.
> >
> >I tested on a driver which did not implement ref/unref. When ping is
> >called, it tries to dereference a freed 'struct watchdog_device' in
> >watchdog_get_drvdata, leading to a panic.
> >
> Yes, that will happen. Problem here is that the driver is buggy -
> pretty much all drivers which dynamically allocate struct watchdog_device
> have this problem.
> 
> This is the ultimate reason for coming up with this patch.
> 
> >>
> >>[ ... ]
> >>
> 
>   /*
> + * struct _watchdog_device - watchdog core internal data
> >>>
> >>>Think it should be /**. Anyway, I find it confusing to have both
> >>>_watchdog_device and watchdog_device, but I can't think of a better
> >>>name right now.
> >>
> >>I renamed the data structure to watchdog_data and moved it into 
> >>watchdog_dev.c
> >>since it is only used there. No '**', though, because it is not a published
> >>API, but just an internal data structure.
> >>
> >>I also renamed the matching variable name to 'wd_data' (from '_wdd').
> >
> >Okay. Also, why didn't you use the explicit type for 'wdd_data' in
> >'struct watchdog_device' instead of a void*?
> >
> 
> This is to hide the data type, since the structure is not exported
> to drivers.
> 
> I could pre-declare the structure with 'struct watchdog_data;', but
> then I'd have to use a different structure name (watchdog_cdev_data,
> maybe, or watchdog_core_data) to make it less generic. Any opinion ?
> Would that be better / preferred ? I am 50/50 about it.

My personal preference would be to be explicit. That makes code
nagivation easier and it might help the compiler catch some mistakes.
Plus, as you moved the structure definition in watchdog_dev.c, it is
very clear that drivers aren't supposed to use it.

Thanks,
Damien
--
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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Guenter Roeck


On 12/22/2015 02:05 PM, Tomas Winkler wrote:



 > Do you see a situation where holding the lock between calls into the driver
 > might be a problem ?

I don't think u are holding the lock now in watchdog_unregister when 
WDOG_UNREGISTERED was dropped.


the lock is held while clearing the pointers:

mutex_lock(_data->lock);
wd_data->wdd = NULL;
wdd->wd_data = NULL;
mutex_unlock(_data->lock);

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Damien Riegel

On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
> On 12/21/2015 09:28 AM, Damien Riegel wrote:
> >On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> >>All variables required by the watchdog core to manage a watchdog are
> >>currently stored in struct watchdog_device. The lifetime of those
> >>variables is determined by the watchdog driver. However, the lifetime
> >>of variables used by the watchdog core differs from the lifetime of
> >>struct watchdog_device. To remedy this situation, watchdog drivers
> >>can implement ref and unref callbacks, to be used by the watchdog
> >>core to lock struct watchdog_device in memory.
> >>
> >>While this solves the immediate problem, it depends on watchdog drivers
> >>to actually implement the ref/unref callbacks. This is error prone,
> >>often not implemented in the first place, or not implemented correctly.
> >>
> >>To solve the problem without requiring driver support, split the variables
> >>in struct watchdog_device into two data structures - one for variables
> >>associated with the watchdog driver, one for variables associated with
> >>the watchdog core. With this approach, the watchdog core can keep track
> >>of its variable lifetime and no longer depends on ref/unref callbacks
> >>in the driver. As a side effect, some of the variables originally in
> >>struct watchdog_driver are now private to the watchdog core and no longer
> >>visible in watchdog drivers.
> >>
> >>The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> >>used and marked as deprecated.
> >
> >Two comments below. It's great to see that unbinding a driver no longer
> >triggers a kernel panic.
> >
> It should not have caused a panic to start with, but the ref/unref functions
> for the most part were either not or wrongly implemented. Not really
> surprising - it took me a while to understand the problem.

I tested on a driver which did not implement ref/unref. When ping is
called, it tries to dereference a freed 'struct watchdog_device' in
watchdog_get_drvdata, leading to a panic. 

> 
> [ ... ]
> 
> >>
> >>  /*
> >>+ * struct _watchdog_device - watchdog core internal data
> >
> >Think it should be /**. Anyway, I find it confusing to have both
> >_watchdog_device and watchdog_device, but I can't think of a better
> >name right now.
> 
> I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
> since it is only used there. No '**', though, because it is not a published
> API, but just an internal data structure.
> 
> I also renamed the matching variable name to 'wd_data' (from '_wdd').

Okay. Also, why didn't you use the explicit type for 'wdd_data' in
'struct watchdog_device' instead of a void*?

> 
> >>
> >>  static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> >>  {
> >>-   mutex_lock(>lock);
> >>-   set_bit(WDOG_UNREGISTERED, >status);
> >>-   mutex_unlock(>lock);
> >>+   struct _watchdog_device *_wdd = wdd->wdd_data;
> >>
> >>-   cdev_del(>cdev);
> >>+   cdev_del(&_wdd->cdev);
> >>if (wdd->id == 0) {
> >>misc_deregister(_miscdev);
> >>-   old_wdd = NULL;
> >>+   _old_wdd = NULL;
> >>}
> >>+
> >>+   if (watchdog_active(wdd))
> >>+   pr_crit("watchdog%d: watchdog still running!\n", wdd->id);
> >
> >As it is now safe to unbind and rebind a driver, it means that a
> >watchdog driver probe function can now be called with a running
> >watchdog. Some drivers handle this situation, but I think that most of
> >them expect the watchdog to be off at this point.
> >
> No semantics change, though, and no change in behavior. Drivers _should_
> handle that situation today. Sure, many don't, but that is a different issue.

All right, that's what confused me. It was, and still will be, driver
responsiblity to handle this situation.

> 
> I'll address handling an already-running watchdog by the watchdog core until
> the character device is opened in a separate patch set, but we'll have to have
> this series accepted before I re-introduce that. Even with that, it will still
> be the driver's responsibility to detect and report that/if a watchdog is
> already running.
> 
> Thanks,
> Guenter
> 

Thanks,
Damien
--
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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Guenter Roeck


On 12/22/2015 08:09 AM, Damien Riegel wrote:

On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:

On 12/21/2015 09:28 AM, Damien Riegel wrote:

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.


It should not have caused a panic to start with, but the ref/unref functions
for the most part were either not or wrongly implemented. Not really
surprising - it took me a while to understand the problem.


I tested on a driver which did not implement ref/unref. When ping is
called, it tries to dereference a freed 'struct watchdog_device' in
watchdog_get_drvdata, leading to a panic.


Yes, that will happen. Problem here is that the driver is buggy -
pretty much all drivers which dynamically allocate struct watchdog_device
have this problem.

This is the ultimate reason for coming up with this patch.



[ ... ]



  /*
+ * struct _watchdog_device - watchdog core internal data


Think it should be /**. Anyway, I find it confusing to have both
_watchdog_device and watchdog_device, but I can't think of a better
name right now.


I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
since it is only used there. No '**', though, because it is not a published
API, but just an internal data structure.

I also renamed the matching variable name to 'wd_data' (from '_wdd').


Okay. Also, why didn't you use the explicit type for 'wdd_data' in
'struct watchdog_device' instead of a void*?



This is to hide the data type, since the structure is not exported
to drivers.

I could pre-declare the structure with 'struct watchdog_data;', but
then I'd have to use a different structure name (watchdog_cdev_data,
maybe, or watchdog_core_data) to make it less generic. Any opinion ?
Would that be better / preferred ? I am 50/50 about it.

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-22 Thread Guenter Roeck


On 12/22/2015 11:28 AM, Damien Riegel wrote:

On Tue, Dec 22, 2015 at 08:22:40AM -0800, Guenter Roeck wrote:

On 12/22/2015 08:09 AM, Damien Riegel wrote:

On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:

On 12/21/2015 09:28 AM, Damien Riegel wrote:

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.


It should not have caused a panic to start with, but the ref/unref functions
for the most part were either not or wrongly implemented. Not really
surprising - it took me a while to understand the problem.


I tested on a driver which did not implement ref/unref. When ping is
called, it tries to dereference a freed 'struct watchdog_device' in
watchdog_get_drvdata, leading to a panic.


Yes, that will happen. Problem here is that the driver is buggy -
pretty much all drivers which dynamically allocate struct watchdog_device
have this problem.

This is the ultimate reason for coming up with this patch.



[ ... ]



  /*
+ * struct _watchdog_device - watchdog core internal data


Think it should be /**. Anyway, I find it confusing to have both
_watchdog_device and watchdog_device, but I can't think of a better
name right now.


I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
since it is only used there. No '**', though, because it is not a published
API, but just an internal data structure.

I also renamed the matching variable name to 'wd_data' (from '_wdd').


Okay. Also, why didn't you use the explicit type for 'wdd_data' in
'struct watchdog_device' instead of a void*?



This is to hide the data type, since the structure is not exported
to drivers.

I could pre-declare the structure with 'struct watchdog_data;', but
then I'd have to use a different structure name (watchdog_cdev_data,
maybe, or watchdog_core_data) to make it less generic. Any opinion ?
Would that be better / preferred ? I am 50/50 about it.


My personal preference would be to be explicit. That makes code
nagivation easier and it might help the compiler catch some mistakes.
Plus, as you moved the structure definition in watchdog_dev.c, it is
very clear that drivers aren't supposed to use it.



Ok, makes sense. I'll do that.

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Guenter Roeck


On 12/21/2015 03:36 PM, Tomas Winkler wrote:

On Mon, Dec 21, 2015 at 7:28 PM, Damien Riegel
 wrote:


On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.



Signed-off-by: Guenter Roeck 
---
  Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
  drivers/watchdog/watchdog_core.c   |   2 -
  drivers/watchdog/watchdog_core.h   |  23 ++
  drivers/watchdog/watchdog_dev.c| 377 +
  include/linux/watchdog.h   |  21 +-
  5 files changed, 239 insertions(+), 229 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
b/Documentation/watchdog/watchdog-kernel-api.txt
index 0a37da76acef..3db5092924e5 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -44,7 +44,6 @@ The watchdog device structure looks like this:

  struct watchdog_device {
   int id;
- struct cdev cdev;
   struct device *dev;
   struct device *parent;
   const struct watchdog_info *info;
@@ -56,7 +55,7 @@ struct watchdog_device {
   struct notifier_block reboot_nb;
   struct notifier_block restart_nb;
   void *driver_data;
- struct mutex lock;
+ void *wdd_data;
   unsigned long status;
   struct list_head deferred;
  };
@@ -66,8 +65,6 @@ It contains following fields:
/dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
/dev/watchdog miscdev. The id is set automatically when calling
watchdog_register_device.
-* cdev: cdev for the dynamic /dev/watchdog device nodes. This
-  field is also populated by watchdog_register_device.
  * dev: device under the watchdog class (created by watchdog_register_device).
  * parent: set this to the parent device (or NULL) before calling
watchdog_register_device.
@@ -89,11 +86,10 @@ It contains following fields:
  * driver_data: a pointer to the drivers private data of a watchdog device.
This data should only be accessed via the watchdog_set_drvdata and
watchdog_get_drvdata routines.
-* lock: Mutex for WatchDog Timer Driver Core internal use only.
+* wdd_data: a pointer to watchdog core internal data.
  * status: this field contains a number of status bits that give extra
information about the status of the device (Like: is the watchdog timer
-  running/active, is the nowayout bit set, is the device opened via
-  the /dev/watchdog interface or not, ...).
+  running/active, or is the nowayout bit set).
  * deferred: entry in wtd_deferred_reg_list which is used to
register early initialized watchdogs.

@@ -110,8 +106,8 @@ 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 *);
- void (*unref)(struct watchdog_device *);
+ void (*ref)(struct watchdog_device *) __deprecated;
+ void (*unref)(struct watchdog_device *) __deprecated;
   long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
  };

@@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
lock the module when
  the watchdog is active. (This to avoid a system crash when you unload the
  module and /dev/watchdog is still open).

-If the watchdog_device struct is dynamically allocated, just locking the module
-is not enough and a driver also needs to define the ref and unref operations to
-ensure the structure holding the watchdog_device does not go away.
-
-The simplest (and usually

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Guenter Roeck


On 12/21/2015 09:28 AM, Damien Riegel wrote:

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.


It should not have caused a panic to start with, but the ref/unref functions
for the most part were either not or wrongly implemented. Not really
surprising - it took me a while to understand the problem.

[ ... ]



  /*
+ * struct _watchdog_device - watchdog core internal data


Think it should be /**. Anyway, I find it confusing to have both
_watchdog_device and watchdog_device, but I can't think of a better
name right now.


I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
since it is only used there. No '**', though, because it is not a published
API, but just an internal data structure.

I also renamed the matching variable name to 'wd_data' (from '_wdd').



  static void watchdog_cdev_unregister(struct watchdog_device *wdd)
  {
-   mutex_lock(>lock);
-   set_bit(WDOG_UNREGISTERED, >status);
-   mutex_unlock(>lock);
+   struct _watchdog_device *_wdd = wdd->wdd_data;

-   cdev_del(>cdev);
+   cdev_del(&_wdd->cdev);
if (wdd->id == 0) {
misc_deregister(_miscdev);
-   old_wdd = NULL;
+   _old_wdd = NULL;
}
+
+   if (watchdog_active(wdd))
+   pr_crit("watchdog%d: watchdog still running!\n", wdd->id);


As it is now safe to unbind and rebind a driver, it means that a
watchdog driver probe function can now be called with a running
watchdog. Some drivers handle this situation, but I think that most of
them expect the watchdog to be off at this point.


No semantics change, though, and no change in behavior. Drivers _should_
handle that situation today. Sure, many don't, but that is a different issue.

I'll address handling an already-running watchdog by the watchdog core until
the character device is opened in a separate patch set, but we'll have to have
this series accepted before I re-introduce that. Even with that, it will still
be the driver's responsibility to detect and report that/if a watchdog is
already running.

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Tomas Winkler

On Mon, Dec 21, 2015 at 7:28 PM, Damien Riegel
 wrote:
>
> On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> > All variables required by the watchdog core to manage a watchdog are
> > currently stored in struct watchdog_device. The lifetime of those
> > variables is determined by the watchdog driver. However, the lifetime
> > of variables used by the watchdog core differs from the lifetime of
> > struct watchdog_device. To remedy this situation, watchdog drivers
> > can implement ref and unref callbacks, to be used by the watchdog
> > core to lock struct watchdog_device in memory.
> >
> > While this solves the immediate problem, it depends on watchdog drivers
> > to actually implement the ref/unref callbacks. This is error prone,
> > often not implemented in the first place, or not implemented correctly.
> >
> > To solve the problem without requiring driver support, split the variables
> > in struct watchdog_device into two data structures - one for variables
> > associated with the watchdog driver, one for variables associated with
> > the watchdog core. With this approach, the watchdog core can keep track
> > of its variable lifetime and no longer depends on ref/unref callbacks
> > in the driver. As a side effect, some of the variables originally in
> > struct watchdog_driver are now private to the watchdog core and no longer
> > visible in watchdog drivers.
> >
> > The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> > used and marked as deprecated.
>
> Two comments below. It's great to see that unbinding a driver no longer
> triggers a kernel panic.
>
> >
> > Signed-off-by: Guenter Roeck 
> > ---
> >  Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
> >  drivers/watchdog/watchdog_core.c   |   2 -
> >  drivers/watchdog/watchdog_core.h   |  23 ++
> >  drivers/watchdog/watchdog_dev.c| 377 
> > +
> >  include/linux/watchdog.h   |  21 +-
> >  5 files changed, 239 insertions(+), 229 deletions(-)
> >
> > diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
> > b/Documentation/watchdog/watchdog-kernel-api.txt
> > index 0a37da76acef..3db5092924e5 100644
> > --- a/Documentation/watchdog/watchdog-kernel-api.txt
> > +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> > @@ -44,7 +44,6 @@ The watchdog device structure looks like this:
> >
> >  struct watchdog_device {
> >   int id;
> > - struct cdev cdev;
> >   struct device *dev;
> >   struct device *parent;
> >   const struct watchdog_info *info;
> > @@ -56,7 +55,7 @@ struct watchdog_device {
> >   struct notifier_block reboot_nb;
> >   struct notifier_block restart_nb;
> >   void *driver_data;
> > - struct mutex lock;
> > + void *wdd_data;
> >   unsigned long status;
> >   struct list_head deferred;
> >  };
> > @@ -66,8 +65,6 @@ It contains following fields:
> >/dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
> >/dev/watchdog miscdev. The id is set automatically when calling
> >watchdog_register_device.
> > -* cdev: cdev for the dynamic /dev/watchdog device nodes. This
> > -  field is also populated by watchdog_register_device.
> >  * dev: device under the watchdog class (created by 
> > watchdog_register_device).
> >  * parent: set this to the parent device (or NULL) before calling
> >watchdog_register_device.
> > @@ -89,11 +86,10 @@ It contains following fields:
> >  * driver_data: a pointer to the drivers private data of a watchdog device.
> >This data should only be accessed via the watchdog_set_drvdata and
> >watchdog_get_drvdata routines.
> > -* lock: Mutex for WatchDog Timer Driver Core internal use only.
> > +* wdd_data: a pointer to watchdog core internal data.
> >  * status: this field contains a number of status bits that give extra
> >information about the status of the device (Like: is the watchdog timer
> > -  running/active, is the nowayout bit set, is the device opened via
> > -  the /dev/watchdog interface or not, ...).
> > +  running/active, or is the nowayout bit set).
> >  * deferred: entry in wtd_deferred_reg_list which is used to
> >register early initialized watchdogs.
> >
> > @@ -110,8 +106,8 @@ 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 *);
> > - void (*unref)(struct watchdog_device *);
> > + void (*ref)(struct watchdog_device *) __deprecated;
> > + void (*unref)(struct watchdog_device *) __deprecated;
> >   long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> >  };
> >
> > @@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
> > lock the module when
> >  the watchdog is active. (This to avoid a system crash when you unload the
> >  module

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Damien Riegel

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> All variables required by the watchdog core to manage a watchdog are
> currently stored in struct watchdog_device. The lifetime of those
> variables is determined by the watchdog driver. However, the lifetime
> of variables used by the watchdog core differs from the lifetime of
> struct watchdog_device. To remedy this situation, watchdog drivers
> can implement ref and unref callbacks, to be used by the watchdog
> core to lock struct watchdog_device in memory.
> 
> While this solves the immediate problem, it depends on watchdog drivers
> to actually implement the ref/unref callbacks. This is error prone,
> often not implemented in the first place, or not implemented correctly.
> 
> To solve the problem without requiring driver support, split the variables
> in struct watchdog_device into two data structures - one for variables
> associated with the watchdog driver, one for variables associated with
> the watchdog core. With this approach, the watchdog core can keep track
> of its variable lifetime and no longer depends on ref/unref callbacks
> in the driver. As a side effect, some of the variables originally in
> struct watchdog_driver are now private to the watchdog core and no longer
> visible in watchdog drivers.
> 
> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> used and marked as deprecated.

Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.

> 
> Signed-off-by: Guenter Roeck 
> ---
>  Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
>  drivers/watchdog/watchdog_core.c   |   2 -
>  drivers/watchdog/watchdog_core.h   |  23 ++
>  drivers/watchdog/watchdog_dev.c| 377 
> +
>  include/linux/watchdog.h   |  21 +-
>  5 files changed, 239 insertions(+), 229 deletions(-)
> 
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
> b/Documentation/watchdog/watchdog-kernel-api.txt
> index 0a37da76acef..3db5092924e5 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -44,7 +44,6 @@ The watchdog device structure looks like this:
>  
>  struct watchdog_device {
>   int id;
> - struct cdev cdev;
>   struct device *dev;
>   struct device *parent;
>   const struct watchdog_info *info;
> @@ -56,7 +55,7 @@ struct watchdog_device {
>   struct notifier_block reboot_nb;
>   struct notifier_block restart_nb;
>   void *driver_data;
> - struct mutex lock;
> + void *wdd_data;
>   unsigned long status;
>   struct list_head deferred;
>  };
> @@ -66,8 +65,6 @@ It contains following fields:
>/dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
>/dev/watchdog miscdev. The id is set automatically when calling
>watchdog_register_device.
> -* cdev: cdev for the dynamic /dev/watchdog device nodes. This
> -  field is also populated by watchdog_register_device.
>  * dev: device under the watchdog class (created by watchdog_register_device).
>  * parent: set this to the parent device (or NULL) before calling
>watchdog_register_device.
> @@ -89,11 +86,10 @@ It contains following fields:
>  * driver_data: a pointer to the drivers private data of a watchdog device.
>This data should only be accessed via the watchdog_set_drvdata and
>watchdog_get_drvdata routines.
> -* lock: Mutex for WatchDog Timer Driver Core internal use only.
> +* wdd_data: a pointer to watchdog core internal data.
>  * status: this field contains a number of status bits that give extra
>information about the status of the device (Like: is the watchdog timer
> -  running/active, is the nowayout bit set, is the device opened via
> -  the /dev/watchdog interface or not, ...).
> +  running/active, or is the nowayout bit set).
>  * deferred: entry in wtd_deferred_reg_list which is used to
>register early initialized watchdogs.
>  
> @@ -110,8 +106,8 @@ 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 *);
> - void (*unref)(struct watchdog_device *);
> + void (*ref)(struct watchdog_device *) __deprecated;
> + void (*unref)(struct watchdog_device *) __deprecated;
>   long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>  };
>  
> @@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
> lock the module when
>  the watchdog is active. (This to avoid a system crash when you unload the
>  module and /dev/watchdog is still open).
>  
> -If the watchdog_device struct is dynamically allocated, just locking the 
> module
> -is not enough and a driver also needs to define the ref and unref operations 
> to
> -ensure the structure holding the

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Tomas Winkler

On Mon, Dec 21, 2015 at 7:28 PM, Damien Riegel
 wrote:
>
> On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> > All variables required by the watchdog core to manage a watchdog are
> > currently stored in struct watchdog_device. The lifetime of those
> > variables is determined by the watchdog driver. However, the lifetime
> > of variables used by the watchdog core differs from the lifetime of
> > struct watchdog_device. To remedy this situation, watchdog drivers
> > can implement ref and unref callbacks, to be used by the watchdog
> > core to lock struct watchdog_device in memory.
> >
> > While this solves the immediate problem, it depends on watchdog drivers
> > to actually implement the ref/unref callbacks. This is error prone,
> > often not implemented in the first place, or not implemented correctly.
> >
> > To solve the problem without requiring driver support, split the variables
> > in struct watchdog_device into two data structures - one for variables
> > associated with the watchdog driver, one for variables associated with
> > the watchdog core. With this approach, the watchdog core can keep track
> > of its variable lifetime and no longer depends on ref/unref callbacks
> > in the driver. As a side effect, some of the variables originally in
> > struct watchdog_driver are now private to the watchdog core and no longer
> > visible in watchdog drivers.
> >
> > The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> > used and marked as deprecated.
>
> Two comments below. It's great to see that unbinding a driver no longer
> triggers a kernel panic.
>
> >
> > Signed-off-by: Guenter Roeck 
> > ---
> >  Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
> >  drivers/watchdog/watchdog_core.c   |   2 -
> >  drivers/watchdog/watchdog_core.h   |  23 ++
> >  drivers/watchdog/watchdog_dev.c| 377 
> > +
> >  include/linux/watchdog.h   |  21 +-
> >  5 files changed, 239 insertions(+), 229 deletions(-)
> >
> > diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
> > b/Documentation/watchdog/watchdog-kernel-api.txt
> > index 0a37da76acef..3db5092924e5 100644
> > --- a/Documentation/watchdog/watchdog-kernel-api.txt
> > +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> > @@ -44,7 +44,6 @@ The watchdog device structure looks like this:
> >
> >  struct watchdog_device {
> >   int id;
> > - struct cdev cdev;
> >   struct device *dev;
> >   struct device *parent;
> >   const struct watchdog_info *info;
> > @@ -56,7 +55,7 @@ struct watchdog_device {
> >   struct notifier_block reboot_nb;
> >   struct notifier_block restart_nb;
> >   void *driver_data;
> > - struct mutex lock;
> > + void *wdd_data;
> >   unsigned long status;
> >   struct list_head deferred;
> >  };
> > @@ -66,8 +65,6 @@ It contains following fields:
> >/dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
> >/dev/watchdog miscdev. The id is set automatically when calling
> >watchdog_register_device.
> > -* cdev: cdev for the dynamic /dev/watchdog device nodes. This
> > -  field is also populated by watchdog_register_device.
> >  * dev: device under the watchdog class (created by 
> > watchdog_register_device).
> >  * parent: set this to the parent device (or NULL) before calling
> >watchdog_register_device.
> > @@ -89,11 +86,10 @@ It contains following fields:
> >  * driver_data: a pointer to the drivers private data of a watchdog device.
> >This data should only be accessed via the watchdog_set_drvdata and
> >watchdog_get_drvdata routines.
> > -* lock: Mutex for WatchDog Timer Driver Core internal use only.
> > +* wdd_data: a pointer to watchdog core internal data.
> >  * status: this field contains a number of status bits that give extra
> >information about the status of the device (Like: is the watchdog timer
> > -  running/active, is the nowayout bit set, is the device opened via
> > -  the /dev/watchdog interface or not, ...).
> > +  running/active, or is the nowayout bit set).
> >  * deferred: entry in wtd_deferred_reg_list which is used to
> >register early initialized watchdogs.
> >
> > @@ -110,8 +106,8 @@ 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 *);
> > - void (*unref)(struct watchdog_device *);
> > + void (*ref)(struct watchdog_device *) __deprecated;
> > + void (*unref)(struct watchdog_device *) __deprecated;
> >   long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> >  };
> >
> > @@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
> > lock the module when
> >  the watchdog is active. (This to

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Guenter Roeck


On 12/21/2015 09:28 AM, Damien Riegel wrote:

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.


It should not have caused a panic to start with, but the ref/unref functions
for the most part were either not or wrongly implemented. Not really
surprising - it took me a while to understand the problem.

[ ... ]



  /*
+ * struct _watchdog_device - watchdog core internal data


Think it should be /**. Anyway, I find it confusing to have both
_watchdog_device and watchdog_device, but I can't think of a better
name right now.


I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
since it is only used there. No '**', though, because it is not a published
API, but just an internal data structure.

I also renamed the matching variable name to 'wd_data' (from '_wdd').



  static void watchdog_cdev_unregister(struct watchdog_device *wdd)
  {
-   mutex_lock(>lock);
-   set_bit(WDOG_UNREGISTERED, >status);
-   mutex_unlock(>lock);
+   struct _watchdog_device *_wdd = wdd->wdd_data;

-   cdev_del(>cdev);
+   cdev_del(&_wdd->cdev);
if (wdd->id == 0) {
misc_deregister(_miscdev);
-   old_wdd = NULL;
+   _old_wdd = NULL;
}
+
+   if (watchdog_active(wdd))
+   pr_crit("watchdog%d: watchdog still running!\n", wdd->id);


As it is now safe to unbind and rebind a driver, it means that a
watchdog driver probe function can now be called with a running
watchdog. Some drivers handle this situation, but I think that most of
them expect the watchdog to be off at this point.


No semantics change, though, and no change in behavior. Drivers _should_
handle that situation today. Sure, many don't, but that is a different issue.

I'll address handling an already-running watchdog by the watchdog core until
the character device is opened in a separate patch set, but we'll have to have
this series accepted before I re-introduce that. Even with that, it will still
be the driver's responsibility to detect and report that/if a watchdog is
already running.

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/

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Guenter Roeck


On 12/21/2015 03:36 PM, Tomas Winkler wrote:

On Mon, Dec 21, 2015 at 7:28 PM, Damien Riegel
 wrote:


On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.


Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.



Signed-off-by: Guenter Roeck 
---
  Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
  drivers/watchdog/watchdog_core.c   |   2 -
  drivers/watchdog/watchdog_core.h   |  23 ++
  drivers/watchdog/watchdog_dev.c| 377 +
  include/linux/watchdog.h   |  21 +-
  5 files changed, 239 insertions(+), 229 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
b/Documentation/watchdog/watchdog-kernel-api.txt
index 0a37da76acef..3db5092924e5 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -44,7 +44,6 @@ The watchdog device structure looks like this:

  struct watchdog_device {
   int id;
- struct cdev cdev;
   struct device *dev;
   struct device *parent;
   const struct watchdog_info *info;
@@ -56,7 +55,7 @@ struct watchdog_device {
   struct notifier_block reboot_nb;
   struct notifier_block restart_nb;
   void *driver_data;
- struct mutex lock;
+ void *wdd_data;
   unsigned long status;
   struct list_head deferred;
  };
@@ -66,8 +65,6 @@ It contains following fields:
/dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
/dev/watchdog miscdev. The id is set automatically when calling
watchdog_register_device.
-* cdev: cdev for the dynamic /dev/watchdog device nodes. This
-  field is also populated by watchdog_register_device.
  * dev: device under the watchdog class (created by watchdog_register_device).
  * parent: set this to the parent device (or NULL) before calling
watchdog_register_device.
@@ -89,11 +86,10 @@ It contains following fields:
  * driver_data: a pointer to the drivers private data of a watchdog device.
This data should only be accessed via the watchdog_set_drvdata and
watchdog_get_drvdata routines.
-* lock: Mutex for WatchDog Timer Driver Core internal use only.
+* wdd_data: a pointer to watchdog core internal data.
  * status: this field contains a number of status bits that give extra
information about the status of the device (Like: is the watchdog timer
-  running/active, is the nowayout bit set, is the device opened via
-  the /dev/watchdog interface or not, ...).
+  running/active, or is the nowayout bit set).
  * deferred: entry in wtd_deferred_reg_list which is used to
register early initialized watchdogs.

@@ -110,8 +106,8 @@ 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 *);
- void (*unref)(struct watchdog_device *);
+ void (*ref)(struct watchdog_device *) __deprecated;
+ void (*unref)(struct watchdog_device *) __deprecated;
   long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
  };

@@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
lock the module when
  the watchdog is active. (This to avoid a system crash when you unload the
  module and /dev/watchdog is still open).

-If the watchdog_device struct is dynamically allocated, just locking the module
-is not enough and a driver also needs to define the ref and unref operations to
-ensure the structure holding the

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-21 Thread Damien Riegel

On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> All variables required by the watchdog core to manage a watchdog are
> currently stored in struct watchdog_device. The lifetime of those
> variables is determined by the watchdog driver. However, the lifetime
> of variables used by the watchdog core differs from the lifetime of
> struct watchdog_device. To remedy this situation, watchdog drivers
> can implement ref and unref callbacks, to be used by the watchdog
> core to lock struct watchdog_device in memory.
> 
> While this solves the immediate problem, it depends on watchdog drivers
> to actually implement the ref/unref callbacks. This is error prone,
> often not implemented in the first place, or not implemented correctly.
> 
> To solve the problem without requiring driver support, split the variables
> in struct watchdog_device into two data structures - one for variables
> associated with the watchdog driver, one for variables associated with
> the watchdog core. With this approach, the watchdog core can keep track
> of its variable lifetime and no longer depends on ref/unref callbacks
> in the driver. As a side effect, some of the variables originally in
> struct watchdog_driver are now private to the watchdog core and no longer
> visible in watchdog drivers.
> 
> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> used and marked as deprecated.

Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.

> 
> Signed-off-by: Guenter Roeck 
> ---
>  Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
>  drivers/watchdog/watchdog_core.c   |   2 -
>  drivers/watchdog/watchdog_core.h   |  23 ++
>  drivers/watchdog/watchdog_dev.c| 377 
> +
>  include/linux/watchdog.h   |  21 +-
>  5 files changed, 239 insertions(+), 229 deletions(-)
> 
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
> b/Documentation/watchdog/watchdog-kernel-api.txt
> index 0a37da76acef..3db5092924e5 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -44,7 +44,6 @@ The watchdog device structure looks like this:
>  
>  struct watchdog_device {
>   int id;
> - struct cdev cdev;
>   struct device *dev;
>   struct device *parent;
>   const struct watchdog_info *info;
> @@ -56,7 +55,7 @@ struct watchdog_device {
>   struct notifier_block reboot_nb;
>   struct notifier_block restart_nb;
>   void *driver_data;
> - struct mutex lock;
> + void *wdd_data;
>   unsigned long status;
>   struct list_head deferred;
>  };
> @@ -66,8 +65,6 @@ It contains following fields:
>/dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
>/dev/watchdog miscdev. The id is set automatically when calling
>watchdog_register_device.
> -* cdev: cdev for the dynamic /dev/watchdog device nodes. This
> -  field is also populated by watchdog_register_device.
>  * dev: device under the watchdog class (created by watchdog_register_device).
>  * parent: set this to the parent device (or NULL) before calling
>watchdog_register_device.
> @@ -89,11 +86,10 @@ It contains following fields:
>  * driver_data: a pointer to the drivers private data of a watchdog device.
>This data should only be accessed via the watchdog_set_drvdata and
>watchdog_get_drvdata routines.
> -* lock: Mutex for WatchDog Timer Driver Core internal use only.
> +* wdd_data: a pointer to watchdog core internal data.
>  * status: this field contains a number of status bits that give extra
>information about the status of the device (Like: is the watchdog timer
> -  running/active, is the nowayout bit set, is the device opened via
> -  the /dev/watchdog interface or not, ...).
> +  running/active, or is the nowayout bit set).
>  * deferred: entry in wtd_deferred_reg_list which is used to
>register early initialized watchdogs.
>  
> @@ -110,8 +106,8 @@ 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 *);
> - void (*unref)(struct watchdog_device *);
> + void (*ref)(struct watchdog_device *) __deprecated;
> + void (*unref)(struct watchdog_device *) __deprecated;
>   long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>  };
>  
> @@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
> lock the module when
>  the watchdog is active. (This to avoid a system crash when you unload the
>  module and /dev/watchdog is still open).
>  
> -If the watchdog_device struct is dynamically allocated, just locking the 
> module
> -is not enough and a driver also needs to define the ref and unref operations 
> to
> -ensure the

[PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-20 Thread Guenter Roeck

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.

Signed-off-by: Guenter Roeck 
---
 Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
 drivers/watchdog/watchdog_core.c   |   2 -
 drivers/watchdog/watchdog_core.h   |  23 ++
 drivers/watchdog/watchdog_dev.c| 377 +
 include/linux/watchdog.h   |  21 +-
 5 files changed, 239 insertions(+), 229 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
b/Documentation/watchdog/watchdog-kernel-api.txt
index 0a37da76acef..3db5092924e5 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -44,7 +44,6 @@ The watchdog device structure looks like this:
 
 struct watchdog_device {
int id;
-   struct cdev cdev;
struct device *dev;
struct device *parent;
const struct watchdog_info *info;
@@ -56,7 +55,7 @@ struct watchdog_device {
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
void *driver_data;
-   struct mutex lock;
+   void *wdd_data;
unsigned long status;
struct list_head deferred;
 };
@@ -66,8 +65,6 @@ It contains following fields:
   /dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
   /dev/watchdog miscdev. The id is set automatically when calling
   watchdog_register_device.
-* cdev: cdev for the dynamic /dev/watchdog device nodes. This
-  field is also populated by watchdog_register_device.
 * dev: device under the watchdog class (created by watchdog_register_device).
 * parent: set this to the parent device (or NULL) before calling
   watchdog_register_device.
@@ -89,11 +86,10 @@ It contains following fields:
 * driver_data: a pointer to the drivers private data of a watchdog device.
   This data should only be accessed via the watchdog_set_drvdata and
   watchdog_get_drvdata routines.
-* lock: Mutex for WatchDog Timer Driver Core internal use only.
+* wdd_data: a pointer to watchdog core internal data.
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
-  running/active, is the nowayout bit set, is the device opened via
-  the /dev/watchdog interface or not, ...).
+  running/active, or is the nowayout bit set).
 * deferred: entry in wtd_deferred_reg_list which is used to
   register early initialized watchdogs.
 
@@ -110,8 +106,8 @@ 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 *);
-   void (*unref)(struct watchdog_device *);
+   void (*ref)(struct watchdog_device *) __deprecated;
+   void (*unref)(struct watchdog_device *) __deprecated;
long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
@@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
lock the module when
 the watchdog is active. (This to avoid a system crash when you unload the
 module and /dev/watchdog is still open).
 
-If the watchdog_device struct is dynamically allocated, just locking the module
-is not enough and a driver also needs to define the ref and unref operations to
-ensure the structure holding the watchdog_device does not go away.
-
-The simplest (and usually sufficient) implementation of this is to:
-1) Add a kref struct to the same structure which is holding the watchdog_device
-2) Define a release callback for the kref which frees the struct holding both
-3) Call kref_init on this kref *before* calling

[PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

2015-12-20 Thread Guenter Roeck

All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.

While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.

To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.

The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.

Signed-off-by: Guenter Roeck 
---
 Documentation/watchdog/watchdog-kernel-api.txt |  45 +--
 drivers/watchdog/watchdog_core.c   |   2 -
 drivers/watchdog/watchdog_core.h   |  23 ++
 drivers/watchdog/watchdog_dev.c| 377 +
 include/linux/watchdog.h   |  21 +-
 5 files changed, 239 insertions(+), 229 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt 
b/Documentation/watchdog/watchdog-kernel-api.txt
index 0a37da76acef..3db5092924e5 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -44,7 +44,6 @@ The watchdog device structure looks like this:
 
 struct watchdog_device {
int id;
-   struct cdev cdev;
struct device *dev;
struct device *parent;
const struct watchdog_info *info;
@@ -56,7 +55,7 @@ struct watchdog_device {
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
void *driver_data;
-   struct mutex lock;
+   void *wdd_data;
unsigned long status;
struct list_head deferred;
 };
@@ -66,8 +65,6 @@ It contains following fields:
   /dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
   /dev/watchdog miscdev. The id is set automatically when calling
   watchdog_register_device.
-* cdev: cdev for the dynamic /dev/watchdog device nodes. This
-  field is also populated by watchdog_register_device.
 * dev: device under the watchdog class (created by watchdog_register_device).
 * parent: set this to the parent device (or NULL) before calling
   watchdog_register_device.
@@ -89,11 +86,10 @@ It contains following fields:
 * driver_data: a pointer to the drivers private data of a watchdog device.
   This data should only be accessed via the watchdog_set_drvdata and
   watchdog_get_drvdata routines.
-* lock: Mutex for WatchDog Timer Driver Core internal use only.
+* wdd_data: a pointer to watchdog core internal data.
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
-  running/active, is the nowayout bit set, is the device opened via
-  the /dev/watchdog interface or not, ...).
+  running/active, or is the nowayout bit set).
 * deferred: entry in wtd_deferred_reg_list which is used to
   register early initialized watchdogs.
 
@@ -110,8 +106,8 @@ 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 *);
-   void (*unref)(struct watchdog_device *);
+   void (*ref)(struct watchdog_device *) __deprecated;
+   void (*unref)(struct watchdog_device *) __deprecated;
long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
@@ -120,20 +116,6 @@ driver's operations. This module owner will be used to 
lock the module when
 the watchdog is active. (This to avoid a system crash when you unload the
 module and /dev/watchdog is still open).
 
-If the watchdog_device struct is dynamically allocated, just locking the module
-is not enough and a driver also needs to define the ref and unref operations to
-ensure the structure holding the watchdog_device does not go away.
-
-The simplest (and usually sufficient) implementation of this is to:
-1) Add a kref struct to the same structure which is holding the watchdog_device
-2) Define a release callback for the kref which frees the struct holding both
-3) Call kref_init on this kref *before*

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

Re: [PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

[PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

[PATCH 2/5] watchdog: Separate and maintain variables based on variable lifetime

20 matches

Site Navigation

Mail list logo

Footer information