Hi Guenter,
Thanks for the inputs. I understand that timeout is independent of maximum hardware timeout. The other checks were added to prevent truncation of bits in the register when it crosses 32bits. I will fix and send v2.
Thanks,
Harini T
> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Tuesday, March 19, 2024 7:37 PM
> To: T, Harini <Harini.T@xxxxxxx>; Simek, Michal
> <michal.simek@xxxxxxx>; Neeli, Srinivas <srinivas.neeli@xxxxxxx>; Datta,
> Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>
> Cc: Goud, Srinivas <srinivas.goud@xxxxxxx>; wim@xxxxxxxxxxxxxxxxxx;
> linux-watchdog@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>
> Subject: Re: [PATCH] watchdog: xilinx_wwdt: Add check for timeout limit and
> set maximum value if exceeded
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On 3/19/24 04:12, Harini T wrote:
> > Current implementation fails to verify if the user input such as
> > timeout or closed window percentage exceeds the maximum value that
> the
> > hardware supports.
> >
> > Maximum timeout is derived based on input clock frequency.
> > If the user input timeout exceeds the maximum timeout supported, limit
> > the timeout to maximum supported value.
> > Limit the close and open window percent to hardware supported value.
> >
> > Signed-off-by: Harini T <harini.t@xxxxxxx>
> > ---
> > drivers/watchdog/xilinx_wwdt.c | 30 +++++++++++++++++++++++++++++-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/xilinx_wwdt.c
> > b/drivers/watchdog/xilinx_wwdt.c index d271e2e8d6e2..86e2edc4f3c7
> > 100644
> > --- a/drivers/watchdog/xilinx_wwdt.c
> > +++ b/drivers/watchdog/xilinx_wwdt.c
> > @@ -36,6 +36,12 @@
> >
> > #define XWWDT_CLOSE_WINDOW_PERCENT 50
> >
> > +/* Maximum count value of each 32 bit window */
> > +#define XWWDT_MAX_COUNT_WINDOW GENMASK(31, 0)
> > +
> > +/* Maximum count value of closed and open window combined*/
> #define
> > +XWWDT_MAX_COUNT_WINDOW_COMBINED GENMASK_ULL(32, 1)
> > +
> > static int wwdt_timeout;
> > static int closed_window_percent;
> >
> > @@ -73,6 +79,24 @@ static int xilinx_wwdt_start(struct watchdog_device
> *wdd)
> > /* Calculate timeout count */
> > time_out = xdev->freq * wdd->timeout;
> > closed_timeout = div_u64(time_out * xdev->close_percent, 100);
> > +
> > + if (time_out > XWWDT_MAX_COUNT_WINDOW) {
> > + u64 min_close_timeout = time_out -
> XWWDT_MAX_COUNT_WINDOW;
> > + u64 max_close_timeout = XWWDT_MAX_COUNT_WINDOW;
> > +
> > + if (closed_timeout > max_close_timeout) {
> > + dev_info(xilinx_wwdt_wdd->parent,
> > + "Closed window cannot be set to %d%%. Using
> maximum supported value.\n",
> > + xdev->close_percent);
> > + closed_timeout = max_close_timeout;
> > + } else if (closed_timeout < min_close_timeout) {
> > + dev_info(xilinx_wwdt_wdd->parent,
> > + "Closed window cannot be set to %d%%. Using
> minimum supported value.\n",
> > + xdev->close_percent);
> > + closed_timeout = min_close_timeout;
> > + }
> > + }
> > +
> > open_timeout = time_out - closed_timeout;
> > wdd->min_hw_heartbeat_ms = xdev->close_percent * 10 *
> > wdd->timeout;
> >
> > @@ -132,6 +156,7 @@ static int xwwdt_probe(struct platform_device
> *pdev)
> > {
> > struct watchdog_device *xilinx_wwdt_wdd;
> > struct device *dev = &pdev->dev;
> > + unsigned int max_hw_heartbeat;
> > struct xwwdt_device *xdev;
> > struct clk *clk;
> > int ret;
> > @@ -157,9 +182,11 @@ static int xwwdt_probe(struct platform_device
> *pdev)
> > if (!xdev->freq)
> > return -EINVAL;
> >
> > + max_hw_heartbeat =
> div64_u64(XWWDT_MAX_COUNT_WINDOW_COMBINED,
> > + xdev->freq);
> > +
> > xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
> > xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
> > - xilinx_wwdt_wdd->max_hw_heartbeat_ms = 1000 * xilinx_wwdt_wdd-
> >timeout;
> > + xilinx_wwdt_wdd->max_hw_heartbeat_ms = 1000 *
> max_hw_heartbeat;
> >
> > if (closed_window_percent == 0 || closed_window_percent >= 100)
> > xdev->close_percent = XWWDT_CLOSE_WINDOW_PERCENT; @@
> > -167,6 +194,7 @@ static int xwwdt_probe(struct platform_device *pdev)
> > xdev->close_percent = closed_window_percent;
> >
> > watchdog_init_timeout(xilinx_wwdt_wdd, wwdt_timeout,
> > &pdev->dev);
> > + xilinx_wwdt_wdd->timeout =
> > + min_not_zero(xilinx_wwdt_wdd->timeout, max_hw_heartbeat);
>
> I have not tried to understand the rest of the code, but this is just wrong.
> The whole point of having max_hw_heartbeat_ms is to make the actual
> timeout independent of the maximum hardware timeout.
>
> As for the rest of the changes, max_hw_heartbeat_ms should be set to the
> maximum hardware timeout. Similar, the minimum timeout should be set
> to the minimum timeout possible. As such, the checks added above should
> not be necessary. Something looks wrong, but I won't spend time trying to
> understand this because, again, limiting the actual timeout to
> max_hw_heartbeat is just wrong.
>
> Guenter
>
> > spin_lock_init(&xdev->spinlock);
> > watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
> > watchdog_set_nowayout(xilinx_wwdt_wdd, 1);
