[PATCH] watchdog: sp5100_tco: Immediately trigger upon starting.

[PATCH] watchdog: sp5100_tco: Immediately trigger upon starting.

[Gregory Oakes]

The watchdog countdown is supposed to begin when the device file is
opened. Instead, it would begin countdown upon the first write to or
close of the device file. Now, the ping operation is called within the
start operation which ensures the countdown begins. From experimenation,
it does not appear possible to do this with a single write including
both the start bit and the trigger bit. So, it is done as two distinct
writes.

Signed-off-by: Gregory Oakes <gregory.oakes@amd.com>
---

I tested on several AMD client devices. I verified the countdown begins
immediately upon the open ioctl and resets upon subsequent write ioctl's.

 drivers/watchdog/sp5100_tco.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index fb426b7d81da..18ab0e096f99 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -86,6 +86,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
  * Some TCO specific functions
  */
 
+static int tco_timer_ping(struct watchdog_device *wdd);
+
 static enum tco_reg_layout tco_reg_layout(struct pci_dev *dev)
 {
 	if (dev->vendor == PCI_VENDOR_ID_ATI &&
@@ -115,7 +117,7 @@ static int tco_timer_start(struct watchdog_device *wdd)
 	val |= SP5100_WDT_START_STOP_BIT;
 	writel(val, SP5100_WDT_CONTROL(tco->tcobase));
 
-	return 0;
+	return tco_timer_ping(wdd);
 }
 
 static int tco_timer_stop(struct watchdog_device *wdd)
-- 
2.39.1

Re: [PATCH] watchdog: sp5100_tco: Immediately trigger upon starting.

[Guenter Roeck]

On 3/1/23 11:50, Gregory Oakes wrote:
> The watchdog countdown is supposed to begin when the device file is
> opened. Instead, it would begin countdown upon the first write to or
> close of the device file. Now, the ping operation is called within the
> start operation which ensures the countdown begins. From experimenation,
> it does not appear possible to do this with a single write including
> both the start bit and the trigger bit. So, it is done as two distinct
> writes.
> 
> Signed-off-by: Gregory Oakes <gregory.oakes@amd.com>
> ---
> 
> I tested on several AMD client devices. I verified the countdown begins
> immediately upon the open ioctl and resets upon subsequent write ioctl's.
> 
>   drivers/watchdog/sp5100_tco.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index fb426b7d81da..18ab0e096f99 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -86,6 +86,8 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
>    * Some TCO specific functions
>    */
>   
> +static int tco_timer_ping(struct watchdog_device *wdd);
> +
>   static enum tco_reg_layout tco_reg_layout(struct pci_dev *dev)
>   {
>   	if (dev->vendor == PCI_VENDOR_ID_ATI &&
> @@ -115,7 +117,7 @@ static int tco_timer_start(struct watchdog_device *wdd)
>   	val |= SP5100_WDT_START_STOP_BIT;
>   	writel(val, SP5100_WDT_CONTROL(tco->tcobase));
>   
> -	return 0;
> +	return tco_timer_ping(wdd);

I think something like

	val = readl(SP5100_WDT_CONTROL(tco->tcobase));
         val |= SP5100_WDT_START_STOP_BIT;
         writel(val, SP5100_WDT_CONTROL(tco->tcobase));
	val |= SP5100_WDT_TRIGGER_BIT;
	writel(val, SP5100_WDT_CONTROL(tco->tcobase));

would be better here to avoid the extra read operation.

Also please add a comment into the code explaining that a single
write operation is insufficient to avoid someone stepping in later
and trying to "optimize" the code.

Thanks,
Guenter

>   }
>   
>   static int tco_timer_stop(struct watchdog_device *wdd)