linux-kernel - RE: [PATCH 1/2] drivers: watchdog: at91sam9

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <B256D81BAE5131468A838E5D7A243641C9F82110@penmbx01>
Date:	Wed, 29 Jul 2015 01:50:25 +0000
From:	"Yang, Wenyou" <Wenyou.Yang@...el.com>
To:	Guenter Roeck <linux@...ck-us.net>,
	"wim@...ana.be" <wim@...ana.be>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>
CC:	"sylvain.rochet@...secur.com" <sylvain.rochet@...secur.com>,
	"Ferre, Nicolas" <Nicolas.FERRE@...el.com>,
	"boris.brezillon@...e-electrons.com" 
	<boris.brezillon@...e-electrons.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature
 support

Hi Guenter,

Thank you for your prompt answer.

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@...ck-us.net]
> Sent: 2015年7月29日 9:23
> To: Yang, Wenyou; wim@...ana.be; robh+dt@...nel.org; pawel.moll@....com;
> mark.rutland@....com; ijc+devicetree@...lion.org.uk; galak@...eaurora.org
> Cc: sylvain.rochet@...secur.com; Ferre, Nicolas; boris.brezillon@...e-
> electrons.com; devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; linux-
> watchdog@...r.kernel.org; linux-arm-kernel@...ts.infradead.org
> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new feature
> support
> 
> On 07/28/2015 05:38 PM, Yang, Wenyou wrote:
> > Hi Guenter,
> >
> > Thank you very much for your review.
> >
> >> -----Original Message-----
> >> From: Guenter Roeck [mailto:linux@...ck-us.net]
> >> Sent: 2015年7月28日 15:14
> >> To: Yang, Wenyou; wim@...ana.be; robh+dt@...nel.org;
> >> pawel.moll@....com; mark.rutland@....com;
> >> ijc+devicetree@...lion.org.uk; galak@...eaurora.org
> >> Cc: sylvain.rochet@...secur.com; Ferre, Nicolas;
> >> boris.brezillon@...e- electrons.com; devicetree@...r.kernel.org;
> >> linux-kernel@...r.kernel.org; linux- watchdog@...r.kernel.org;
> >> linux-arm-kernel@...ts.infradead.org
> >> Subject: Re: [PATCH 1/2] drivers: watchdog: at91sam9_wdt: add new
> >> feature support
> >>
> >> On 07/28/2015 12:00 AM, Wenyou Yang wrote:
> >>> In the datasheet, the new feature is describled as "WDT_MR can be
> >>> written until a LOCKMR command is issued in WDT_CR".
> >>> That is to say, as long as the bootstrap and u-boot don't issue a
> >>> LOCKMR command, WDT_MR can be written in kernel.
> >>>
> >>> So the driver can enable/disable the watchdog timer hardware, set
> >>> WDV(Watchdog Counter Value) and WDD(Watchdog Delta Value) fields of
> >>> WDT_MR register to set the watchdog timer timeout.
> >>>
> >>> The timer is not necessary that regularly sends a keepalive ping to
> >>> the watchdog timer hardware.
> >>>
> >>> It is introduced from sama5d4 SoCs.
> >>>
> >> Since there are so many changes, I wonder is a separate driver would
> >> make more sense.
> > Yes, a bit many changes.
> > I thought reuse the driver code.
> > If a separate driver, I am afraid it includes much duplicated code.
> > After all, it is for the same device with different feature.
> >
> > I don't think it is necessary to have multiple drivers for the same peripheral with
> different feature.
> >
> 
> The concept for the two mechanisms is all different: In one, the watchdog
> keepalive is triggered from timer code. In the other, the watchdog timeout is
> triggered directly from the heartbeat function. One assumes that the watchdog is
> always running, and that it must be pinged even if closed. The other disables the
> watchdog on close.
> 
> What I _can_ see is that the driver is becoming an unmaintainable mess, with lots
> of if/else in pretty much every function. I consider this much less desirable than a
> bit of code duplication - if there is any. Seriously, most of the added code might as
> well be for a completely different chip.

You are right, I accepted your advice. I will rewrite it. Thanks.

> 
> Guenter

Best Regards,
Wenyou Yang