Linux-Watchdog Archive on lore.kernel.org
 help / Atom feed
From: Ken Sloat <KSloat@aampglobal.com>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	"nicolas.ferre@microchip.com" <nicolas.ferre@microchip.com>,
	"ludovic.desroches@microchip.com"
	<ludovic.desroches@microchip.com>,
	"wim@linux-watchdog.org" <wim@linux-watchdog.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Ken Sloat <KSloat@aampglobal.com>
Subject: RE: [PATCH v2 1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable watchdog on system suspend
Date: Fri, 14 Jun 2019 18:43:22 +0000
Message-ID: <BL0PR07MB4115D5ECDEDCC028197637E5ADEE0@BL0PR07MB4115.namprd07.prod.outlook.com> (raw)
In-Reply-To: <20190614180826.GD3369@piout.net>

> -----Original Message-----
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Sent: Friday, June 14, 2019 2:08 PM
> To: Ken Sloat <KSloat@aampglobal.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; nicolas.ferre@microchip.com;
> ludovic.desroches@microchip.com; wim@linux-watchdog.org; linux-arm-
> kernel@lists.infradead.org; linux-watchdog@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable
> watchdog on system suspend
> 
> [This is an EXTERNAL EMAIL]
> ________________________________
> 
> On 14/06/2019 17:53:01+0000, Ken Sloat wrote:
> > > The call to sama5d4_wdt_init() above now explicitly stops the
> > > watchdog even if we want to (re)start it. I think this would be
> > > better handled with an else case here
> > >
> > >         else
> > >                 sama5d4_wdt_stop(&wdt->wdd);
> > >
> >
> > So we completely remove the sama5d4_wdt_init() call then correct?
> >
> > To leave the code as it behaves today with the addition of wdt
> > stop/start, shouldn't we call init in the else instead?
> >
> >       if (watchdog_active(&wdt->wdd))
> >               sama5d4_wdt_start(&wdt->wdd);
> >       else
> >               sama5d4_wdt_init();
> >
> > I guess I don't really understand the purpose of having the init
> > statement in resume in the first place. I agree, calling this first
> > does end up essentially resetting the wdt it will start again if it was running
> before, but the count will be reset.
> >
> 
> There is a nice comment explaining why ;)

Well I'm a little confused still because there are two separate comments
in these statements. The first within resume implies that the init should
be called because we might have lost register values for some reason
unexplained. Then within the init it says that the bootloader might have
modified the registers so we should check them and then update it or
otherwise disable it. I'm not trying to pick apart the logic or anything, 
I'm just readily assuming it is good as it was already reviewed before. 

So without digging into that too much, if we don't know if any of the runtime
situations above might have occurred, then isn't it best to leave my patch
as is? Yes this has the side effect of resetting the timer count, but if 
the init call is needed and we don't have any way to know if any
of the situations occurred, then we have no choice right?

Happy to modify it either way, just didn't want to change
logic without understanding it thoroughly.

> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks,
Ken Sloat

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-14 12:53 Ken Sloat
2019-06-14 16:46 ` Guenter Roeck
2019-06-14 17:53   ` Ken Sloat
2019-06-14 18:08     ` Alexandre Belloni
2019-06-14 18:43       ` Ken Sloat [this message]
2019-06-14 20:33         ` Alexandre Belloni
2019-06-14 20:45           ` Ken Sloat
2019-06-15 14:22             ` Guenter Roeck

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BL0PR07MB4115D5ECDEDCC028197637E5ADEE0@BL0PR07MB4115.namprd07.prod.outlook.com \
    --to=ksloat@aampglobal.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=ludovic.desroches@microchip.com \
    --cc=nicolas.ferre@microchip.com \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox