Re: [PATCH 6/6] watchdog: Add Renesas RZ/N1 Watchdog driver

[Geert Uytterhoeven]

Hi Jean-Jacques,

On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@xxxxxxxxxxxxxxx> wrote:
> From: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
>
> This is a driver for the standard WDT on the RZ/N1 devices. This WDT has
> very limited timeout capabilities. However, it can reset the device.
> To do so, the corresponding bits in the SysCtrl RSTEN register need to
> be enabled. This is not done by this driver.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/watchdog/rzn1_wdt.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/N1 Watchdog timer.
> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even
> + * cope with 2 seconds.
> + *
> + * Copyright 2018 Renesas Electronics Europe Ltd.
> + *
> + * Derived from Ralink RT288x watchdog timer.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define RZN1_WDT_RETRIGGER                     0x0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL          0
> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK     0xfff
> +#define RZN1_WDT_RETRIGGER_PRESCALE            BIT(12)
> +#define RZN1_WDT_RETRIGGER_ENABLE              BIT(13)
> +#define RZN1_WDT_RETRIGGER_WDSI                        (0x2 << 14)
> +
> +#define RZN1_WDT_PRESCALER                     BIT(14)

"BIT(14)" is a bit strange, as this is used as a scalar, never as
a bitmask.

> +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned int t)
> +{
> +       struct rzn1_watchdog *wdt = to_rzn1_watchdog(w);
> +
> +       w->timeout = t;
> +
> +       wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER;

I guess the multiplication can overflow in 32-bit arithmetic?

> +
> +       return 0;
> +}

> +static int rzn1_wdt_probe(struct platform_device *pdev)
> +{
> +       struct rzn1_watchdog *wdt;
> +       int ret;
> +       struct device_node *np = pdev->dev.of_node;
> +       int err;
> +       struct clk *clk;
> +
> +       wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +       if (!wdt)
> +               return -ENOMEM;
> +       wdt->dev = &pdev->dev;
> +       wdt->wdt = rzn1_wdt_dev;
> +       platform_set_drvdata(pdev, wdt);
> +
> +       wdt->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(wdt->base)) {
> +               dev_err(wdt->dev, "unable to get register bank\n");

No need to print an error message, __devm_ioremap_resource() takes
care of that.

> +               return PTR_ERR(wdt->base);
> +       }
> +       wdt->irq = irq_of_parse_and_map(np, 0);
> +       if (wdt->irq == NO_IRQ) {
> +               dev_err(wdt->dev, "failed to get IRQ from DT\n");
> +               return -EINVAL;
> +       }

Please use platform_get_irq() instead. Note that on error, it prints
an error message, and returns a negative value.  So you cannot assign
it to wdt->irq before checking, as the latter is unsigned:

    ret = platform_get_irq(pdev, 0);
    if (ret < 0)
            return ret;

    wdt->irq = ret;

> +       wdt->reload_val = RZN1_WDT_MAX;
> +       wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
> +                                       (wdt->clk_rate / RZN1_WDT_PRESCALER);

To avoid loss of precision, it's better to reorder operations.
As the dividend doesn't fit in 32-bit, you have to use a
64-bit-by-unsigned-long division:

    div64_ul(RZN1_WDT_MAX * 1000ULL * RZN1_WDT_PRESCALER,
             wdt->clk_rate);

> +
> +       ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
> +       if (ret)
> +               dev_warn(&pdev->dev, "Specified timeout invalid, using default");
> +
> +       ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);

"return devm_watchdog_register_device(...);"?

> +       if (ret)
> +               goto error;
> +
> +       dev_info(wdt->dev, "Initialized\n");
> +
> +       return 0;


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds