Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1416450 > unrolled thread

[PATCH v3 1/6] watchdog: add set_pretimeout interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 1/6] watchdog: add set_pretimeout interface Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:40 +0200
    Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface Guenter Roeck <linux@roeck-us.net> - 2016-06-07 22:40 +0200
    Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 08:40 +0200
      Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 15:00 +0200
        Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:20 +0200

#1416450 — [PATCH v3 1/6] watchdog: add set_pretimeout interface

From: Robin Gong <b38343@freescale.com>

Add set_pretimeout since our watchdog driver has those interfaces and
obviously, the new common watchdog framework didn't implement this
interface.

Signed-off-by: Robin Gong <b38343@freescale.com>
[vzapolskiy: rebased, added an inline comment to describe new interface]
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v2 to v3:
* none, new change

Changes from v1 to v2:
* none, new change

 include/linux/watchdog.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 51732d6..e3d23d3 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -28,6 +28,7 @@ struct watchdog_core_data;
  * @ping:	The routine that sends a keepalive ping to the watchdog device.
  * @status:	The routine that shows the status of the watchdog device.
  * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
+ * @set_pretimeout:The routine for setting the watchdog devices pretimeout (in seconds).
  * @get_timeleft:The routine that gets the time left before a reset (in seconds).
  * @restart:	The routine for restarting the machine.
  * @ioctl:	The routines that handles extra ioctl calls.
@@ -46,6 +47,7 @@ struct watchdog_ops {
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
+	int (*set_pretimeout)(struct watchdog_device *, unsigned int);
 	unsigned int (*get_timeleft)(struct watchdog_device *);
 	int (*restart)(struct watchdog_device *, unsigned long, void *);
 	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
@@ -95,6 +97,7 @@ struct watchdog_device {
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int pretimeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	unsigned int min_hw_heartbeat_ms;
@@ -162,6 +165,13 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
 		 t > wdd->max_timeout);
 }
 
+/* Use the following function to check if a pretimeout value is invalid */
+static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
+					       unsigned int t)
+{
+	return wdd->timeout && t >= wdd->timeout;
+}
+
 /* Use the following functions to manipulate watchdog driver specific data */
 static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
 {
-- 
2.5.0

[toc] | [next] | [standalone]

#1416600

On Tue, Jun 07, 2016 at 08:38:42PM +0300, Vladimir Zapolskiy wrote:
> From: Robin Gong <b38343@freescale.com>
> 
> Add set_pretimeout since our watchdog driver has those interfaces and
> obviously, the new common watchdog framework didn't implement this
> interface.
> 
> Signed-off-by: Robin Gong <b38343@freescale.com>
> [vzapolskiy: rebased, added an inline comment to describe new interface]
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
> Changes from v2 to v3:
> * none, new change
> 
> Changes from v1 to v2:
> * none, new change
> 
>  include/linux/watchdog.h | 10 ++++++++++

Please also document the new API function and variable in
Documentation/watchdog/watchdog-kernel-api.txt.


>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 51732d6..e3d23d3 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -28,6 +28,7 @@ struct watchdog_core_data;
>   * @ping:	The routine that sends a keepalive ping to the watchdog device.
>   * @status:	The routine that shows the status of the watchdog device.
>   * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
> + * @set_pretimeout:The routine for setting the watchdog devices pretimeout (in seconds).
>   * @get_timeleft:The routine that gets the time left before a reset (in seconds).
>   * @restart:	The routine for restarting the machine.
>   * @ioctl:	The routines that handles extra ioctl calls.
> @@ -46,6 +47,7 @@ struct watchdog_ops {
>  	int (*ping)(struct watchdog_device *);
>  	unsigned int (*status)(struct watchdog_device *);
>  	int (*set_timeout)(struct watchdog_device *, unsigned int);
> +	int (*set_pretimeout)(struct watchdog_device *, unsigned int);
>  	unsigned int (*get_timeleft)(struct watchdog_device *);
>  	int (*restart)(struct watchdog_device *, unsigned long, void *);
>  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> @@ -95,6 +97,7 @@ struct watchdog_device {
>  	const struct watchdog_ops *ops;
>  	unsigned int bootstatus;
>  	unsigned int timeout;
> +	unsigned int pretimeout;
>  	unsigned int min_timeout;
>  	unsigned int max_timeout;
>  	unsigned int min_hw_heartbeat_ms;
> @@ -162,6 +165,13 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
>  		 t > wdd->max_timeout);
>  }
>  
> +/* Use the following function to check if a pretimeout value is invalid */
> +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
> +					       unsigned int t)
> +{
> +	return wdd->timeout && t >= wdd->timeout;
> +}

If no timeout is configured, this would happily accept timeout values larger
than the maximum supported timeout. Given that we can not evaluate true or false
if timeout is not set or not known, I think this will have to be something like
	return !wdd->timeout || t >= wdd->timeout;
(unless somone has a better idea).

I am missing a function to initialize the pretimeout from a driver, similar
to watchdog_init_timeout(). Is this an oversight or on purpose ?

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1416914

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jun 07, 2016 at 08:38:42PM +0300, Vladimir Zapolskiy wrote:
> From: Robin Gong <b38343@freescale.com>
> 
> Add set_pretimeout since our watchdog driver has those interfaces and
> obviously, the new common watchdog framework didn't implement this
> interface.
> 
> Signed-off-by: Robin Gong <b38343@freescale.com>
> [vzapolskiy: rebased, added an inline comment to describe new interface]
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Why didn't you just take my patch and worked on it? This would have
added the documentation Guenter explicitly requested when reviewing
Robin's patch. And it would have get rid of the bug you have in the
*_invalid function. I also don't think the split-up into three patches
is necessary here, but that might just be me.

[toc] | [prev] | [next] | [standalone]

#1417378

Hi Wolfram,

On 08.06.2016 09:34, Wolfram Sang wrote:
> On Tue, Jun 07, 2016 at 08:38:42PM +0300, Vladimir Zapolskiy wrote:
>> From: Robin Gong <b38343@freescale.com>
>>
>> Add set_pretimeout since our watchdog driver has those interfaces and
>> obviously, the new common watchdog framework didn't implement this
>> interface.
>>
>> Signed-off-by: Robin Gong <b38343@freescale.com>
>> [vzapolskiy: rebased, added an inline comment to describe new interface]
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
> Why didn't you just take my patch and worked on it? This would have
> added the documentation Guenter explicitly requested when reviewing
> Robin's patch. And it would have get rid of the bug you have in the
> *_invalid function. I also don't think the split-up into three patches
> is necessary here, but that might just be me.
> 

I agree, your version is obviously more advanced, I will take it for v4.

Best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

#1418678

[Multipart message — attachments visible in raw view] — view raw

> I agree, your version is obviously more advanced, I will take it for v4.

Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web