Lowering the log level in watchdog_dev_register when err==-EBUSY

Lowering the log level in watchdog_dev_register when err==-EBUSY

[Radu Rendec]

Hello,

In a project I'm working on we have a valid use case where we activate
both the i6300esb and softdog watchdogs. We always activate i6300esb
first (which uses the "legacy" watchdog API) and then softdog. This
gets us two "error" level messages (coming from watchdog_cdev_register)
although softdog falls back to the "new" API and registers its char
device just fine.

Since watchdog_cdev_register/watchdog_dev_register seem to be used only
by watchdog_register_device and the latter always falls back to the
"new" API, I'm thinking about lowering the log level of these messages
when err is -EBUSY. Something along the lines of:

--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -928,11 +928,14 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
                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);
-                       if (err == -EBUSY)
-                               pr_err("%s: a legacy watchdog module is probably present.\n",
-                                       wdd->info->identity);
+                       if (err == -EBUSY) {
+                               pr_info("%s: cannot register miscdev on minor=%d (err=%d).\n",
+                                               wdd->info->identity, WATCHDOG_MINOR, err);
+                               pr_info("%s: a legacy watchdog module is probably present.\n",
+                                               wdd->info->identity);
+                       } else
+                               pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
+                                       wdd->info->identity, WATCHDOG_MINOR, err);
                        old_wd_data = NULL;
                        kfree(wd_data);
                        return err;

Does this look like a good approach? If not, what would you recommend?
In any case, I want to upstream the change, so better ask first :)

Thanks,
Radu Rendec

Re: Lowering the log level in watchdog_dev_register when err==-EBUSY

[Guenter Roeck]

On Wed, Oct 11, 2017 at 06:46:31PM +0100, Radu Rendec wrote:
> Hello,
> 
> In a project I'm working on we have a valid use case where we activate
> both the i6300esb and softdog watchdogs. We always activate i6300esb
> first (which uses the "legacy" watchdog API) and then softdog. This
> gets us two "error" level messages (coming from watchdog_cdev_register)
> although softdog falls back to the "new" API and registers its char
> device just fine.
> 
> Since watchdog_cdev_register/watchdog_dev_register seem to be used only
> by watchdog_register_device and the latter always falls back to the
> "new" API, I'm thinking about lowering the log level of these messages
> when err is -EBUSY. Something along the lines of:
> 
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -928,11 +928,14 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>                 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);
> -                       if (err == -EBUSY)
> -                               pr_err("%s: a legacy watchdog module is probably present.\n",
> -                                       wdd->info->identity);
> +                       if (err == -EBUSY) {
> +                               pr_info("%s: cannot register miscdev on minor=%d (err=%d).\n",
> +                                               wdd->info->identity, WATCHDOG_MINOR, err);
> +                               pr_info("%s: a legacy watchdog module is probably present.\n",
> +                                               wdd->info->identity);
> +                       } else
> +                               pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> +                                       wdd->info->identity, WATCHDOG_MINOR, err);
>                         old_wd_data = NULL;
>                         kfree(wd_data);
>                         return err;
> 
> Does this look like a good approach? If not, what would you recommend?
> In any case, I want to upstream the change, so better ask first :)
> 

I would suggest to convert the offending driver to use the watchdog subsystem
(and along the line remove the restriction of only supporting a single
instance). You have the hardware, so that should be a straightforward change.

Thanks,
Guenter

Re: Lowering the log level in watchdog_dev_register when err==-EBUSY

[Radu Rendec]

On Wed, 2017-10-11 at 11:46 -0700, Guenter Roeck wrote:
> On Wed, Oct 11, 2017 at 06:46:31PM +0100, Radu Rendec wrote:
> > In a project I'm working on we have a valid use case where we activate
> > both the i6300esb and softdog watchdogs. We always activate i6300esb
> > first (which uses the "legacy" watchdog API) and then softdog. This
> > gets us two "error" level messages (coming from watchdog_cdev_register)
> > although softdog falls back to the "new" API and registers its char
> > device just fine.
> > 
> > Since watchdog_cdev_register/watchdog_dev_register seem to be used only
> > by watchdog_register_device and the latter always falls back to the
> > "new" API, I'm thinking about lowering the log level of these messages
> > when err is -EBUSY.
>
> I would suggest to convert the offending driver to use the watchdog subsystem
> (and along the line remove the restriction of only supporting a single
> instance). You have the hardware, so that should be a straightforward change.

Thanks for the suggestion! That makes sense. I will start working on
converting i6300esb and submit a patch in a few days.

By the way, I don't have the hardware. I'm using it with KVM (Qemu),
but I guess that's good enough since I'm not going to touch any of the
code parts that deal with the hardware.

Radu

Re: Lowering the log level in watchdog_dev_register when err==-EBUSY

[Guenter Roeck]

On Thu, Oct 12, 2017 at 07:15:59PM +0100, Radu Rendec wrote:
> On Wed, 2017-10-11 at 11:46 -0700, Guenter Roeck wrote:
> > On Wed, Oct 11, 2017 at 06:46:31PM +0100, Radu Rendec wrote:
> > > In a project I'm working on we have a valid use case where we activate
> > > both the i6300esb and softdog watchdogs. We always activate i6300esb
> > > first (which uses the "legacy" watchdog API) and then softdog. This
> > > gets us two "error" level messages (coming from watchdog_cdev_register)
> > > although softdog falls back to the "new" API and registers its char
> > > device just fine.
> > > 
> > > Since watchdog_cdev_register/watchdog_dev_register seem to be used only
> > > by watchdog_register_device and the latter always falls back to the
> > > "new" API, I'm thinking about lowering the log level of these messages
> > > when err is -EBUSY.
> >
> > I would suggest to convert the offending driver to use the watchdog subsystem
> > (and along the line remove the restriction of only supporting a single
> > instance). You have the hardware, so that should be a straightforward change.
> 
> Thanks for the suggestion! That makes sense. I will start working on
> converting i6300esb and submit a patch in a few days.
> 
> By the way, I don't have the hardware. I'm using it with KVM (Qemu),
> but I guess that's good enough since I'm not going to touch any of the
> code parts that deal with the hardware.
> 
Ah, interesting. Can you send me the qemu command line ?

Thanks,
Guenter

Re: Lowering the log level in watchdog_dev_register when err==-EBUSY

[Radu Rendec]

On Thu, 2017-10-12 at 12:54 -0700, Guenter Roeck wrote:
> On Thu, Oct 12, 2017 at 07:15:59PM +0100, Radu Rendec wrote:
> > Thanks for the suggestion! That makes sense. I will start working on
> > converting i6300esb and submit a patch in a few days.
> > 
> > By the way, I don't have the hardware. I'm using it with KVM (Qemu),
> > but I guess that's good enough since I'm not going to touch any of the
> > code parts that deal with the hardware.
> > 
> 
> Ah, interesting. Can you send me the qemu command line ?

Sure, this is as easy as adding "-device i6300esb" to the qemu command
line.

For what is worth, my full command line is:

qemu-system-i386 \
	-enable-kvm \
	-hda fed26.qcow2 \
	-cdrom ~/Downloads/Fedora-Everything-netinst-i386-26-1.5.iso \
	-m 512 \
	-smp 4 \
	-netdev user,id=net0 \
	-netdev tap,id=net1,br=virbr0,helper=/usr/libexec/qemu-bridge-helper \
	-device e1000,netdev=net0 \
	-device e1000,netdev=net1 \
	-device i6300esb

Thanks,
Radu