Re: [PATCH 1/2] watchdog: bcm281xx: Watchdog Driver

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

On 11/12/2013 02:17 PM, Markus Mayer wrote:

>>> +
>>> +     if (!timeout)
>>> +             dev_info(wdog->dev, "Watchdog timer stopped");
>>> +
>> All that noise.
>
> Would it be acceptable to turn these calls into dev_dbg() calls, here
> and elsewhere?
>

Ok with me.


>>> +
>>> +     wdt->resolution = SECWDOG_DEFAULT_RESOLUTION;
>>> +     ret = bcm_kona_wdt_set_resolution_reg(wdt);
>>> +     if (ret) {
>>> +             dev_err(dev, "Failed to set resolution (error: %d)", ret);
>>
>> ret can be -EAGAIN or -EINVAL. -EINVAL suggests a bad internale error (hopefully
>> SECWDOG_DEFAULT_RESOLUTION is defined to be smaller than SECWDOG_MAX_RES),
>> and if it is -EAGAIN there should be no error message.
>>
>> Actually, bcm_kona_wdt_set_resolution_reg is only called from here, meaning the
>> error check in the function is really unnecessary.
>
> This again goes back to making resolution available to userland. Then
> bcm_kona_wdt_set_resolution_reg() would be called from elsewhere. Why
> is it bad to print an error message on timeout? Would this still apply

That was related to -EAGAIN. Which would be bad here anyway as it could result
in an endless loop if there is a problem with the chip.

> if I switch the code to -ETIMEDOUT?
>
That is one option, or -EIO if the condition indicates a chip error.

>>> +             return ret;
>>> +     }
>>> +
>>> +     spin_lock_init(&wdt->lock);
>>> +     platform_set_drvdata(pdev, wdt);
>>> +     watchdog_set_drvdata(&bcm_kona_wdt_wdd, wdt);
>>> +
>>> +     ret = bcm_kona_wdt_set_timeout_reg(&bcm_kona_wdt_wdd);
>>> +     if (ret) {
>>> +             dev_err(dev, "Failed set watchdog timeout");
>>
>> The only error case is -EAGAIN. I don't think there should be an error mesasge
>> in this case (though I am not sure what the reaction should be).
>
> I am thinking that probe() needs to return an error if setting the
> timeout fails, as it can't really rely on the watchdog timer or let
> the system use it. Shouldn't that be accompanied by an error message
> letting the user know what happened?
>
Oh, I agree it should return an error, and an error message is ok as well.
I am just sure it should not be -EAGAIN, but I don't know what it should be.
Maybe -ETIMEDOUT, or -EIO.

>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = watchdog_register_device(&bcm_kona_wdt_wdd);
>>> +     if (ret) {
>>> +             dev_err(dev, "Failed to register watchdog device");
>>> +             return ret;
>>> +     }
>>> +
>>> +#ifdef CONFIG_BCM_KONA_WDT_DEBUG
>>> +     wdt->debugfs = bcm_kona_wdt_debugfs_init(wdt, &bcm_kona_wdt_wdd);
>>> +#endif
>>> +     dev_info(dev, "Broadcom Kona Watchdog Timer");
>>> +
>> Such messages are in general considered nuisance nowadays. I would suggest to
>> remove it (or ask Greg KH for advice).
>>

Referring to your other mail.... seems those messages are falling out of favor.
I consider it a nuisance, though so far I let it go through. The messages do
increase boot time, especially on systems with slow serial console, and IMO
do not provide any real value. Users either don't care, or can check if the
driver is loaded by other means. I would suggest to at least make it dev_dbg.

Thanks,
Guenter