Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
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
Thread-Topic: [PATCH v2 1/1] watchdog: atmel: atmel-sama5d4-wdt: Disable
 watchdog on system suspend
Thread-Index: AQHVIrAmpSe7ef5LPkq+gtVGV8yaCKabW/WAgAAQIfCAAAbcAIAAAenA
Date:   Fri, 14 Jun 2019 18:43:22 +0000
Message-ID: <BL0PR07MB4115D5ECDEDCC028197637E5ADEE0@BL0PR07MB4115.namprd07.prod.outlook.com>
References: <20190614125310.29458-1-ksloat@aampglobal.com>
 <20190614164609.GA29814@roeck-us.net>
 <BL0PR07MB4115E99D065FD9BEA4C43BB5ADEE0@BL0PR07MB4115.namprd07.prod.outlook.com>
 <20190614180826.GD3369@piout.net>
In-Reply-To: <20190614180826.GD3369@piout.net>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: aampglobal.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 5919de89-18ca-4172-f084-08d6f0f82dbd
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jun 2019 18:43:22.7233
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: e20e3a66-8b9e-46e9-b859-cb654c1ec6ea
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ken.sloat@aampglobal.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR07MB5057
X-MS-Exchange-CrossPremises-AuthAs: Internal
X-MS-Exchange-CrossPremises-AuthMechanism: 04
X-MS-Exchange-CrossPremises-AuthSource: BL0PR07MB4115.namprd07.prod.outlook.com
X-MS-Exchange-CrossPremises-TransportTrafficType: Email
X-MS-Exchange-CrossPremises-TransportTrafficSubType: 
X-MS-Exchange-CrossPremises-SCL: 1
X-MS-Exchange-CrossPremises-messagesource: StoreDriver
X-MS-Exchange-CrossPremises-BCC: 
X-MS-Exchange-CrossPremises-originalclientipaddress: 100.3.71.115
X-MS-Exchange-CrossPremises-transporttraffictype: Email
X-MS-Exchange-CrossPremises-transporttrafficsubtype: 
X-MS-Exchange-CrossPremises-antispam-scancontext: DIR:Originating;SFV:NSPM;SKIP:0;
X-MS-Exchange-CrossPremises-processed-by-journaling: Journal Agent
X-OrganizationHeadersPreserved: BL0PR07MB5057.namprd07.prod.outlook.com
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----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
>=20
> [This is an EXTERNAL EMAIL]
> ________________________________
>=20
> 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.
> >
>=20
> 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,=20
I'm just readily assuming it is good as it was already reviewed before.=20

So without digging into that too much, if we don't know if any of the runti=
me
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=20
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.

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

Thanks,
Ken Sloat