From: Guenter Roeck <linux@roeck-us.net> To: Timo Kokkonen <timo.kokkonen@offcode.fi> Cc: linux-arm-kernel@lists.infradead.org, linux-watchdog@vger.kernel.org, boris.brezillon@free-electrons.com, nicolas.ferre@atmel.com, alexandre.belloni@free-electrons.com, Wenyou.Yang@atmel.com Subject: Re: [PATCHv7 1/8] watchdog: Extend kernel API to know about HW limitations Date: Fri, 24 Apr 2015 10:08:48 -0700 [thread overview] Message-ID: <20150424170848.GA2505@roeck-us.net> (raw) In-Reply-To: <1429701102-22320-2-git-send-email-timo.kokkonen@offcode.fi> On Wed, Apr 22, 2015 at 02:11:35PM +0300, Timo Kokkonen wrote: > There is a great deal of diversity in the watchdog hardware found on > different devices. Differen hardware have different contstraints on > them, many of the constraints that are excessively difficult for the > user space to satisfy. > > One such constraint is the length of the timeout value, which in many > cases can be just a few seconds. Drivers are creating ad hoc solutions > with timers and workqueues to extend the timeout in order to give user > space more time between updates. Looking at the drivers it is clear > that this has resulted to a lot of duplicate code. > > Add an extension to the watchdog kernel API that allows the driver to > describe tis HW constraints to the watchdog code. A kernel worker in > the core is then used to extend the watchdog timeout on behalf of the > user space. This allows the drivers to be simplified as core takes > over the timer extending. > > Tested-by: Wenyou Yang <wenyou.yang@atmel.com> > Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi> I started twice to review this series, and each time there is a new version before I can finish the review. Guess I'll wait until it settles down a bit before trying again :-(. Just a quick comment below. > --- > drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++-- > drivers/watchdog/watchdog_dev.c | 75 ++++++++++++++++++++++++++--- > include/linux/watchdog.h | 23 +++++++++ > 3 files changed, 189 insertions(+), 10 deletions(-) > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index cec9b55..fd12489 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd, > EXPORT_SYMBOL_GPL(watchdog_init_timeout); > > /** > + * watchdog_init_parms() - initialize generic watchdog parameters > + * @wdd: Watchdog device to operate > + * @dev: Device that stores the device tree properties > + * > + * Initialize the generic timeout parameters. The driver needs to set > + * hw_features bitmask from @wdd prior calling this function in order > + * to ensure the core knows how to handle the HW. > + * > + * A zero is returned on success and -EINVAL for failure. > + */ > +int watchdog_init_params(struct watchdog_device *wdd, struct device *dev) > +{ > + int ret = 0; > + > + ret = watchdog_init_timeout(wdd, wdd->timeout, dev); You are changing the semantics of watchdog_init_timeout here; for all practical purposes it no longer accepts the timeout passed as parameter, but expects the timeout to be configured in wdd->timeout instead. Please don't do that. Guenter
WARNING: multiple messages have this Message-ID (diff)
From: linux@roeck-us.net (Guenter Roeck) To: linux-arm-kernel@lists.infradead.org Subject: [PATCHv7 1/8] watchdog: Extend kernel API to know about HW limitations Date: Fri, 24 Apr 2015 10:08:48 -0700 [thread overview] Message-ID: <20150424170848.GA2505@roeck-us.net> (raw) In-Reply-To: <1429701102-22320-2-git-send-email-timo.kokkonen@offcode.fi> On Wed, Apr 22, 2015 at 02:11:35PM +0300, Timo Kokkonen wrote: > There is a great deal of diversity in the watchdog hardware found on > different devices. Differen hardware have different contstraints on > them, many of the constraints that are excessively difficult for the > user space to satisfy. > > One such constraint is the length of the timeout value, which in many > cases can be just a few seconds. Drivers are creating ad hoc solutions > with timers and workqueues to extend the timeout in order to give user > space more time between updates. Looking at the drivers it is clear > that this has resulted to a lot of duplicate code. > > Add an extension to the watchdog kernel API that allows the driver to > describe tis HW constraints to the watchdog code. A kernel worker in > the core is then used to extend the watchdog timeout on behalf of the > user space. This allows the drivers to be simplified as core takes > over the timer extending. > > Tested-by: Wenyou Yang <wenyou.yang@atmel.com> > Signed-off-by: Timo Kokkonen <timo.kokkonen@offcode.fi> I started twice to review this series, and each time there is a new version before I can finish the review. Guess I'll wait until it settles down a bit before trying again :-(. Just a quick comment below. > --- > drivers/watchdog/watchdog_core.c | 101 +++++++++++++++++++++++++++++++++++++-- > drivers/watchdog/watchdog_dev.c | 75 ++++++++++++++++++++++++++--- > include/linux/watchdog.h | 23 +++++++++ > 3 files changed, 189 insertions(+), 10 deletions(-) > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index cec9b55..fd12489 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -99,6 +99,89 @@ int watchdog_init_timeout(struct watchdog_device *wdd, > EXPORT_SYMBOL_GPL(watchdog_init_timeout); > > /** > + * watchdog_init_parms() - initialize generic watchdog parameters > + * @wdd: Watchdog device to operate > + * @dev: Device that stores the device tree properties > + * > + * Initialize the generic timeout parameters. The driver needs to set > + * hw_features bitmask from @wdd prior calling this function in order > + * to ensure the core knows how to handle the HW. > + * > + * A zero is returned on success and -EINVAL for failure. > + */ > +int watchdog_init_params(struct watchdog_device *wdd, struct device *dev) > +{ > + int ret = 0; > + > + ret = watchdog_init_timeout(wdd, wdd->timeout, dev); You are changing the semantics of watchdog_init_timeout here; for all practical purposes it no longer accepts the timeout passed as parameter, but expects the timeout to be configured in wdd->timeout instead. Please don't do that. Guenter
next prev parent reply other threads:[~2015-04-25 11:50 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-04-22 11:11 [PATCHv7 0/8] watchdog: Extend kernel API and add early_timeout_sec feature Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-04-22 11:11 ` [PATCHv7 1/8] watchdog: Extend kernel API to know about HW limitations Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-04-24 17:08 ` Guenter Roeck [this message] 2015-04-24 17:08 ` Guenter Roeck 2015-04-27 5:41 ` Timo Kokkonen 2015-04-27 5:41 ` Timo Kokkonen 2015-05-04 7:58 ` Uwe Kleine-König 2015-05-04 7:58 ` Uwe Kleine-König 2015-05-04 9:40 ` Timo Kokkonen 2015-05-04 9:40 ` Timo Kokkonen 2015-05-04 15:43 ` Guenter Roeck 2015-05-04 15:43 ` Guenter Roeck 2015-05-05 6:26 ` Timo Kokkonen 2015-05-05 6:26 ` Timo Kokkonen 2015-05-04 21:17 ` Marc Kleine-Budde 2015-05-04 21:17 ` Marc Kleine-Budde 2015-04-22 11:11 ` [PATCHv7 2/8] watchdog: Allow watchdog to reset device at early boot Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-04-22 11:11 ` [PATCHv7 3/8] devicetree: Document generic watchdog properties Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-04-22 11:11 ` [PATCHv7 4/8] Documentation/watchdog: watchdog-test.c: Add support for changing timeout Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-04-22 11:11 ` [PATCHv7 5/8] watchdog: at91sam9_wdt: Convert to use new watchdog core extensions Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-04-22 11:11 ` [PATCHv7 6/8] watchdog: imx2_wdt: Convert to use new " Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-05-05 8:11 ` Marc Kleine-Budde 2015-05-05 8:11 ` Marc Kleine-Budde 2015-05-05 8:31 ` Marc Kleine-Budde 2015-05-05 8:31 ` Marc Kleine-Budde 2015-05-05 9:07 ` Timo Kokkonen 2015-05-05 9:07 ` Timo Kokkonen 2015-04-22 11:11 ` [PATCHv7 7/8] watchdog: omap_wdt: Fix memory leak on probe fail Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-04-26 15:32 ` Guenter Roeck 2015-04-26 15:32 ` Guenter Roeck 2015-04-27 5:50 ` Timo Kokkonen 2015-04-27 5:50 ` Timo Kokkonen 2015-04-22 11:11 ` [PATCHv7 8/8] watchdog: omap_wdt: Convert to use new core extensions Timo Kokkonen 2015-04-22 11:11 ` Timo Kokkonen 2015-05-03 18:56 ` Uwe Kleine-König 2015-05-03 18:56 ` Uwe Kleine-König 2015-05-04 5:59 ` Timo Kokkonen 2015-05-04 5:59 ` Timo Kokkonen 2015-05-04 7:04 ` Uwe Kleine-König 2015-05-04 7:04 ` Uwe Kleine-König 2015-05-04 10:06 ` Timo Kokkonen 2015-05-04 10:06 ` Timo Kokkonen 2015-05-07 6:42 ` Timo Kokkonen 2015-05-07 6:42 ` Timo Kokkonen 2015-05-07 7:30 ` Uwe Kleine-König 2015-05-07 7:30 ` Uwe Kleine-König 2015-05-07 7:39 ` Timo Kokkonen 2015-05-07 7:39 ` Timo Kokkonen 2015-05-04 16:08 ` Guenter Roeck 2015-05-04 16:08 ` Guenter Roeck 2015-05-05 13:50 ` [PATCHv7 0/8] watchdog: Extend kernel API and add early_timeout_sec feature Uwe Kleine-König 2015-05-05 13:50 ` Uwe Kleine-König 2015-05-06 7:26 ` Timo Kokkonen 2015-05-06 7:26 ` Timo Kokkonen 2015-05-06 7:48 ` Uwe Kleine-König 2015-05-06 7:48 ` Uwe Kleine-König 2015-05-06 8:23 ` Timo Kokkonen 2015-05-06 8:23 ` Timo Kokkonen
Reply instructions: You may reply publicly 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=20150424170848.GA2505@roeck-us.net \ --to=linux@roeck-us.net \ --cc=Wenyou.Yang@atmel.com \ --cc=alexandre.belloni@free-electrons.com \ --cc=boris.brezillon@free-electrons.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-watchdog@vger.kernel.org \ --cc=nicolas.ferre@atmel.com \ --cc=timo.kokkonen@offcode.fi \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.