diff mbox series

watchdog: xilinx_wwdt: Add check for timeout limit and set maximum value if exceeded

Message ID 20240319111219.21094-1-harini.t@amd.com
State New
Headers show
Series watchdog: xilinx_wwdt: Add check for timeout limit and set maximum value if exceeded | expand

Commit Message

T, Harini March 19, 2024, 11:12 a.m. UTC
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@amd.com>
---
 drivers/watchdog/xilinx_wwdt.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

Comments

Guenter Roeck March 19, 2024, 2:07 p.m. UTC | #1
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@amd.com>
> ---
>   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);
T, Harini May 8, 2024, 3:17 p.m. UTC | #2
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@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, March 19, 2024 7:37 PM
> To: T, Harini <Harini.T@amd.com>; Simek, Michal
> <michal.simek@amd.com>; Neeli, Srinivas <srinivas.neeli@amd.com>; Datta,
> Shubhrajyoti <shubhrajyoti.datta@amd.com>
> Cc: Goud, Srinivas <srinivas.goud@amd.com>; wim@linux-watchdog.org;
> linux-watchdog@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> 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@amd.com>
> > ---
> >   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);
diff mbox series

Patch

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);
 	spin_lock_init(&xdev->spinlock);
 	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
 	watchdog_set_nowayout(xilinx_wwdt_wdd, 1);