RE: [PATCH] watchdog: xilinx_wwdt: Add check for timeout limit and set maximum value if exceeded

From: "T, Harini" <Harini.T@amd.com>
To: Guenter Roeck <linux@roeck-us.net>,
	"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" <wim@linux-watchdog.org>,
	"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.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
Date: Wed, 8 May 2024 15:17:11 +0000	[thread overview]
Message-ID: <SJ1PR12MB6076D7F6F5DF8A6207966C7492E52@SJ1PR12MB6076.namprd12.prod.outlook.com> (raw)
In-Reply-To: <8e4c2d5a-9f28-4946-b69d-63f5af3bc3da@roeck-us.net>

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);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel