The watchdog character device s currently created in watchdog_dev.c,
and the watchdog device in watchdog_core.c. This results in
cross-dependencies, as the device creation needs to know the watchdog
character device number.
On top of that, the watchdog character device is created before the
watchdog device is created. This can result in race conditions if the
watchdog device node is accessed before the watchdog device has been
created.
To solve the problem, move watchdog device creation into watchdog_dev.c,
and create the watchdog device prior to creating its device node.
Also move device class creation into watchdog_dev.c, since this is now
the only place where the watchdog class is needed.
Inspired by an earlier patch set from Damien Riegel.
Cc: Damien Riegel <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
Hi Damien,
I think this approach would be a bit better. The watchdog device isn't
really used in the watchdog core code, so it is better created in
watchdog_dev.c. That also fits well with other pending changes, such as
sysfs attribute support, and my attempts to move the ref/unref functions
completely into the watchdog core. As a side effect, it also cleans up
the error path in __watchdog_register_device().
What do you think ?
The code has been compile tested only so far. I'll try to test it later
today, but I wanted to get it out for discussion.
Thanks,
Guenter
drivers/watchdog/watchdog_core.c | 37 ++----------------------
drivers/watchdog/watchdog_core.h | 2 +-
drivers/watchdog/watchdog_dev.c | 61 ++++++++++++++++++++++++++++++++--------
3 files changed, 54 insertions(+), 46 deletions(-)
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 873f13972cf4..089e930fce19 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -41,7 +41,6 @@
#include "watchdog_core.h" /* For watchdog_dev_register/... */
static DEFINE_IDA(watchdog_ida);
-static struct class *watchdog_class;
/*
* Deferred Registration infrastructure.
@@ -139,7 +138,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
static int __watchdog_register_device(struct watchdog_device *wdd)
{
- int ret, id = -1, devno;
+ int ret, id = -1;
if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
return -EINVAL;
@@ -192,16 +191,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
}
}
- devno = wdd->cdev.dev;
- wdd->dev = device_create(watchdog_class, wdd->parent, devno,
- NULL, "watchdog%d", wdd->id);
- if (IS_ERR(wdd->dev)) {
- watchdog_dev_unregister(wdd);
- ida_simple_remove(&watchdog_ida, id);
- ret = PTR_ERR(wdd->dev);
- return ret;
- }
-
return 0;
}
@@ -232,19 +221,8 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
static void __watchdog_unregister_device(struct watchdog_device *wdd)
{
- int ret;
- int devno;
-
- if (wdd == NULL)
- return;
-
- devno = wdd->cdev.dev;
- ret = watchdog_dev_unregister(wdd);
- if (ret)
- pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
- device_destroy(watchdog_class, devno);
+ watchdog_dev_unregister(wdd);
ida_simple_remove(&watchdog_ida, wdd->id);
- wdd->dev = NULL;
}
/**
@@ -287,17 +265,9 @@ static int __init watchdog_init(void)
{
int err;
- watchdog_class = class_create(THIS_MODULE, "watchdog");
- if (IS_ERR(watchdog_class)) {
- pr_err("couldn't create class\n");
- return PTR_ERR(watchdog_class);
- }
-
err = watchdog_dev_init();
- if (err < 0) {
- class_destroy(watchdog_class);
+ if (err < 0)
return err;
- }
watchdog_deferred_registration();
return 0;
@@ -306,7 +276,6 @@ static int __init watchdog_init(void)
static void __exit watchdog_exit(void)
{
watchdog_dev_exit();
- class_destroy(watchdog_class);
ida_destroy(&watchdog_ida);
}
diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
index 6c951418fca7..86ff962d1e15 100644
--- a/drivers/watchdog/watchdog_core.h
+++ b/drivers/watchdog/watchdog_core.h
@@ -32,6 +32,6 @@
* Functions/procedures to be called by the core
*/
extern int watchdog_dev_register(struct watchdog_device *);
-extern int watchdog_dev_unregister(struct watchdog_device *);
+extern void watchdog_dev_unregister(struct watchdog_device *);
extern int __init watchdog_dev_init(void);
extern void __exit watchdog_dev_exit(void);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 56a649e66eb2..07abe8c9e58c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -49,6 +49,9 @@ static dev_t watchdog_devt;
/* the watchdog device behind /dev/watchdog */
static struct watchdog_device *old_wdd;
+/* the watchdog device class */
+static struct class *watchdog_class;
+
/*
* watchdog_ping: ping the watchdog.
* @wdd: the watchdog device to ping
@@ -523,7 +526,7 @@ static struct miscdevice watchdog_miscdev = {
* thus we set it up like that.
*/
-int watchdog_dev_register(struct watchdog_device *wdd)
+int _watchdog_dev_register(struct watchdog_device *wdd)
{
int err, devno;
@@ -532,11 +535,12 @@ int watchdog_dev_register(struct watchdog_device *wdd)
watchdog_miscdev.parent = wdd->parent;
err = misc_register(&watchdog_miscdev);
if (err != 0) {
- pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
- wdd->info->identity, WATCHDOG_MINOR, err);
+ dev_err(wdd->dev,
+ "Cannot register miscdev on minor=%d (err=%d).\n",
+ WATCHDOG_MINOR, err);
if (err == -EBUSY)
- pr_err("%s: a legacy watchdog module is probably present.\n",
- wdd->info->identity);
+ dev_err(wdd->dev,
+ "A legacy watchdog module is probably present.\n");
old_wdd = NULL;
return err;
}
@@ -550,8 +554,8 @@ int watchdog_dev_register(struct watchdog_device *wdd)
/* Add the device */
err = cdev_add(&wdd->cdev, devno, 1);
if (err) {
- pr_err("watchdog%d unable to add device %d:%d\n",
- wdd->id, MAJOR(watchdog_devt), wdd->id);
+ dev_err(wdd->dev, "Unable to add device %d:%d\n",
+ MAJOR(watchdog_devt), wdd->id);
if (wdd->id == 0) {
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
@@ -567,7 +571,7 @@ int watchdog_dev_register(struct watchdog_device *wdd)
* Unregister the watchdog and if needed the legacy /dev/watchdog device.
*/
-int watchdog_dev_unregister(struct watchdog_device *wdd)
+void _watchdog_dev_unregister(struct watchdog_device *wdd)
{
mutex_lock(&wdd->lock);
set_bit(WDOG_UNREGISTERED, &wdd->status);
@@ -578,7 +582,31 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
- return 0;
+}
+
+int watchdog_dev_register(struct watchdog_device *wdd)
+{
+ dev_t devno;
+ int ret;
+
+ devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
+ wdd->dev = device_create(watchdog_class, wdd->parent, devno,
+ wdd, "watchdog%d", wdd->id);
+ if (IS_ERR(wdd->dev))
+ return PTR_ERR(wdd->dev);
+
+ ret = _watchdog_dev_register(wdd);
+ if (ret)
+ device_destroy(watchdog_class, devno);
+
+ return ret;
+}
+
+void watchdog_dev_unregister(struct watchdog_device *wdd)
+{
+ _watchdog_dev_unregister(wdd);
+ device_destroy(watchdog_class, wdd->dev->devt);
+ wdd->dev = NULL;
}
/*
@@ -589,9 +617,19 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
int __init watchdog_dev_init(void)
{
- int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
- if (err < 0)
+ int err;
+
+ watchdog_class = class_create(THIS_MODULE, "watchdog");
+ if (IS_ERR(watchdog_class)) {
+ pr_err("couldn't create watchdog class\n");
+ return PTR_ERR(watchdog_class);
+ }
+
+ err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
+ if (err < 0) {
pr_err("watchdog: unable to allocate char dev region\n");
+ class_destroy(watchdog_class);
+ }
return err;
}
@@ -604,4 +642,5 @@ int __init watchdog_dev_init(void)
void __exit watchdog_dev_exit(void)
{
unregister_chrdev_region(watchdog_devt, MAX_DOGS);
+ class_destroy(watchdog_class);
}
--
2.1.4
On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> The watchdog character device s currently created in watchdog_dev.c,
> and the watchdog device in watchdog_core.c. This results in
> cross-dependencies, as the device creation needs to know the watchdog
> character device number.
>
> On top of that, the watchdog character device is created before the
> watchdog device is created. This can result in race conditions if the
> watchdog device node is accessed before the watchdog device has been
> created.
>
> To solve the problem, move watchdog device creation into watchdog_dev.c,
> and create the watchdog device prior to creating its device node.
> Also move device class creation into watchdog_dev.c, since this is now
> the only place where the watchdog class is needed.
>
> Inspired by an earlier patch set from Damien Riegel.
>
> Cc: Damien Riegel <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Hi Damien,
>
> I think this approach would be a bit better. The watchdog device isn't
> really used in the watchdog core code, so it is better created in
> watchdog_dev.c. That also fits well with other pending changes, such as
> sysfs attribute support, and my attempts to move the ref/unref functions
> completely into the watchdog core. As a side effect, it also cleans up
> the error path in __watchdog_register_device().
>
> What do you think ?
Hi Guenter,
Like the idea, but I don't really get the separation. For instance, you
move watchdog_class in watchdog_dev.c but you keep watchdog_ida in
watchdog_core.c whereas it is only used for device creation/deletion.
Also a few minor comments below.
> The code has been compile tested only so far. I'll try to test it later
> today, but I wanted to get it out for discussion.
I'll give it a try but I have only one board to test it.
>
> Thanks,
> Guenter
>
> drivers/watchdog/watchdog_core.c | 37 ++----------------------
> drivers/watchdog/watchdog_core.h | 2 +-
> drivers/watchdog/watchdog_dev.c | 61 ++++++++++++++++++++++++++++++++--------
> 3 files changed, 54 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 873f13972cf4..089e930fce19 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -41,7 +41,6 @@
> #include "watchdog_core.h" /* For watchdog_dev_register/... */
>
> static DEFINE_IDA(watchdog_ida);
> -static struct class *watchdog_class;
>
> /*
> * Deferred Registration infrastructure.
> @@ -139,7 +138,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>
> static int __watchdog_register_device(struct watchdog_device *wdd)
> {
> - int ret, id = -1, devno;
> + int ret, id = -1;
>
> if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
> return -EINVAL;
> @@ -192,16 +191,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
> }
> }
>
> - devno = wdd->cdev.dev;
> - wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> - NULL, "watchdog%d", wdd->id);
> - if (IS_ERR(wdd->dev)) {
> - watchdog_dev_unregister(wdd);
> - ida_simple_remove(&watchdog_ida, id);
> - ret = PTR_ERR(wdd->dev);
> - return ret;
> - }
> -
> return 0;
> }
>
> @@ -232,19 +221,8 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
>
> static void __watchdog_unregister_device(struct watchdog_device *wdd)
> {
> - int ret;
> - int devno;
> -
> - if (wdd == NULL)
> - return;
> -
> - devno = wdd->cdev.dev;
> - ret = watchdog_dev_unregister(wdd);
> - if (ret)
> - pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
> - device_destroy(watchdog_class, devno);
> + watchdog_dev_unregister(wdd);
> ida_simple_remove(&watchdog_ida, wdd->id);
> - wdd->dev = NULL;
> }
>
> /**
> @@ -287,17 +265,9 @@ static int __init watchdog_init(void)
> {
> int err;
>
> - watchdog_class = class_create(THIS_MODULE, "watchdog");
> - if (IS_ERR(watchdog_class)) {
> - pr_err("couldn't create class\n");
> - return PTR_ERR(watchdog_class);
> - }
> -
> err = watchdog_dev_init();
> - if (err < 0) {
> - class_destroy(watchdog_class);
> + if (err < 0)
> return err;
> - }
>
> watchdog_deferred_registration();
> return 0;
> @@ -306,7 +276,6 @@ static int __init watchdog_init(void)
> static void __exit watchdog_exit(void)
> {
> watchdog_dev_exit();
> - class_destroy(watchdog_class);
> ida_destroy(&watchdog_ida);
> }
>
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 6c951418fca7..86ff962d1e15 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -32,6 +32,6 @@
> * Functions/procedures to be called by the core
> */
> extern int watchdog_dev_register(struct watchdog_device *);
> -extern int watchdog_dev_unregister(struct watchdog_device *);
> +extern void watchdog_dev_unregister(struct watchdog_device *);
> extern int __init watchdog_dev_init(void);
> extern void __exit watchdog_dev_exit(void);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 56a649e66eb2..07abe8c9e58c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -49,6 +49,9 @@ static dev_t watchdog_devt;
> /* the watchdog device behind /dev/watchdog */
> static struct watchdog_device *old_wdd;
>
> +/* the watchdog device class */
> +static struct class *watchdog_class;
> +
> /*
> * watchdog_ping: ping the watchdog.
> * @wdd: the watchdog device to ping
> @@ -523,7 +526,7 @@ static struct miscdevice watchdog_miscdev = {
> * thus we set it up like that.
> */
>
> -int watchdog_dev_register(struct watchdog_device *wdd)
> +int _watchdog_dev_register(struct watchdog_device *wdd)
Doc not updated and still refers to watchdog_dev_register.
> {
> int err, devno;
>
> @@ -532,11 +535,12 @@ int watchdog_dev_register(struct watchdog_device *wdd)
> watchdog_miscdev.parent = wdd->parent;
> err = misc_register(&watchdog_miscdev);
> if (err != 0) {
> - pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> - wdd->info->identity, WATCHDOG_MINOR, err);
> + dev_err(wdd->dev,
> + "Cannot register miscdev on minor=%d (err=%d).\n",
> + WATCHDOG_MINOR, err);
> if (err == -EBUSY)
> - pr_err("%s: a legacy watchdog module is probably present.\n",
> - wdd->info->identity);
> + dev_err(wdd->dev,
> + "A legacy watchdog module is probably present.\n");
> old_wdd = NULL;
> return err;
> }
> @@ -550,8 +554,8 @@ int watchdog_dev_register(struct watchdog_device *wdd)
> /* Add the device */
> err = cdev_add(&wdd->cdev, devno, 1);
> if (err) {
> - pr_err("watchdog%d unable to add device %d:%d\n",
> - wdd->id, MAJOR(watchdog_devt), wdd->id);
> + dev_err(wdd->dev, "Unable to add device %d:%d\n",
> + MAJOR(watchdog_devt), wdd->id);
> if (wdd->id == 0) {
> misc_deregister(&watchdog_miscdev);
> old_wdd = NULL;
> @@ -567,7 +571,7 @@ int watchdog_dev_register(struct watchdog_device *wdd)
> * Unregister the watchdog and if needed the legacy /dev/watchdog device.
> */
>
> -int watchdog_dev_unregister(struct watchdog_device *wdd)
> +void _watchdog_dev_unregister(struct watchdog_device *wdd)
Doc not updated.
> {
> mutex_lock(&wdd->lock);
> set_bit(WDOG_UNREGISTERED, &wdd->status);
> @@ -578,7 +582,31 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
> misc_deregister(&watchdog_miscdev);
> old_wdd = NULL;
> }
> - return 0;
> +}
> +
> +int watchdog_dev_register(struct watchdog_device *wdd)
> +{
> + dev_t devno;
> + int ret;
> +
> + devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
This is now done twice, here and in _watchdog_dev_register. Maybe
_watchdog_dev_register should be updated to read:
devno = wdd->dev.devt
The end result is the same but the link between the two devices is more
explicit (like it is now).
> + wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> + wdd, "watchdog%d", wdd->id);
> + if (IS_ERR(wdd->dev))
> + return PTR_ERR(wdd->dev);
> +
> + ret = _watchdog_dev_register(wdd);
> + if (ret)
> + device_destroy(watchdog_class, devno);
> +
> + return ret;
> +}
> +
> +void watchdog_dev_unregister(struct watchdog_device *wdd)
> +{
> + _watchdog_dev_unregister(wdd);
> + device_destroy(watchdog_class, wdd->dev->devt);
> + wdd->dev = NULL;
> }
>
> /*
> @@ -589,9 +617,19 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>
> int __init watchdog_dev_init(void)
> {
> - int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> - if (err < 0)
> + int err;
> +
> + watchdog_class = class_create(THIS_MODULE, "watchdog");
> + if (IS_ERR(watchdog_class)) {
> + pr_err("couldn't create watchdog class\n");
> + return PTR_ERR(watchdog_class);
> + }
> +
> + err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> + if (err < 0) {
> pr_err("watchdog: unable to allocate char dev region\n");
> + class_destroy(watchdog_class);
> + }
Newline here ? checkpatch does not complain and this file style is not
consistent on new line before return. Don't know which style is
preferred.
> return err;
> }
>
> @@ -604,4 +642,5 @@ int __init watchdog_dev_init(void)
> void __exit watchdog_dev_exit(void)
> {
> unregister_chrdev_region(watchdog_devt, MAX_DOGS);
> + class_destroy(watchdog_class);
> }
> --
> 2.1.4
>
Hi Damien,
On 12/07/2015 08:15 AM, Damien Riegel wrote:
> On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
>> The watchdog character device s currently created in watchdog_dev.c,
>> and the watchdog device in watchdog_core.c. This results in
>> cross-dependencies, as the device creation needs to know the watchdog
>> character device number.
>>
>> On top of that, the watchdog character device is created before the
>> watchdog device is created. This can result in race conditions if the
>> watchdog device node is accessed before the watchdog device has been
>> created.
>>
>> To solve the problem, move watchdog device creation into watchdog_dev.c,
>> and create the watchdog device prior to creating its device node.
>> Also move device class creation into watchdog_dev.c, since this is now
>> the only place where the watchdog class is needed.
>>
>> Inspired by an earlier patch set from Damien Riegel.
>>
>> Cc: Damien Riegel <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> Hi Damien,
>>
>> I think this approach would be a bit better. The watchdog device isn't
>> really used in the watchdog core code, so it is better created in
>> watchdog_dev.c. That also fits well with other pending changes, such as
>> sysfs attribute support, and my attempts to move the ref/unref functions
>> completely into the watchdog core. As a side effect, it also cleans up
>> the error path in __watchdog_register_device().
>>
>> What do you think ?
>
> Hi Guenter,
>
> Like the idea, but I don't really get the separation. For instance, you
> move watchdog_class in watchdog_dev.c but you keep watchdog_ida in
> watchdog_core.c whereas it is only used for device creation/deletion.
>
The class is watchdog driver internal, and it is device related, so I think
it made sense to move it to watchdog_dev.c. On top of that, it will be needed
there if/when we introduce sysfs attributes.
The watchdog id can be determined by obtaining an id using ida, or it can
be provided through the watchdog alias. The operation to get it is not
device related, and it is not straightforward to obtain it, so I thought
it makes sense to keep the code in watchdog_core.c.
Of course a lot of it is personal preference.
> Also a few minor comments below.
>
>> The code has been compile tested only so far. I'll try to test it later
>> today, but I wanted to get it out for discussion.
>
> I'll give it a try but I have only one board to test it.
>
I'll definitely test it myself as well.
>>
>> Thanks,
>> Guenter
>>
>> drivers/watchdog/watchdog_core.c | 37 ++----------------------
>> drivers/watchdog/watchdog_core.h | 2 +-
>> drivers/watchdog/watchdog_dev.c | 61 ++++++++++++++++++++++++++++++++--------
>> 3 files changed, 54 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
>> index 873f13972cf4..089e930fce19 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -41,7 +41,6 @@
>> #include "watchdog_core.h" /* For watchdog_dev_register/... */
>>
>> static DEFINE_IDA(watchdog_ida);
>> -static struct class *watchdog_class;
>>
>> /*
>> * Deferred Registration infrastructure.
>> @@ -139,7 +138,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>>
>> static int __watchdog_register_device(struct watchdog_device *wdd)
>> {
>> - int ret, id = -1, devno;
>> + int ret, id = -1;
>>
>> if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
>> return -EINVAL;
>> @@ -192,16 +191,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>> }
>> }
>>
>> - devno = wdd->cdev.dev;
>> - wdd->dev = device_create(watchdog_class, wdd->parent, devno,
>> - NULL, "watchdog%d", wdd->id);
>> - if (IS_ERR(wdd->dev)) {
>> - watchdog_dev_unregister(wdd);
>> - ida_simple_remove(&watchdog_ida, id);
>> - ret = PTR_ERR(wdd->dev);
>> - return ret;
>> - }
>> -
>> return 0;
>> }
>>
>> @@ -232,19 +221,8 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
>>
>> static void __watchdog_unregister_device(struct watchdog_device *wdd)
>> {
>> - int ret;
>> - int devno;
>> -
>> - if (wdd == NULL)
>> - return;
>> -
>> - devno = wdd->cdev.dev;
>> - ret = watchdog_dev_unregister(wdd);
>> - if (ret)
>> - pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
>> - device_destroy(watchdog_class, devno);
>> + watchdog_dev_unregister(wdd);
>> ida_simple_remove(&watchdog_ida, wdd->id);
>> - wdd->dev = NULL;
>> }
>>
>> /**
>> @@ -287,17 +265,9 @@ static int __init watchdog_init(void)
>> {
>> int err;
>>
>> - watchdog_class = class_create(THIS_MODULE, "watchdog");
>> - if (IS_ERR(watchdog_class)) {
>> - pr_err("couldn't create class\n");
>> - return PTR_ERR(watchdog_class);
>> - }
>> -
>> err = watchdog_dev_init();
>> - if (err < 0) {
>> - class_destroy(watchdog_class);
>> + if (err < 0)
>> return err;
>> - }
>>
>> watchdog_deferred_registration();
>> return 0;
>> @@ -306,7 +276,6 @@ static int __init watchdog_init(void)
>> static void __exit watchdog_exit(void)
>> {
>> watchdog_dev_exit();
>> - class_destroy(watchdog_class);
>> ida_destroy(&watchdog_ida);
>> }
>>
>> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
>> index 6c951418fca7..86ff962d1e15 100644
>> --- a/drivers/watchdog/watchdog_core.h
>> +++ b/drivers/watchdog/watchdog_core.h
>> @@ -32,6 +32,6 @@
>> * Functions/procedures to be called by the core
>> */
>> extern int watchdog_dev_register(struct watchdog_device *);
>> -extern int watchdog_dev_unregister(struct watchdog_device *);
>> +extern void watchdog_dev_unregister(struct watchdog_device *);
>> extern int __init watchdog_dev_init(void);
>> extern void __exit watchdog_dev_exit(void);
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 56a649e66eb2..07abe8c9e58c 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -49,6 +49,9 @@ static dev_t watchdog_devt;
>> /* the watchdog device behind /dev/watchdog */
>> static struct watchdog_device *old_wdd;
>>
>> +/* the watchdog device class */
>> +static struct class *watchdog_class;
>> +
>> /*
>> * watchdog_ping: ping the watchdog.
>> * @wdd: the watchdog device to ping
>> @@ -523,7 +526,7 @@ static struct miscdevice watchdog_miscdev = {
>> * thus we set it up like that.
>> */
>>
>> -int watchdog_dev_register(struct watchdog_device *wdd)
>> +int _watchdog_dev_register(struct watchdog_device *wdd)
>
> Doc not updated and still refers to watchdog_dev_register.
>
Yes, that is still the API function that is called from watchdog_register_device().
It should be static, though.
>> {
>> int err, devno;
>>
>> @@ -532,11 +535,12 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>> watchdog_miscdev.parent = wdd->parent;
>> err = misc_register(&watchdog_miscdev);
>> if (err != 0) {
>> - pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
>> - wdd->info->identity, WATCHDOG_MINOR, err);
>> + dev_err(wdd->dev,
>> + "Cannot register miscdev on minor=%d (err=%d).\n",
>> + WATCHDOG_MINOR, err);
>> if (err == -EBUSY)
>> - pr_err("%s: a legacy watchdog module is probably present.\n",
>> - wdd->info->identity);
>> + dev_err(wdd->dev,
>> + "A legacy watchdog module is probably present.\n");
>> old_wdd = NULL;
>> return err;
>> }
>> @@ -550,8 +554,8 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>> /* Add the device */
>> err = cdev_add(&wdd->cdev, devno, 1);
>> if (err) {
>> - pr_err("watchdog%d unable to add device %d:%d\n",
>> - wdd->id, MAJOR(watchdog_devt), wdd->id);
>> + dev_err(wdd->dev, "Unable to add device %d:%d\n",
>> + MAJOR(watchdog_devt), wdd->id);
>> if (wdd->id == 0) {
>> misc_deregister(&watchdog_miscdev);
>> old_wdd = NULL;
>> @@ -567,7 +571,7 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>> * Unregister the watchdog and if needed the legacy /dev/watchdog device.
>> */
>>
>> -int watchdog_dev_unregister(struct watchdog_device *wdd)
>> +void _watchdog_dev_unregister(struct watchdog_device *wdd)
>
> Doc not updated.
>
Same as above.
>> {
>> mutex_lock(&wdd->lock);
>> set_bit(WDOG_UNREGISTERED, &wdd->status);
>> @@ -578,7 +582,31 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>> misc_deregister(&watchdog_miscdev);
>> old_wdd = NULL;
>> }
>> - return 0;
>> +}
>> +
>> +int watchdog_dev_register(struct watchdog_device *wdd)
>> +{
>> + dev_t devno;
>> + int ret;
>> +
>> + devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
>
> This is now done twice, here and in _watchdog_dev_register. Maybe
> _watchdog_dev_register should be updated to read:
>
> devno = wdd->dev.devt
>
> The end result is the same but the link between the two devices is more
> explicit (like it is now).
>
You are right, that makes sense. I dropped the variable and now use
wdd->dev.devt directly.
>> + wdd->dev = device_create(watchdog_class, wdd->parent, devno,
>> + wdd, "watchdog%d", wdd->id);
>> + if (IS_ERR(wdd->dev))
>> + return PTR_ERR(wdd->dev);
>> +
>> + ret = _watchdog_dev_register(wdd);
>> + if (ret)
>> + device_destroy(watchdog_class, devno);
>> +
>> + return ret;
>> +}
>> +
>> +void watchdog_dev_unregister(struct watchdog_device *wdd)
>> +{
>> + _watchdog_dev_unregister(wdd);
>> + device_destroy(watchdog_class, wdd->dev->devt);
>> + wdd->dev = NULL;
>> }
>>
>> /*
>> @@ -589,9 +617,19 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>>
>> int __init watchdog_dev_init(void)
>> {
>> - int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
>> - if (err < 0)
>> + int err;
>> +
>> + watchdog_class = class_create(THIS_MODULE, "watchdog");
>> + if (IS_ERR(watchdog_class)) {
>> + pr_err("couldn't create watchdog class\n");
>> + return PTR_ERR(watchdog_class);
>> + }
>> +
>> + err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
>> + if (err < 0) {
>> pr_err("watchdog: unable to allocate char dev region\n");
>> + class_destroy(watchdog_class);
>> + }
>
> Newline here ? checkpatch does not complain and this file style is not
> consistent on new line before return. Don't know which style is
> preferred.
>
The file actually does use a specific style, though it is maybe not easy to see.
func();
return err;
or
if (expr) {
func();
}
return err;
or
if (expr)
func();
return err;
No idea if that is on purpose, but it does make some sense to me for readability.
Thanks,
Guenter
Hi All,
> On 12/07/2015 08:15 AM, Damien Riegel wrote:
> >On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> >>The watchdog character device s currently created in watchdog_dev.c,
> >>and the watchdog device in watchdog_core.c. This results in
> >>cross-dependencies, as the device creation needs to know the watchdog
> >>character device number.
> >>
> >>On top of that, the watchdog character device is created before the
> >>watchdog device is created. This can result in race conditions if the
> >>watchdog device node is accessed before the watchdog device has been
> >>created.
> >>
> >>To solve the problem, move watchdog device creation into watchdog_dev.c,
> >>and create the watchdog device prior to creating its device node.
> >>Also move device class creation into watchdog_dev.c, since this is now
> >>the only place where the watchdog class is needed.
> >>
> >>Inspired by an earlier patch set from Damien Riegel.
> >>
> >>Cc: Damien Riegel <[email protected]>
> >>Signed-off-by: Guenter Roeck <[email protected]>
> >>---
> >>Hi Damien,
> >>
> >>I think this approach would be a bit better. The watchdog device isn't
> >>really used in the watchdog core code, so it is better created in
> >>watchdog_dev.c. That also fits well with other pending changes, such as
> >>sysfs attribute support, and my attempts to move the ref/unref functions
> >>completely into the watchdog core. As a side effect, it also cleans up
> >>the error path in __watchdog_register_device().
> >>
> >>What do you think ?
> >
> >Hi Guenter,
> >
> >Like the idea, but I don't really get the separation. For instance, you
> >move watchdog_class in watchdog_dev.c but you keep watchdog_ida in
> >watchdog_core.c whereas it is only used for device creation/deletion.
> >
> The class is watchdog driver internal, and it is device related, so I think
> it made sense to move it to watchdog_dev.c. On top of that, it will be
> needed
> there if/when we introduce sysfs attributes.
>
> The watchdog id can be determined by obtaining an id using ida, or it can
> be provided through the watchdog alias. The operation to get it is not
> device related, and it is not straightforward to obtain it, so I thought
> it makes sense to keep the code in watchdog_core.c.
>
> Of course a lot of it is personal preference.
>
Let me go back to how I saw the design when I created the generic watchdog framework:
When using watchdog device drivers we need to be able to support the /dev/watchdog system.
I also foresaw that we should have a sysfs interface and I saw the future for watchdog devices that you should be able to choose between the 2 different systems. You should be able to use only the /dev/watchdog interfacing, but you should also be able to use both a sysfs interface and a /dev/watchdog interface and it should even be possible to have only a sysfs interface in certain embedded devices. So that's why I split the watchdog framework over 3 files: core code, the /dev/watchdog interfacing and the sysfs code. Since I want to have compiled code small enough when choosing either /Dev/watchdog or sysfs or both this sounded the most logical thing to do (Unless you have a single file full of #ifdef-ery that becomes unreadable).
So I do not agree to have sysfs code in watchdog_dev.c . It belongs in watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to listen to it and see what the benefits are. But I want a clean system for excluding both /dev/ (current watchdog_dev.c) and/or sysfs (watchdog_sysfs.c) in the future. Off-course the current behaviour is to have the /dev/ interface and have the option to add sysfs attributes.
Kind regards,
Wim.
On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
> Hi All,
>
> > On 12/07/2015 08:15 AM, Damien Riegel wrote:
> > >On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> > >>The watchdog character device s currently created in
> > >>watchdog_dev.c, and the watchdog device in watchdog_core.c. This
> > >>results in cross-dependencies, as the device creation needs to
> > >>know the watchdog character device number.
> > >>
> > >>On top of that, the watchdog character device is created before
> > >>the watchdog device is created. This can result in race conditions
> > >>if the watchdog device node is accessed before the watchdog device
> > >>has been created.
> > >>
> > >>To solve the problem, move watchdog device creation into
> > >>watchdog_dev.c, and create the watchdog device prior to creating
> > >>its device node. Also move device class creation into
> > >>watchdog_dev.c, since this is now the only place where the
> > >>watchdog class is needed.
> > >>
> > >>Inspired by an earlier patch set from Damien Riegel.
> > >>
> > >>Cc: Damien Riegel <[email protected]>
> > >>Signed-off-by: Guenter Roeck <[email protected]> --- Hi Damien,
> > >>
> > >>I think this approach would be a bit better. The watchdog device
> > >>isn't really used in the watchdog core code, so it is better
> > >>created in watchdog_dev.c. That also fits well with other pending
> > >>changes, such as sysfs attribute support, and my attempts to move
> > >>the ref/unref functions completely into the watchdog core. As a
> > >>side effect, it also cleans up the error path in
> > >>__watchdog_register_device().
> > >>
> > >>What do you think ?
> > >
> > >Hi Guenter,
> > >
> > >Like the idea, but I don't really get the separation. For instance,
> > >you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
> > >in watchdog_core.c whereas it is only used for device
> > >creation/deletion.
> > >
> > The class is watchdog driver internal, and it is device related, so
> > I think it made sense to move it to watchdog_dev.c. On top of that,
> > it will be needed there if/when we introduce sysfs attributes.
> >
> > The watchdog id can be determined by obtaining an id using ida, or
> > it can be provided through the watchdog alias. The operation to get
> > it is not device related, and it is not straightforward to obtain
> > it, so I thought it makes sense to keep the code in watchdog_core.c.
> >
> > Of course a lot of it is personal preference.
> >
>
> Let me go back to how I saw the design when I created the generic
> watchdog framework: When using watchdog device drivers we need to be
> able to support the /dev/watchdog system. I also foresaw that we
> should have a sysfs interface and I saw the future for watchdog
> devices that you should be able to choose between the 2 different
> systems. You should be able to use only the /dev/watchdog interfacing,
> but you should also be able to use both a sysfs interface and a
> /dev/watchdog interface and it should even be possible to have only a
> sysfs interface in certain embedded devices. So that's why I split the
> watchdog framework over 3 files: core code, the /dev/watchdog
> interfacing and the sysfs code. Since I want to have compiled code
> small enough when choosing either /Dev/watchdog or sysfs or both this
> sounded the most logical thing to do (Unless you have a single file
> full of #ifdef-ery that becomes unreadable).
>
> So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
> watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
> listen to it and see what the benefits are. But I want a clean system
> for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
> (watchdog_sysfs.c) in the future. Off-course the current behaviour is
> to have the /dev/ interface and have the option to add sysfs
> attributes.
I agree that keeping sysfs code separate makes sense, as someone might
want to not use it.
The question is: can we make the /dev/watchdog entries optional ? That
would break the compatibility, right? Imho, it would be saner to keep
only one way to interact with watchdogs (ie. keep /dev/watchdog as is
and don't make it optional, and sysfs read-only and eventually
optional). I think that question should be answered before we can decide
how we want to split the code between watchdog_dev.c and watchdog_core.c
Thanks,
Damien
>
> Kind regards, Wim.
>
On 12/13/2015 02:02 PM, Damien Riegel wrote:
> On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
>> Hi All,
>>
>>> On 12/07/2015 08:15 AM, Damien Riegel wrote:
>>>> On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
>>>>> The watchdog character device s currently created in
>>>>> watchdog_dev.c, and the watchdog device in watchdog_core.c. This
>>>>> results in cross-dependencies, as the device creation needs to
>>>>> know the watchdog character device number.
>>>>>
>>>>> On top of that, the watchdog character device is created before
>>>>> the watchdog device is created. This can result in race conditions
>>>>> if the watchdog device node is accessed before the watchdog device
>>>>> has been created.
>>>>>
>>>>> To solve the problem, move watchdog device creation into
>>>>> watchdog_dev.c, and create the watchdog device prior to creating
>>>>> its device node. Also move device class creation into
>>>>> watchdog_dev.c, since this is now the only place where the
>>>>> watchdog class is needed.
>>>>>
>>>>> Inspired by an earlier patch set from Damien Riegel.
>>>>>
>>>>> Cc: Damien Riegel <[email protected]>
>>>>> Signed-off-by: Guenter Roeck <[email protected]> --- Hi Damien,
>>>>>
>>>>> I think this approach would be a bit better. The watchdog device
>>>>> isn't really used in the watchdog core code, so it is better
>>>>> created in watchdog_dev.c. That also fits well with other pending
>>>>> changes, such as sysfs attribute support, and my attempts to move
>>>>> the ref/unref functions completely into the watchdog core. As a
>>>>> side effect, it also cleans up the error path in
>>>>> __watchdog_register_device().
>>>>>
>>>>> What do you think ?
>>>>
>>>> Hi Guenter,
>>>>
>>>> Like the idea, but I don't really get the separation. For instance,
>>>> you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
>>>> in watchdog_core.c whereas it is only used for device
>>>> creation/deletion.
>>>>
>>> The class is watchdog driver internal, and it is device related, so
>>> I think it made sense to move it to watchdog_dev.c. On top of that,
>>> it will be needed there if/when we introduce sysfs attributes.
>>>
>>> The watchdog id can be determined by obtaining an id using ida, or
>>> it can be provided through the watchdog alias. The operation to get
>>> it is not device related, and it is not straightforward to obtain
>>> it, so I thought it makes sense to keep the code in watchdog_core.c.
>>>
>>> Of course a lot of it is personal preference.
>>>
>>
>> Let me go back to how I saw the design when I created the generic
>> watchdog framework: When using watchdog device drivers we need to be
>> able to support the /dev/watchdog system. I also foresaw that we
>> should have a sysfs interface and I saw the future for watchdog
>> devices that you should be able to choose between the 2 different
>> systems. You should be able to use only the /dev/watchdog interfacing,
>> but you should also be able to use both a sysfs interface and a
>> /dev/watchdog interface and it should even be possible to have only a
>> sysfs interface in certain embedded devices. So that's why I split the
>> watchdog framework over 3 files: core code, the /dev/watchdog
>> interfacing and the sysfs code. Since I want to have compiled code
>> small enough when choosing either /Dev/watchdog or sysfs or both this
>> sounded the most logical thing to do (Unless you have a single file
>> full of #ifdef-ery that becomes unreadable).
>>
>> So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
>> watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
>> listen to it and see what the benefits are. But I want a clean system
>> for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
>> (watchdog_sysfs.c) in the future. Off-course the current behaviour is
>> to have the /dev/ interface and have the option to add sysfs
>> attributes.
>
> I agree that keeping sysfs code separate makes sense, as someone might
> want to not use it.
>
I am not really sure about that. I don't recall a similar concern with
any other subsystem.
Anyway, sure, we can move the code to another file. Sure, we can add a
configuration option. That means we'll also need to make several functions
non-static, and possibly move some functions out of watchdog_dev.c
into yet another file. But we'll need some guidance for that and an idea
what is going to be acceptable.
> The question is: can we make the /dev/watchdog entries optional ? That
> would break the compatibility, right? Imho, it would be saner to keep
> only one way to interact with watchdogs (ie. keep /dev/watchdog as is
> and don't make it optional, and sysfs read-only and eventually
> optional). I think that question should be answered before we can decide
> how we want to split the code between watchdog_dev.c and watchdog_core.c
>
All I know for my part is that all pending structural enhancements in one
way or another depend on the assumption that the character device and the
sysfs (kernel) device both exist. I have not been able to find any code
in the kernel where cdev_add is not associated with device_create, so I
don't even know if cdev_add without device_create is even possible.
The current code in watchdog_dev.c definitely depends on watchdog->dev,
though not deeply (it uses it for some dev_ messages). If that dependency
isn't supposed to exist, we'll need to do something about it now.
We now have the sysfs changes, the changes to move "soft" timeout handling
into the kernel, multiple approaches to add pretimeout into the watchdog core,
and my attempt to solve the ref/unref mess. All of those won't be able to
proceed without knowing how to handle the cdev vs. dev problem.
Wim, we really need some guidance from you.
Thanks,
Guenter
On Sun, Dec 13, 2015 at 10:24:35PM -0800, Guenter Roeck wrote:
> On 12/13/2015 02:02 PM, Damien Riegel wrote:
> >On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
> >>Hi All,
> >>
> >>>On 12/07/2015 08:15 AM, Damien Riegel wrote:
> >>>>On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> >>>>>The watchdog character device s currently created in
> >>>>>watchdog_dev.c, and the watchdog device in watchdog_core.c. This
> >>>>>results in cross-dependencies, as the device creation needs to
> >>>>>know the watchdog character device number.
> >>>>>
> >>>>>On top of that, the watchdog character device is created before
> >>>>>the watchdog device is created. This can result in race conditions
> >>>>>if the watchdog device node is accessed before the watchdog device
> >>>>>has been created.
> >>>>>
> >>>>>To solve the problem, move watchdog device creation into
> >>>>>watchdog_dev.c, and create the watchdog device prior to creating
> >>>>>its device node. Also move device class creation into
> >>>>>watchdog_dev.c, since this is now the only place where the
> >>>>>watchdog class is needed.
> >>>>>
> >>>>>Inspired by an earlier patch set from Damien Riegel.
> >>>>>
> >>>>>Cc: Damien Riegel <[email protected]>
> >>>>>Signed-off-by: Guenter Roeck <[email protected]> --- Hi Damien,
> >>>>>
> >>>>>I think this approach would be a bit better. The watchdog device
> >>>>>isn't really used in the watchdog core code, so it is better
> >>>>>created in watchdog_dev.c. That also fits well with other pending
> >>>>>changes, such as sysfs attribute support, and my attempts to move
> >>>>>the ref/unref functions completely into the watchdog core. As a
> >>>>>side effect, it also cleans up the error path in
> >>>>>__watchdog_register_device().
> >>>>>
> >>>>>What do you think ?
> >>>>
> >>>>Hi Guenter,
> >>>>
> >>>>Like the idea, but I don't really get the separation. For instance,
> >>>>you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
> >>>>in watchdog_core.c whereas it is only used for device
> >>>>creation/deletion.
> >>>>
> >>>The class is watchdog driver internal, and it is device related, so
> >>>I think it made sense to move it to watchdog_dev.c. On top of that,
> >>>it will be needed there if/when we introduce sysfs attributes.
> >>>
> >>>The watchdog id can be determined by obtaining an id using ida, or
> >>>it can be provided through the watchdog alias. The operation to get
> >>>it is not device related, and it is not straightforward to obtain
> >>>it, so I thought it makes sense to keep the code in watchdog_core.c.
> >>>
> >>>Of course a lot of it is personal preference.
> >>>
> >>
> >>Let me go back to how I saw the design when I created the generic
> >>watchdog framework: When using watchdog device drivers we need to be
> >>able to support the /dev/watchdog system. I also foresaw that we
> >>should have a sysfs interface and I saw the future for watchdog
> >>devices that you should be able to choose between the 2 different
> >>systems. You should be able to use only the /dev/watchdog interfacing,
> >>but you should also be able to use both a sysfs interface and a
> >>/dev/watchdog interface and it should even be possible to have only a
> >>sysfs interface in certain embedded devices. So that's why I split the
> >>watchdog framework over 3 files: core code, the /dev/watchdog
> >>interfacing and the sysfs code. Since I want to have compiled code
> >>small enough when choosing either /Dev/watchdog or sysfs or both this
> >>sounded the most logical thing to do (Unless you have a single file
> >>full of #ifdef-ery that becomes unreadable).
> >>
> >>So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
> >>watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
> >>listen to it and see what the benefits are. But I want a clean system
> >>for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
> >>(watchdog_sysfs.c) in the future. Off-course the current behaviour is
> >>to have the /dev/ interface and have the option to add sysfs
> >>attributes.
> >
> >I agree that keeping sysfs code separate makes sense, as someone might
> >want to not use it.
> >
> I am not really sure about that. I don't recall a similar concern with
> any other subsystem.
>
> Anyway, sure, we can move the code to another file. Sure, we can add a
> configuration option. That means we'll also need to make several functions
> non-static, and possibly move some functions out of watchdog_dev.c
> into yet another file. But we'll need some guidance for that and an idea
> what is going to be acceptable.
>
> >The question is: can we make the /dev/watchdog entries optional ? That
> >would break the compatibility, right? Imho, it would be saner to keep
> >only one way to interact with watchdogs (ie. keep /dev/watchdog as is
> >and don't make it optional, and sysfs read-only and eventually
> >optional). I think that question should be answered before we can decide
> >how we want to split the code between watchdog_dev.c and watchdog_core.c
> >
>
> All I know for my part is that all pending structural enhancements in one
> way or another depend on the assumption that the character device and the
> sysfs (kernel) device both exist. I have not been able to find any code
> in the kernel where cdev_add is not associated with device_create, so I
> don't even know if cdev_add without device_create is even possible.
> The current code in watchdog_dev.c definitely depends on watchdog->dev,
> though not deeply (it uses it for some dev_ messages). If that dependency
> isn't supposed to exist, we'll need to do something about it now.
>
> We now have the sysfs changes, the changes to move "soft" timeout handling
> into the kernel, multiple approaches to add pretimeout into the watchdog
> core,
> and my attempt to solve the ref/unref mess. All of those won't be able to
> proceed without knowing how to handle the cdev vs. dev problem.
>
> Wim, we really need some guidance from you.
Basically the options we have are:
1) go with the original design. The problem here (as Guenter pointed it out
correctly) is that sysfs went from optional to integrated with the character
devices.
2) change the origial design so that the watchdog_sysfs part is still optional
(meaning the additional read-only sysfs attributes) but is called from within
watchdog_dev.c instead of from watchdog_core.c . This would make it possible
to avoid race conditions. This would also mean that it is harder for an
embedded developer to get rid of the /dev/watchdog interface.
3) put the sysfs attributes together with the /dev/watchdog code in
watchdog_dev.c . The sysfs attributes can be optional with a minimum of
#ifdef's if needed.
4) consolidate everything in only watchdog_core.c . (if we put the sysfs and
the /dev/watchdog* code in 1 file, then why not put everything in one file).
For the linux-kernel leaving the dev/watchdog* interface and only use the sysfs
interface (but then read-write so that you can use the interface also for
starting and stopping a watchdog) would indeed mean a compatibility break.
But as an embedded developer it would not be a necessity and thus not a
compatibility issue.
Since /dev/watchdog was always tied with the misc devices, it makes sence
to follow the misc route and thus the character device route. This indeed
means that we always have the watchdog class (but also linkage to parent
devices via misc.parent).
So I think we should stick to the misc device approach (since this is how
it has always been). Which means that we also follow the character device
approach. So to me we should go for option 3 or 4.
Kind regards,
Wim.
On 12/14/2015 12:44 PM, Wim Van Sebroeck wrote:
> On Sun, Dec 13, 2015 at 10:24:35PM -0800, Guenter Roeck wrote:
>
>> On 12/13/2015 02:02 PM, Damien Riegel wrote:
>>> On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
>>>> Hi All,
>>>>
>>>>> On 12/07/2015 08:15 AM, Damien Riegel wrote:
>>>>>> On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
>>>>>>> The watchdog character device s currently created in
>>>>>>> watchdog_dev.c, and the watchdog device in watchdog_core.c. This
>>>>>>> results in cross-dependencies, as the device creation needs to
>>>>>>> know the watchdog character device number.
>>>>>>>
>>>>>>> On top of that, the watchdog character device is created before
>>>>>>> the watchdog device is created. This can result in race conditions
>>>>>>> if the watchdog device node is accessed before the watchdog device
>>>>>>> has been created.
>>>>>>>
>>>>>>> To solve the problem, move watchdog device creation into
>>>>>>> watchdog_dev.c, and create the watchdog device prior to creating
>>>>>>> its device node. Also move device class creation into
>>>>>>> watchdog_dev.c, since this is now the only place where the
>>>>>>> watchdog class is needed.
>>>>>>>
>>>>>>> Inspired by an earlier patch set from Damien Riegel.
>>>>>>>
>>>>>>> Cc: Damien Riegel <[email protected]>
>>>>>>> Signed-off-by: Guenter Roeck <[email protected]> --- Hi Damien,
>>>>>>>
>>>>>>> I think this approach would be a bit better. The watchdog device
>>>>>>> isn't really used in the watchdog core code, so it is better
>>>>>>> created in watchdog_dev.c. That also fits well with other pending
>>>>>>> changes, such as sysfs attribute support, and my attempts to move
>>>>>>> the ref/unref functions completely into the watchdog core. As a
>>>>>>> side effect, it also cleans up the error path in
>>>>>>> __watchdog_register_device().
>>>>>>>
>>>>>>> What do you think ?
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>> Like the idea, but I don't really get the separation. For instance,
>>>>>> you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
>>>>>> in watchdog_core.c whereas it is only used for device
>>>>>> creation/deletion.
>>>>>>
>>>>> The class is watchdog driver internal, and it is device related, so
>>>>> I think it made sense to move it to watchdog_dev.c. On top of that,
>>>>> it will be needed there if/when we introduce sysfs attributes.
>>>>>
>>>>> The watchdog id can be determined by obtaining an id using ida, or
>>>>> it can be provided through the watchdog alias. The operation to get
>>>>> it is not device related, and it is not straightforward to obtain
>>>>> it, so I thought it makes sense to keep the code in watchdog_core.c.
>>>>>
>>>>> Of course a lot of it is personal preference.
>>>>>
>>>>
>>>> Let me go back to how I saw the design when I created the generic
>>>> watchdog framework: When using watchdog device drivers we need to be
>>>> able to support the /dev/watchdog system. I also foresaw that we
>>>> should have a sysfs interface and I saw the future for watchdog
>>>> devices that you should be able to choose between the 2 different
>>>> systems. You should be able to use only the /dev/watchdog interfacing,
>>>> but you should also be able to use both a sysfs interface and a
>>>> /dev/watchdog interface and it should even be possible to have only a
>>>> sysfs interface in certain embedded devices. So that's why I split the
>>>> watchdog framework over 3 files: core code, the /dev/watchdog
>>>> interfacing and the sysfs code. Since I want to have compiled code
>>>> small enough when choosing either /Dev/watchdog or sysfs or both this
>>>> sounded the most logical thing to do (Unless you have a single file
>>>> full of #ifdef-ery that becomes unreadable).
>>>>
>>>> So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
>>>> watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
>>>> listen to it and see what the benefits are. But I want a clean system
>>>> for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
>>>> (watchdog_sysfs.c) in the future. Off-course the current behaviour is
>>>> to have the /dev/ interface and have the option to add sysfs
>>>> attributes.
>>>
>>> I agree that keeping sysfs code separate makes sense, as someone might
>>> want to not use it.
>>>
>> I am not really sure about that. I don't recall a similar concern with
>> any other subsystem.
>>
>> Anyway, sure, we can move the code to another file. Sure, we can add a
>> configuration option. That means we'll also need to make several functions
>> non-static, and possibly move some functions out of watchdog_dev.c
>> into yet another file. But we'll need some guidance for that and an idea
>> what is going to be acceptable.
>>
>>> The question is: can we make the /dev/watchdog entries optional ? That
>>> would break the compatibility, right? Imho, it would be saner to keep
>>> only one way to interact with watchdogs (ie. keep /dev/watchdog as is
>>> and don't make it optional, and sysfs read-only and eventually
>>> optional). I think that question should be answered before we can decide
>>> how we want to split the code between watchdog_dev.c and watchdog_core.c
>>>
>>
>> All I know for my part is that all pending structural enhancements in one
>> way or another depend on the assumption that the character device and the
>> sysfs (kernel) device both exist. I have not been able to find any code
>> in the kernel where cdev_add is not associated with device_create, so I
>> don't even know if cdev_add without device_create is even possible.
>> The current code in watchdog_dev.c definitely depends on watchdog->dev,
>> though not deeply (it uses it for some dev_ messages). If that dependency
>> isn't supposed to exist, we'll need to do something about it now.
>>
>> We now have the sysfs changes, the changes to move "soft" timeout handling
>> into the kernel, multiple approaches to add pretimeout into the watchdog
>> core,
>> and my attempt to solve the ref/unref mess. All of those won't be able to
>> proceed without knowing how to handle the cdev vs. dev problem.
>>
>> Wim, we really need some guidance from you.
>
> Basically the options we have are:
> 1) go with the original design. The problem here (as Guenter pointed it out
> correctly) is that sysfs went from optional to integrated with the character
> devices.
> 2) change the origial design so that the watchdog_sysfs part is still optional
> (meaning the additional read-only sysfs attributes) but is called from within
> watchdog_dev.c instead of from watchdog_core.c . This would make it possible
> to avoid race conditions. This would also mean that it is harder for an
> embedded developer to get rid of the /dev/watchdog interface.
> 3) put the sysfs attributes together with the /dev/watchdog code in
> watchdog_dev.c . The sysfs attributes can be optional with a minimum of
> #ifdef's if needed.
> 4) consolidate everything in only watchdog_core.c . (if we put the sysfs and
> the /dev/watchdog* code in 1 file, then why not put everything in one file).
>
> For the linux-kernel leaving the dev/watchdog* interface and only use the sysfs
> interface (but then read-write so that you can use the interface also for
> starting and stopping a watchdog) would indeed mean a compatibility break.
> But as an embedded developer it would not be a necessity and thus not a
> compatibility issue.
>
> Since /dev/watchdog was always tied with the misc devices, it makes sence
> to follow the misc route and thus the character device route. This indeed
> means that we always have the watchdog class (but also linkage to parent
> devices via misc.parent).
>
> So I think we should stick to the misc device approach (since this is how
> it has always been). Which means that we also follow the character device
> approach. So to me we should go for option 3 or 4.
>
I would suggest to go with 3), and also move the watchdog class to watchdog_dev.c.
I don't see a real reason to move everything into a single file - it would just
make code maintenance more difficult, and the current separation seems to work
quite well as far as I can see.
Thanks,
Guenter
On 15/12/2015:06:56:50 PM, Guenter Roeck wrote:
> On 12/14/2015 12:44 PM, Wim Van Sebroeck wrote:
> >On Sun, Dec 13, 2015 at 10:24:35PM -0800, Guenter Roeck wrote:
> >
> >>On 12/13/2015 02:02 PM, Damien Riegel wrote:
> >>>On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
> >>>>Hi All,
> >>>>
> >>>>>On 12/07/2015 08:15 AM, Damien Riegel wrote:
> >>>>>>On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> >>>>>>>The watchdog character device s currently created in
> >>>>>>>watchdog_dev.c, and the watchdog device in watchdog_core.c. This
> >>>>>>>results in cross-dependencies, as the device creation needs to
> >>>>>>>know the watchdog character device number.
> >>>>>>>
> >>>>>>>On top of that, the watchdog character device is created before
> >>>>>>>the watchdog device is created. This can result in race conditions
> >>>>>>>if the watchdog device node is accessed before the watchdog device
> >>>>>>>has been created.
> >>>>>>>
> >>>>>>>To solve the problem, move watchdog device creation into
> >>>>>>>watchdog_dev.c, and create the watchdog device prior to creating
> >>>>>>>its device node. Also move device class creation into
> >>>>>>>watchdog_dev.c, since this is now the only place where the
> >>>>>>>watchdog class is needed.
> >>>>>>>
> >>>>>>>Inspired by an earlier patch set from Damien Riegel.
> >>>>>>>
> >>>>>>>Cc: Damien Riegel <[email protected]>
> >>>>>>>Signed-off-by: Guenter Roeck <[email protected]> --- Hi Damien,
> >>>>>>>
> >>>>>>>I think this approach would be a bit better. The watchdog device
> >>>>>>>isn't really used in the watchdog core code, so it is better
> >>>>>>>created in watchdog_dev.c. That also fits well with other pending
> >>>>>>>changes, such as sysfs attribute support, and my attempts to move
> >>>>>>>the ref/unref functions completely into the watchdog core. As a
> >>>>>>>side effect, it also cleans up the error path in
> >>>>>>>__watchdog_register_device().
> >>>>>>>
> >>>>>>>What do you think ?
> >>>>>>
> >>>>>>Hi Guenter,
> >>>>>>
> >>>>>>Like the idea, but I don't really get the separation. For instance,
> >>>>>>you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
> >>>>>>in watchdog_core.c whereas it is only used for device
> >>>>>>creation/deletion.
> >>>>>>
> >>>>>The class is watchdog driver internal, and it is device related, so
> >>>>>I think it made sense to move it to watchdog_dev.c. On top of that,
> >>>>>it will be needed there if/when we introduce sysfs attributes.
> >>>>>
> >>>>>The watchdog id can be determined by obtaining an id using ida, or
> >>>>>it can be provided through the watchdog alias. The operation to get
> >>>>>it is not device related, and it is not straightforward to obtain
> >>>>>it, so I thought it makes sense to keep the code in watchdog_core.c.
> >>>>>
> >>>>>Of course a lot of it is personal preference.
> >>>>>
> >>>>
> >>>>Let me go back to how I saw the design when I created the generic
> >>>>watchdog framework: When using watchdog device drivers we need to be
> >>>>able to support the /dev/watchdog system. I also foresaw that we
> >>>>should have a sysfs interface and I saw the future for watchdog
> >>>>devices that you should be able to choose between the 2 different
> >>>>systems. You should be able to use only the /dev/watchdog interfacing,
> >>>>but you should also be able to use both a sysfs interface and a
> >>>>/dev/watchdog interface and it should even be possible to have only a
> >>>>sysfs interface in certain embedded devices. So that's why I split the
> >>>>watchdog framework over 3 files: core code, the /dev/watchdog
> >>>>interfacing and the sysfs code. Since I want to have compiled code
> >>>>small enough when choosing either /Dev/watchdog or sysfs or both this
> >>>>sounded the most logical thing to do (Unless you have a single file
> >>>>full of #ifdef-ery that becomes unreadable).
> >>>>
> >>>>So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
> >>>>watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
> >>>>listen to it and see what the benefits are. But I want a clean system
> >>>>for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
> >>>>(watchdog_sysfs.c) in the future. Off-course the current behaviour is
> >>>>to have the /dev/ interface and have the option to add sysfs
> >>>>attributes.
> >>>
> >>>I agree that keeping sysfs code separate makes sense, as someone might
> >>>want to not use it.
> >>>
> >>I am not really sure about that. I don't recall a similar concern with
> >>any other subsystem.
> >>
> >>Anyway, sure, we can move the code to another file. Sure, we can add a
> >>configuration option. That means we'll also need to make several functions
> >>non-static, and possibly move some functions out of watchdog_dev.c
> >>into yet another file. But we'll need some guidance for that and an idea
> >>what is going to be acceptable.
> >>
> >>>The question is: can we make the /dev/watchdog entries optional ? That
> >>>would break the compatibility, right? Imho, it would be saner to keep
> >>>only one way to interact with watchdogs (ie. keep /dev/watchdog as is
> >>>and don't make it optional, and sysfs read-only and eventually
> >>>optional). I think that question should be answered before we can decide
> >>>how we want to split the code between watchdog_dev.c and watchdog_core.c
> >>>
> >>
> >>All I know for my part is that all pending structural enhancements in one
> >>way or another depend on the assumption that the character device and the
> >>sysfs (kernel) device both exist. I have not been able to find any code
> >>in the kernel where cdev_add is not associated with device_create, so I
> >>don't even know if cdev_add without device_create is even possible.
> >>The current code in watchdog_dev.c definitely depends on watchdog->dev,
> >>though not deeply (it uses it for some dev_ messages). If that dependency
> >>isn't supposed to exist, we'll need to do something about it now.
> >>
> >>We now have the sysfs changes, the changes to move "soft" timeout handling
> >>into the kernel, multiple approaches to add pretimeout into the watchdog
> >>core,
> >>and my attempt to solve the ref/unref mess. All of those won't be able to
> >>proceed without knowing how to handle the cdev vs. dev problem.
> >>
> >>Wim, we really need some guidance from you.
> >
> >Basically the options we have are:
> >1) go with the original design. The problem here (as Guenter pointed it out
> >correctly) is that sysfs went from optional to integrated with the character
> >devices.
> >2) change the origial design so that the watchdog_sysfs part is still optional
> >(meaning the additional read-only sysfs attributes) but is called from within
> >watchdog_dev.c instead of from watchdog_core.c . This would make it possible
> >to avoid race conditions. This would also mean that it is harder for an
> >embedded developer to get rid of the /dev/watchdog interface.
> >3) put the sysfs attributes together with the /dev/watchdog code in
> >watchdog_dev.c . The sysfs attributes can be optional with a minimum of
> >#ifdef's if needed.
> >4) consolidate everything in only watchdog_core.c . (if we put the sysfs and
> >the /dev/watchdog* code in 1 file, then why not put everything in one file).
> >
> >For the linux-kernel leaving the dev/watchdog* interface and only use the sysfs
> >interface (but then read-write so that you can use the interface also for
> >starting and stopping a watchdog) would indeed mean a compatibility break.
> >But as an embedded developer it would not be a necessity and thus not a
> >compatibility issue.
> >
> >Since /dev/watchdog was always tied with the misc devices, it makes sence
> >to follow the misc route and thus the character device route. This indeed
> >means that we always have the watchdog class (but also linkage to parent
> >devices via misc.parent).
> >
> >So I think we should stick to the misc device approach (since this is how
> >it has always been). Which means that we also follow the character device
> >approach. So to me we should go for option 3 or 4.
> >
>
> I would suggest to go with 3), and also move the watchdog class to watchdog_dev.c.
> I don't see a real reason to move everything into a single file - it would just
> make code maintenance more difficult, and the current separation seems to work
> quite well as far as I can see.
If it is the final consensus then I will send my sysfs patches with changes to
protect sysfs part with some config option like CONFIG_WATCHDOG_SYSFS.
~Pratyush
Hi Guenter,
> On 12/14/2015 12:44 PM, Wim Van Sebroeck wrote:
> >On Sun, Dec 13, 2015 at 10:24:35PM -0800, Guenter Roeck wrote:
> >
> >>On 12/13/2015 02:02 PM, Damien Riegel wrote:
> >>>On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
> >>>>Hi All,
> >>>>
> >>>>>On 12/07/2015 08:15 AM, Damien Riegel wrote:
> >>>>>>On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> >>>>>>>The watchdog character device s currently created in
> >>>>>>>watchdog_dev.c, and the watchdog device in watchdog_core.c. This
> >>>>>>>results in cross-dependencies, as the device creation needs to
> >>>>>>>know the watchdog character device number.
> >>>>>>>
> >>>>>>>On top of that, the watchdog character device is created before
> >>>>>>>the watchdog device is created. This can result in race conditions
> >>>>>>>if the watchdog device node is accessed before the watchdog device
> >>>>>>>has been created.
> >>>>>>>
> >>>>>>>To solve the problem, move watchdog device creation into
> >>>>>>>watchdog_dev.c, and create the watchdog device prior to creating
> >>>>>>>its device node. Also move device class creation into
> >>>>>>>watchdog_dev.c, since this is now the only place where the
> >>>>>>>watchdog class is needed.
> >>>>>>>
> >>>>>>>Inspired by an earlier patch set from Damien Riegel.
> >>>>>>>
> >>>>>>>Cc: Damien Riegel <[email protected]>
> >>>>>>>Signed-off-by: Guenter Roeck <[email protected]> --- Hi Damien,
> >>>>>>>
> >>>>>>>I think this approach would be a bit better. The watchdog device
> >>>>>>>isn't really used in the watchdog core code, so it is better
> >>>>>>>created in watchdog_dev.c. That also fits well with other pending
> >>>>>>>changes, such as sysfs attribute support, and my attempts to move
> >>>>>>>the ref/unref functions completely into the watchdog core. As a
> >>>>>>>side effect, it also cleans up the error path in
> >>>>>>>__watchdog_register_device().
> >>>>>>>
> >>>>>>>What do you think ?
> >>>>>>
> >>>>>>Hi Guenter,
> >>>>>>
> >>>>>>Like the idea, but I don't really get the separation. For instance,
> >>>>>>you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
> >>>>>>in watchdog_core.c whereas it is only used for device
> >>>>>>creation/deletion.
> >>>>>>
> >>>>>The class is watchdog driver internal, and it is device related, so
> >>>>>I think it made sense to move it to watchdog_dev.c. On top of that,
> >>>>>it will be needed there if/when we introduce sysfs attributes.
> >>>>>
> >>>>>The watchdog id can be determined by obtaining an id using ida, or
> >>>>>it can be provided through the watchdog alias. The operation to get
> >>>>>it is not device related, and it is not straightforward to obtain
> >>>>>it, so I thought it makes sense to keep the code in watchdog_core.c.
> >>>>>
> >>>>>Of course a lot of it is personal preference.
> >>>>>
> >>>>
> >>>>Let me go back to how I saw the design when I created the generic
> >>>>watchdog framework: When using watchdog device drivers we need to be
> >>>>able to support the /dev/watchdog system. I also foresaw that we
> >>>>should have a sysfs interface and I saw the future for watchdog
> >>>>devices that you should be able to choose between the 2 different
> >>>>systems. You should be able to use only the /dev/watchdog interfacing,
> >>>>but you should also be able to use both a sysfs interface and a
> >>>>/dev/watchdog interface and it should even be possible to have only a
> >>>>sysfs interface in certain embedded devices. So that's why I split the
> >>>>watchdog framework over 3 files: core code, the /dev/watchdog
> >>>>interfacing and the sysfs code. Since I want to have compiled code
> >>>>small enough when choosing either /Dev/watchdog or sysfs or both this
> >>>>sounded the most logical thing to do (Unless you have a single file
> >>>>full of #ifdef-ery that becomes unreadable).
> >>>>
> >>>>So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
> >>>>watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
> >>>>listen to it and see what the benefits are. But I want a clean system
> >>>>for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
> >>>>(watchdog_sysfs.c) in the future. Off-course the current behaviour is
> >>>>to have the /dev/ interface and have the option to add sysfs
> >>>>attributes.
> >>>
> >>>I agree that keeping sysfs code separate makes sense, as someone might
> >>>want to not use it.
> >>>
> >>I am not really sure about that. I don't recall a similar concern with
> >>any other subsystem.
> >>
> >>Anyway, sure, we can move the code to another file. Sure, we can add a
> >>configuration option. That means we'll also need to make several functions
> >>non-static, and possibly move some functions out of watchdog_dev.c
> >>into yet another file. But we'll need some guidance for that and an idea
> >>what is going to be acceptable.
> >>
> >>>The question is: can we make the /dev/watchdog entries optional ? That
> >>>would break the compatibility, right? Imho, it would be saner to keep
> >>>only one way to interact with watchdogs (ie. keep /dev/watchdog as is
> >>>and don't make it optional, and sysfs read-only and eventually
> >>>optional). I think that question should be answered before we can decide
> >>>how we want to split the code between watchdog_dev.c and watchdog_core.c
> >>>
> >>
> >>All I know for my part is that all pending structural enhancements in one
> >>way or another depend on the assumption that the character device and the
> >>sysfs (kernel) device both exist. I have not been able to find any code
> >>in the kernel where cdev_add is not associated with device_create, so I
> >>don't even know if cdev_add without device_create is even possible.
> >>The current code in watchdog_dev.c definitely depends on watchdog->dev,
> >>though not deeply (it uses it for some dev_ messages). If that dependency
> >>isn't supposed to exist, we'll need to do something about it now.
> >>
> >>We now have the sysfs changes, the changes to move "soft" timeout handling
> >>into the kernel, multiple approaches to add pretimeout into the watchdog
> >>core,
> >>and my attempt to solve the ref/unref mess. All of those won't be able to
> >>proceed without knowing how to handle the cdev vs. dev problem.
> >>
> >>Wim, we really need some guidance from you.
> >
> >Basically the options we have are:
> >1) go with the original design. The problem here (as Guenter pointed it out
> >correctly) is that sysfs went from optional to integrated with the
> >character
> >devices.
> >2) change the origial design so that the watchdog_sysfs part is still
> >optional
> >(meaning the additional read-only sysfs attributes) but is called from
> >within
> >watchdog_dev.c instead of from watchdog_core.c . This would make it
> >possible
> >to avoid race conditions. This would also mean that it is harder for an
> >embedded developer to get rid of the /dev/watchdog interface.
> >3) put the sysfs attributes together with the /dev/watchdog code in
> >watchdog_dev.c . The sysfs attributes can be optional with a minimum of
> >#ifdef's if needed.
> >4) consolidate everything in only watchdog_core.c . (if we put the sysfs
> >and
> >the /dev/watchdog* code in 1 file, then why not put everything in one
> >file).
> >
> >For the linux-kernel leaving the dev/watchdog* interface and only use the
> >sysfs
> >interface (but then read-write so that you can use the interface also for
> >starting and stopping a watchdog) would indeed mean a compatibility break.
> >But as an embedded developer it would not be a necessity and thus not a
> >compatibility issue.
> >
> >Since /dev/watchdog was always tied with the misc devices, it makes sence
> >to follow the misc route and thus the character device route. This indeed
> >means that we always have the watchdog class (but also linkage to parent
> >devices via misc.parent).
> >
> >So I think we should stick to the misc device approach (since this is how
> >it has always been). Which means that we also follow the character device
> >approach. So to me we should go for option 3 or 4.
> >
>
> I would suggest to go with 3), and also move the watchdog class to
> watchdog_dev.c.
> I don't see a real reason to move everything into a single file - it would
> just
> make code maintenance more difficult, and the current separation seems to
> work
> quite well as far as I can see.
Then we will go with option 3.
Kind regards,
Wim.
Hi Pratyush,
> On 15/12/2015:06:56:50 PM, Guenter Roeck wrote:
> > On 12/14/2015 12:44 PM, Wim Van Sebroeck wrote:
> > >On Sun, Dec 13, 2015 at 10:24:35PM -0800, Guenter Roeck wrote:
> > >
> > >>On 12/13/2015 02:02 PM, Damien Riegel wrote:
> > >>>On Mon, Dec 07, 2015 at 09:41:03PM +0100, Wim Van Sebroeck wrote:
> > >>>>Hi All,
> > >>>>
> > >>>>>On 12/07/2015 08:15 AM, Damien Riegel wrote:
> > >>>>>>On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> > >>>>>>>The watchdog character device s currently created in
> > >>>>>>>watchdog_dev.c, and the watchdog device in watchdog_core.c. This
> > >>>>>>>results in cross-dependencies, as the device creation needs to
> > >>>>>>>know the watchdog character device number.
> > >>>>>>>
> > >>>>>>>On top of that, the watchdog character device is created before
> > >>>>>>>the watchdog device is created. This can result in race conditions
> > >>>>>>>if the watchdog device node is accessed before the watchdog device
> > >>>>>>>has been created.
> > >>>>>>>
> > >>>>>>>To solve the problem, move watchdog device creation into
> > >>>>>>>watchdog_dev.c, and create the watchdog device prior to creating
> > >>>>>>>its device node. Also move device class creation into
> > >>>>>>>watchdog_dev.c, since this is now the only place where the
> > >>>>>>>watchdog class is needed.
> > >>>>>>>
> > >>>>>>>Inspired by an earlier patch set from Damien Riegel.
> > >>>>>>>
> > >>>>>>>Cc: Damien Riegel <[email protected]>
> > >>>>>>>Signed-off-by: Guenter Roeck <[email protected]> --- Hi Damien,
> > >>>>>>>
> > >>>>>>>I think this approach would be a bit better. The watchdog device
> > >>>>>>>isn't really used in the watchdog core code, so it is better
> > >>>>>>>created in watchdog_dev.c. That also fits well with other pending
> > >>>>>>>changes, such as sysfs attribute support, and my attempts to move
> > >>>>>>>the ref/unref functions completely into the watchdog core. As a
> > >>>>>>>side effect, it also cleans up the error path in
> > >>>>>>>__watchdog_register_device().
> > >>>>>>>
> > >>>>>>>What do you think ?
> > >>>>>>
> > >>>>>>Hi Guenter,
> > >>>>>>
> > >>>>>>Like the idea, but I don't really get the separation. For instance,
> > >>>>>>you move watchdog_class in watchdog_dev.c but you keep watchdog_ida
> > >>>>>>in watchdog_core.c whereas it is only used for device
> > >>>>>>creation/deletion.
> > >>>>>>
> > >>>>>The class is watchdog driver internal, and it is device related, so
> > >>>>>I think it made sense to move it to watchdog_dev.c. On top of that,
> > >>>>>it will be needed there if/when we introduce sysfs attributes.
> > >>>>>
> > >>>>>The watchdog id can be determined by obtaining an id using ida, or
> > >>>>>it can be provided through the watchdog alias. The operation to get
> > >>>>>it is not device related, and it is not straightforward to obtain
> > >>>>>it, so I thought it makes sense to keep the code in watchdog_core.c.
> > >>>>>
> > >>>>>Of course a lot of it is personal preference.
> > >>>>>
> > >>>>
> > >>>>Let me go back to how I saw the design when I created the generic
> > >>>>watchdog framework: When using watchdog device drivers we need to be
> > >>>>able to support the /dev/watchdog system. I also foresaw that we
> > >>>>should have a sysfs interface and I saw the future for watchdog
> > >>>>devices that you should be able to choose between the 2 different
> > >>>>systems. You should be able to use only the /dev/watchdog interfacing,
> > >>>>but you should also be able to use both a sysfs interface and a
> > >>>>/dev/watchdog interface and it should even be possible to have only a
> > >>>>sysfs interface in certain embedded devices. So that's why I split the
> > >>>>watchdog framework over 3 files: core code, the /dev/watchdog
> > >>>>interfacing and the sysfs code. Since I want to have compiled code
> > >>>>small enough when choosing either /Dev/watchdog or sysfs or both this
> > >>>>sounded the most logical thing to do (Unless you have a single file
> > >>>>full of #ifdef-ery that becomes unreadable).
> > >>>>
> > >>>>So I do not agree to have sysfs code in watchdog_dev.c . It belongs in
> > >>>>watchdog_sysfs.c imho. If someone has a better idea, I'll be glad to
> > >>>>listen to it and see what the benefits are. But I want a clean system
> > >>>>for excluding both /dev/ (current watchdog_dev.c) and/or sysfs
> > >>>>(watchdog_sysfs.c) in the future. Off-course the current behaviour is
> > >>>>to have the /dev/ interface and have the option to add sysfs
> > >>>>attributes.
> > >>>
> > >>>I agree that keeping sysfs code separate makes sense, as someone might
> > >>>want to not use it.
> > >>>
> > >>I am not really sure about that. I don't recall a similar concern with
> > >>any other subsystem.
> > >>
> > >>Anyway, sure, we can move the code to another file. Sure, we can add a
> > >>configuration option. That means we'll also need to make several functions
> > >>non-static, and possibly move some functions out of watchdog_dev.c
> > >>into yet another file. But we'll need some guidance for that and an idea
> > >>what is going to be acceptable.
> > >>
> > >>>The question is: can we make the /dev/watchdog entries optional ? That
> > >>>would break the compatibility, right? Imho, it would be saner to keep
> > >>>only one way to interact with watchdogs (ie. keep /dev/watchdog as is
> > >>>and don't make it optional, and sysfs read-only and eventually
> > >>>optional). I think that question should be answered before we can decide
> > >>>how we want to split the code between watchdog_dev.c and watchdog_core.c
> > >>>
> > >>
> > >>All I know for my part is that all pending structural enhancements in one
> > >>way or another depend on the assumption that the character device and the
> > >>sysfs (kernel) device both exist. I have not been able to find any code
> > >>in the kernel where cdev_add is not associated with device_create, so I
> > >>don't even know if cdev_add without device_create is even possible.
> > >>The current code in watchdog_dev.c definitely depends on watchdog->dev,
> > >>though not deeply (it uses it for some dev_ messages). If that dependency
> > >>isn't supposed to exist, we'll need to do something about it now.
> > >>
> > >>We now have the sysfs changes, the changes to move "soft" timeout handling
> > >>into the kernel, multiple approaches to add pretimeout into the watchdog
> > >>core,
> > >>and my attempt to solve the ref/unref mess. All of those won't be able to
> > >>proceed without knowing how to handle the cdev vs. dev problem.
> > >>
> > >>Wim, we really need some guidance from you.
> > >
> > >Basically the options we have are:
> > >1) go with the original design. The problem here (as Guenter pointed it out
> > >correctly) is that sysfs went from optional to integrated with the character
> > >devices.
> > >2) change the origial design so that the watchdog_sysfs part is still optional
> > >(meaning the additional read-only sysfs attributes) but is called from within
> > >watchdog_dev.c instead of from watchdog_core.c . This would make it possible
> > >to avoid race conditions. This would also mean that it is harder for an
> > >embedded developer to get rid of the /dev/watchdog interface.
> > >3) put the sysfs attributes together with the /dev/watchdog code in
> > >watchdog_dev.c . The sysfs attributes can be optional with a minimum of
> > >#ifdef's if needed.
> > >4) consolidate everything in only watchdog_core.c . (if we put the sysfs and
> > >the /dev/watchdog* code in 1 file, then why not put everything in one file).
> > >
> > >For the linux-kernel leaving the dev/watchdog* interface and only use the sysfs
> > >interface (but then read-write so that you can use the interface also for
> > >starting and stopping a watchdog) would indeed mean a compatibility break.
> > >But as an embedded developer it would not be a necessity and thus not a
> > >compatibility issue.
> > >
> > >Since /dev/watchdog was always tied with the misc devices, it makes sence
> > >to follow the misc route and thus the character device route. This indeed
> > >means that we always have the watchdog class (but also linkage to parent
> > >devices via misc.parent).
> > >
> > >So I think we should stick to the misc device approach (since this is how
> > >it has always been). Which means that we also follow the character device
> > >approach. So to me we should go for option 3 or 4.
> > >
> >
> > I would suggest to go with 3), and also move the watchdog class to watchdog_dev.c.
> > I don't see a real reason to move everything into a single file - it would just
> > make code maintenance more difficult, and the current separation seems to work
> > quite well as far as I can see.
>
> If it is the final consensus then I will send my sysfs patches with changes to
> protect sysfs part with some config option like CONFIG_WATCHDOG_SYSFS.
Yes, that is the final consensus.
Kind regards,
Wim.