[kbuild-all] Re: [groeck-staging:watchdog-next 12/32] drivers/watchdog/s3c2410_wdt.c:663 s3c2410wdt_probe() warn: passing zero to 'PTR_ERR'

On 12/10/21 3:43 AM, Dan Carpenter wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git watchdog-next > head: 59a29872ed5c746bba5898ed8e77c3e33d3aa9b6 > commit: 5c0145c7f9262dfd7085239eca95b15967c539fe [12/32] watchdog: s3c2410: Support separate source clock > config: nios2-randconfig-m031-20211202 (https://download.01.org/0day-ci/archive/20211209/202112091052.ZSHK1XkV-lk...) > compiler: nios2-linux-gcc (GCC) 11.2.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp(a)intel.com&gt; > Reported-by: Dan Carpenter <dan.carpenter(a)oracle.com&gt; > > smatch warnings: > drivers/watchdog/s3c2410_wdt.c:663 s3c2410wdt_probe() warn: passing zero to 'PTR_ERR' > > vim +/PTR_ERR +663 drivers/watchdog/s3c2410_wdt.c > > 2d991a164a6185 drivers/watchdog/s3c2410_wdt.c Bill Pemberton 2012-11-19 601 static int s3c2410wdt_probe(struct platform_device *pdev) > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 602 { > 08497f22b15aff drivers/watchdog/s3c2410_wdt.c Krzysztof Kozlowski 2017-03-13 603 struct device *dev = &pdev->dev; > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 604 struct s3c2410_wdt *wdt; > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 605 struct resource *wdt_irq; > 46b814d6e00c1a drivers/char/watchdog/s3c2410_wdt.c Ben Dooks 2007-06-14 606 unsigned int wtcon; > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 607 int ret; > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 608 > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 609 wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 610 if (!wdt) > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 611 return -ENOMEM; > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 612 > 08497f22b15aff drivers/watchdog/s3c2410_wdt.c Krzysztof Kozlowski 2017-03-13 613 wdt->dev = dev; > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 614 spin_lock_init(&wdt->lock); > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 615 wdt->wdt_device = s3c2410_wdd; > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 616 > e3a60ead2c9b81 drivers/watchdog/s3c2410_wdt.c Krzysztof Kozlowski 2017-02-24 617 wdt->drv_data = s3c2410_get_wdt_drv_data(pdev); > cffc9a60ebac3b drivers/watchdog/s3c2410_wdt.c Doug Anderson 2013-12-06 618 if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) { > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 619 wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node, > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 620 "samsung,syscon-phandle"); > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 621 if (IS_ERR(wdt->pmureg)) { > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 622 dev_err(dev, "syscon regmap lookup failed.\n"); > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 623 return PTR_ERR(wdt->pmureg); > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 624 } > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 625 } > 4f1f653a68d67c drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-12-06 626 > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 627 wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 628 if (wdt_irq == NULL) { > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 629 dev_err(dev, "no irq resource specified\n"); > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 630 ret = -ENOENT; > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 631 goto err; > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 632 } > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 633 > 78d3e00bb0bcfb drivers/watchdog/s3c2410_wdt.c MyungJoo Ham 2012-01-13 634 /* get the memory region for the watchdog timer */ > 0f0a6a285ec0c7 drivers/watchdog/s3c2410_wdt.c Guenter Roeck 2019-04-02 635 wdt->reg_base = devm_platform_ioremap_resource(pdev, 0); > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 636 if (IS_ERR(wdt->reg_base)) { > af4ea6312cebd7 drivers/watchdog/s3c2410_wdt.c Leela Krishna Amudala 2013-08-27 637 ret = PTR_ERR(wdt->reg_base); > 04ecc7dc8ee625 drivers/watchdog/s3c2410_wdt.c Jingoo Han 2013-01-10 638 goto err; > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 639 } > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 640 > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 641 wdt->bus_clk = devm_clk_get(dev, "watchdog"); > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 642 if (IS_ERR(wdt->bus_clk)) { > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 643 dev_err(dev, "failed to find bus clock\n"); > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 644 ret = PTR_ERR(wdt->bus_clk); > 04ecc7dc8ee625 drivers/watchdog/s3c2410_wdt.c Jingoo Han 2013-01-10 645 goto err; > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 646 } > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 647 > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 648 ret = clk_prepare_enable(wdt->bus_clk); > 01b6af91593629 drivers/watchdog/s3c2410_wdt.c Sachin Kamat 2014-03-04 649 if (ret < 0) { > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 650 dev_err(dev, "failed to enable bus clock\n"); > 01b6af91593629 drivers/watchdog/s3c2410_wdt.c Sachin Kamat 2014-03-04 651 return ret; > 01b6af91593629 drivers/watchdog/s3c2410_wdt.c Sachin Kamat 2014-03-04 652 } > ^1da177e4c3f41 drivers/char/watchdog/s3c2410_wdt.c Linus Torvalds 2005-04-16 653 > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 654 /* > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 655 * "watchdog_src" clock is optional; if it's not present -- just skip it > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 656 * and use "watchdog" clock as both bus and source clock. > > That's not the right way to understand "optional". > > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 657 */ > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 658 wdt->src_clk = devm_clk_get(dev, "watchdog_src"); > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 659 if (!IS_ERR(wdt->src_clk)) { > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 660 ret = clk_prepare_enable(wdt->src_clk); > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 661 if (ret < 0) { > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 662 dev_err(dev, "failed to enable source clock\n"); > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 @663 ret = PTR_ERR(wdt->src_clk); > > Delete this assignment. "ret" is already the correct error code. > Most definitely, that is correct. > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 664 goto err_bus_clk; > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 665 } > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 666 } else { > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 667 wdt->src_clk = NULL; > 5c0145c7f9262d drivers/watchdog/s3c2410_wdt.c Sam Protsenko 2021-11-07 668 } > > "Optional" doesn't mean the kernel can ignore errors. It means it's up > to the user. If the user doesn't want an optional feature, then > devm_clk_get() will return NULL. Otherwise errors need to be reported > to the user. Imagine if this code returns -EPROBE_DEFER for example. > In other words the way to implement this is: > > wdt->src_clk = devm_clk_get(dev, "watchdog_src"); > if (IS_ERR(wdt->src_clk)) { > dev_err(dev, "failed to get watchdog clock\n"); > ret = PTR_ERR(wdt->src_clk); > goto err_bus_clk; > } > > ret = clk_prepare_enable(wdt->src_clk); > if (ret) { > dev_err(dev, "failed to enable source clock\n"); > goto err_bus_clk; > } > > Maybe there is an argument for continuing if PTR_ERR(wdt->src_clk) == -EINVAL, > but I don't know the code well enough to say for sure? Normally, when > the kernel has an error then we should just return the error code and > let the user fix their system or disable the feature. > I am not sure if devm_clk_get() ever returns NULL. The code should probably use devm_clk_get_optional(), which explicitly does return NULL if the clock is not there. wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src"); if (IS_ERR(wdt->src_clk)) { err = PTR_ERR(wdt->src_clk); goto err_bus_clk; } ... It should probably also use dev_err_probe() for other error handling messages in the probe function to avoid spurious error messages if a function returns -EPROBE_DEFER.