| Message ID | 20190518212801.31010-12-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | watchdog: move 'registration failed' messages into core | expand |
Hi Wolfram, On Sun, May 19 2019, Wolfram Sang wrote: > The core will print out details now. devm_watchdog_register_device() might return -ENOMEM when devres_alloc() fails without printing anything. You might consider that a non-issue since small memory allocation never fail in practice[1]. But then __watchdog_unregister_device() does some sanity checks, potentially returning -EINVAL without any print: if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL) return -EINVAL; /* Mandatory operations need to be supported */ if (!wdd->ops->start || (!wdd->ops->stop && !wdd->max_hw_heartbeat_ms)) return -EINVAL; Do you consider that not important/likely enough to be worth an error message in the driver? baruch [1] https://lwn.net/Articles/627419/ > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/watchdog/digicolor_wdt.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/watchdog/digicolor_wdt.c b/drivers/watchdog/digicolor_wdt.c > index 8af6e9a67d0d..33cda95bd238 100644 > --- a/drivers/watchdog/digicolor_wdt.c > +++ b/drivers/watchdog/digicolor_wdt.c > @@ -141,13 +141,7 @@ static int dc_wdt_probe(struct platform_device *pdev) > watchdog_set_restart_priority(&dc_wdt_wdd, 128); > watchdog_init_timeout(&dc_wdt_wdd, timeout, dev); > watchdog_stop_on_reboot(&dc_wdt_wdd); > - ret = devm_watchdog_register_device(dev, &dc_wdt_wdd); > - if (ret) { > - dev_err(dev, "Failed to register watchdog device"); > - return ret; > - } > - > - return 0; > + return devm_watchdog_register_device(dev, &dc_wdt_wdd); > } > > static const struct of_device_id dc_wdt_of_match[] = {
> Do you consider that not important/likely enough to be worth an error > message in the driver? This can be discussed as a second step IMO. I was looking at adding more error strings to the core but then wondered if we really need error messages for e.g. IDA failures. And if so, shouldn't those be in the IDA core. Do all IDA users want that? (Sidenote: to the best of my knowledge, if memory allocation fails, it will WARN you, so no need to print something in the driver.) So, I took a step back and saw that watchdog drivers mostly print "registration failed", not more. Some printed the error code. This series simplifies the current behaviour. It does not extend it. We can do that on top of it. Thanks for the comment!
On 5/19/19 1:32 AM, Wolfram Sang wrote: > >> Do you consider that not important/likely enough to be worth an error >> message in the driver? > > This can be discussed as a second step IMO. > > I was looking at adding more error strings to the core but then wondered > if we really need error messages for e.g. IDA failures. And if so, > shouldn't those be in the IDA core. Do all IDA users want that? > > (Sidenote: to the best of my knowledge, if memory allocation fails, it > will WARN you, so no need to print something in the driver.) > Correct. This is why it is discouraged to display an error message for memory allocation failures (though some maintainers want it anyway, but not me). Guenter > So, I took a step back and saw that watchdog drivers mostly print > "registration failed", not more. Some printed the error code. > > This series simplifies the current behaviour. It does not extend it. We > can do that on top of it. > > Thanks for the comment! >
diff --git a/drivers/watchdog/digicolor_wdt.c b/drivers/watchdog/digicolor_wdt.c index 8af6e9a67d0d..33cda95bd238 100644 --- a/drivers/watchdog/digicolor_wdt.c +++ b/drivers/watchdog/digicolor_wdt.c @@ -141,13 +141,7 @@ static int dc_wdt_probe(struct platform_device *pdev) watchdog_set_restart_priority(&dc_wdt_wdd, 128); watchdog_init_timeout(&dc_wdt_wdd, timeout, dev); watchdog_stop_on_reboot(&dc_wdt_wdd); - ret = devm_watchdog_register_device(dev, &dc_wdt_wdd); - if (ret) { - dev_err(dev, "Failed to register watchdog device"); - return ret; - } - - return 0; + return devm_watchdog_register_device(dev, &dc_wdt_wdd); } static const struct of_device_id dc_wdt_of_match[] = {
The core will print out details now. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/watchdog/digicolor_wdt.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)