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

Groups > linux.kernel > #1220686 > unrolled thread

Re: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in watchdog core

Started byUwe Kleine-König <u.kleine-koenig@pengutronix.de>
First post2015-09-08 12:40 +0200
Last post2015-09-09 10:00 +0200
Articles 5 — 2 participants

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

  Re: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in  watchdog core Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-09-08 12:40 +0200
    Re: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in  watchdog core Guenter Roeck <linux@roeck-us.net> - 2015-09-08 15:50 +0200
      Re: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in  watchdog core Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-09-08 22:10 +0200
        Re: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in  watchdog core Guenter Roeck <linux@roeck-us.net> - 2015-09-08 23:10 +0200
          Re: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in  watchdog core Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-09-09 10:00 +0200

#1220686 — Re: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in watchdog core

FromUwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date2015-09-08 12:40 +0200
SubjectRe: [PATCH v3 2/9] watchdog: Introduce hardware maximum timeout in watchdog core
Message-ID<q6otH-1Du-3@gated-at.bofh.it>
Hello,

On Sat, Aug 29, 2015 at 12:32:31PM -0700, Guenter Roeck wrote:
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index d8b0d3367706..3306249aa17d 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -53,9 +53,12 @@ struct watchdog_device {
>  	unsigned int timeout;
>  	unsigned int min_timeout;
>  	unsigned int max_timeout;
> +	unsigned int max_hw_timeout_ms;
>  	void *driver_data;
> -	struct mutex lock;
>  	unsigned long status;
> +	struct mutex lock;
> +	unsigned long last_keepalive;
> +	struct delayed_work work;
>  	struct list_head deferred;
>  };
>  
> @@ -73,18 +76,31 @@ It contains following fields:
>    additional information about the watchdog timer itself. (Like it's unique name)
>  * ops: a pointer to the list of watchdog operations that the watchdog supports.
>  * timeout: the watchdog timer's timeout value (in seconds).
> +  This is the time after which the system will reboot if user space does
> +  not send a heartbeat request if WDOG_ACTIVE is set.
>  * min_timeout: the watchdog timer's minimum timeout value (in seconds).
> -* max_timeout: the watchdog timer's maximum timeout value (in seconds).
> +  If set, the minimum configurable value for 'timeout'.
> +* max_timeout: the watchdog timer's maximum timeout value (in seconds),
> +  as seen from userspace. If set, the maximum configurable value for
> +  'timeout'. Not used if max_hw_timeout_ms is provided.

s/provided/non-zero/?

> +* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds.
> +  If set, the infrastructure will send heartbeats to the watchdog driver
> +  if 'timeout' is larger than 'max_hw_timeout / 2', unless WDOG_ACTIVE

This "/ 2" isn't true any more.

> +  is set and userspace failed to send a heartbeat for at least 'timeout'
> +  seconds.
>  * bootstatus: status of the device after booting (reported with watchdog
>    WDIOF_* status bits).
>  * driver_data: a pointer to the drivers private data of a watchdog device.
>    This data should only be accessed via the watchdog_set_drvdata and
>    watchdog_get_drvdata routines.
> -* lock: Mutex for WatchDog Timer Driver Core internal use only.
>  * status: this field contains a number of status bits that give extra
>    information about the status of the device (Like: is the watchdog timer
>    running/active, is the nowayout bit set, is the device opened via
>    the /dev/watchdog interface or not, ...).
> +* lock: Mutex for WatchDog Timer Driver Core internal use only.
> +* last_keepalive: Time of most recent keepalive triggered from user space,
> +  in jiffies.
> +* work: Worker data structure for WatchDog Timer Driver Core internal use only.
>  * deferred: entry in wtd_deferred_reg_list which is used to
>    register early initialized watchdogs.
>  
> [...]
> +static long watchdog_next_keepalive(struct watchdog_device *wdd)
> +{
> +	unsigned int hw_timeout_ms = wdd->timeout * 1000;
> +	unsigned long keepalive_interval;
> +	unsigned long last_heartbeat;
> +	unsigned long virt_timeout;
> +
> +	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(hw_timeout_ms);

Just looking at this line this is wrong. It just happens to be correct
here because hw_timeout_ms non-intuitively is set to wdd->timeout * 1000
which might not reflect what is programmed into the hardware.

I'd write:

	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(wdd->timeout * 1000);

...

> +	if (hw_timeout_ms > wdd->max_hw_timeout_ms)
> +		hw_timeout_ms = wdd->max_hw_timeout_ms;

	hw_timeout_ms = min(wdd->timeout * 1000, wdd->max_hw_timeout_ms);

> +
> +	keepalive_interval = msecs_to_jiffies(hw_timeout_ms / 2);
> +
> +	/*
> +	 * To ensure that the watchdog times out wdd->timeout seconds
> +	 * after the most recent ping from userspace, the last
> +	 * worker ping has to come in hw_timeout_ms before this timeout.
> +	 */
> +	last_heartbeat = virt_timeout - msecs_to_jiffies(hw_timeout_ms);
> +
> +	/*
> +	 * To ensure that the watchdog times out wdd->timeout seconds after
> +	 * the most recent ping from userspace, the last worker ping has to
> +	 * come hw_timeout_ms before this timeout.

duplicated comment.

> +	 */
> +	return min_t(long, last_heartbeat - jiffies, keepalive_interval);
> +}
> +
> [...]
> @@ -61,26 +143,27 @@ static struct watchdog_device *old_wdd;
>  
>  static int watchdog_ping(struct watchdog_device *wdd)
>  {
> -	int err = 0;
> +	int err;
>  
>  	mutex_lock(&wdd->lock);
> +	wdd->last_keepalive = jiffies;
> +	err = _watchdog_ping(wdd);
> +	watchdog_update_worker(wdd, false);

Here the cancel argument could also be true, right? That's because after
a ping (that doesn't modify the timeout) the result of
watchdog_need_worker doesn't change and so either the worker isn't
running + stopping it again doesn't hurt, or the timer is running and so
it's not tried to be stopped.

> +	mutex_unlock(&wdd->lock);
>  
> -	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> -		err = -ENODEV;
> -		goto out_ping;
> -	}
> +	return err;
> +}
>  
> -	if (!watchdog_active(wdd))
> -		goto out_ping;
> +static void watchdog_ping_work(struct work_struct *work)
> +{
> +	struct watchdog_device *wdd;
>  
> -	if (wdd->ops->ping)
> -		err = wdd->ops->ping(wdd);	/* ping the watchdog */
> -	else
> -		err = wdd->ops->start(wdd);	/* restart watchdog */
> +	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
>  
> -out_ping:
> +	mutex_lock(&wdd->lock);
> +	_watchdog_ping(wdd);
> +	watchdog_update_worker(wdd, false);

Here for the same reason you could pass true. So there is no caller that
needs to pass false which allows to simplify the function. (i.e. drop
the cancel parameter and simplify it assuming cancel is true)

>  	mutex_unlock(&wdd->lock);
> -	return err;
>  }
>  
>  /*
> [...]
> @@ -119,8 +134,9 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
>  /* Use the following function to check if a timeout value is invalid */
>  static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
>  {
> -	return ((wdd->max_timeout != 0) &&
> -		(t < wdd->min_timeout || t > wdd->max_timeout));

Is this (old) code correct? watchdog_timeout_invalid returns false if
wdd->max_timeout == 0 && t < wdd->min_timeout. I would have expected:

	return (wdd->max_timeout != 0 && t > wdd->max_timeout) ||
		t < wdd->min_timeout;

> +	return t > UINT_MAX / 1000 ||
> +		(!wdd->max_hw_timeout_ms && wdd->max_timeout &&
> +		 (t < wdd->min_timeout || t > wdd->max_timeout));

So should this better be:

	/* internal calculation is done in ms using unsigned variables */
	if (t > UINT_MAX / 1000)
		return 1;

	/*
	 * compat code for drivers not being aware of framework pings to
	 * bridge timeouts longer than supported by the hardware.
	 */
	if (!wdd->max_hw_timeout && wdd->max_timeout && t > wdd->max_timeout)
		return 1;

	if (t < wdd->min_timeout)
		return 1;

unless I'm missing something.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1220801

FromGuenter Roeck <linux@roeck-us.net>
Date2015-09-08 15:50 +0200
Message-ID<q6rrA-5QO-15@gated-at.bofh.it>
In reply to#1220686
Hi Uwe,

On 09/08/2015 03:33 AM, Uwe Kleine-König wrote:
> Hello,
>

>> [...]
>> +static long watchdog_next_keepalive(struct watchdog_device *wdd)
>> +{
>> +	unsigned int hw_timeout_ms = wdd->timeout * 1000;
>> +	unsigned long keepalive_interval;
>> +	unsigned long last_heartbeat;
>> +	unsigned long virt_timeout;
>> +
>> +	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(hw_timeout_ms);
>
> Just looking at this line this is wrong. It just happens to be correct
> here because hw_timeout_ms non-intuitively is set to wdd->timeout * 1000
> which might not reflect what is programmed into the hardware.
>
I don't see where the code is wrong. Sure, the variable name doesn't match
its initial use, but that doesn't make it wrong. I can pick a different variable
name if that helps (any suggested name ?).

> I'd write:
>
> 	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(wdd->timeout * 1000);
>
> ...
>
>> +	if (hw_timeout_ms > wdd->max_hw_timeout_ms)
>> +		hw_timeout_ms = wdd->max_hw_timeout_ms;
>
> 	hw_timeout_ms = min(wdd->timeout * 1000, wdd->max_hw_timeout_ms);
>

The reason for writing the code as is was to avoid the double 'wdd->timeout * 1000'
(and to avoid a line > 80 columns in the first line).

>> [...]
>> @@ -61,26 +143,27 @@ static struct watchdog_device *old_wdd;
>>
>>   static int watchdog_ping(struct watchdog_device *wdd)
>>   {
>> -	int err = 0;
>> +	int err;
>>
>>   	mutex_lock(&wdd->lock);
>> +	wdd->last_keepalive = jiffies;
>> +	err = _watchdog_ping(wdd);
>> +	watchdog_update_worker(wdd, false);
>
> Here the cancel argument could also be true, right? That's because after
> a ping (that doesn't modify the timeout) the result of
> watchdog_need_worker doesn't change and so either the worker isn't
> running + stopping it again doesn't hurt, or the timer is running and so
> it's not tried to be stopped.
>
Could, but it isn't necessary.

>> +	mutex_unlock(&wdd->lock);
>>
>> -	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>> -		err = -ENODEV;
>> -		goto out_ping;
>> -	}
>> +	return err;
>> +}
>>
>> -	if (!watchdog_active(wdd))
>> -		goto out_ping;
>> +static void watchdog_ping_work(struct work_struct *work)
>> +{
>> +	struct watchdog_device *wdd;
>>
>> -	if (wdd->ops->ping)
>> -		err = wdd->ops->ping(wdd);	/* ping the watchdog */
>> -	else
>> -		err = wdd->ops->start(wdd);	/* restart watchdog */
>> +	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
>>
>> -out_ping:
>> +	mutex_lock(&wdd->lock);
>> +	_watchdog_ping(wdd);
>> +	watchdog_update_worker(wdd, false);
>
> Here for the same reason you could pass true. So there is no caller that
> needs to pass false which allows to simplify the function. (i.e. drop
> the cancel parameter and simplify it assuming cancel is true)
>

There will be another call with 'false' added with a later patch, though
that could live with 'true'.

The function is executed by the worker, and since it is already executing
canceling it would not be necessary.

I don't know what happens if an attempt is made to cancel a worker from its
work function. I seem to recall that it causes a stall, but I may be wrong.
Any idea ?

>>   	mutex_unlock(&wdd->lock);
>> -	return err;
>>   }
>>
>>   /*
>> [...]
>> @@ -119,8 +134,9 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
>>   /* Use the following function to check if a timeout value is invalid */
>>   static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
>>   {
>> -	return ((wdd->max_timeout != 0) &&
>> -		(t < wdd->min_timeout || t > wdd->max_timeout));
>
> Is this (old) code correct? watchdog_timeout_invalid returns false if
> wdd->max_timeout == 0 && t < wdd->min_timeout. I would have expected:
>
> 	return (wdd->max_timeout != 0 && t > wdd->max_timeout) ||
> 		t < wdd->min_timeout;
>
You are correct. However, that is a different problem, which I addressed in
'watchdog: Always evaluate new timeout against min_timeout'.

>> +	return t > UINT_MAX / 1000 ||
>> +		(!wdd->max_hw_timeout_ms && wdd->max_timeout &&
>> +		 (t < wdd->min_timeout || t > wdd->max_timeout));
>
> So should this better be:
>
> 	/* internal calculation is done in ms using unsigned variables */
> 	if (t > UINT_MAX / 1000)
> 		return 1;
>
> 	/*
> 	 * compat code for drivers not being aware of framework pings to
> 	 * bridge timeouts longer than supported by the hardware.
> 	 */
> 	if (!wdd->max_hw_timeout && wdd->max_timeout && t > wdd->max_timeout)
> 		return 1;
>
> 	if (t < wdd->min_timeout)
> 		return 1;
>

After all patches are applied, my code is

/* Use the following function to check if a timeout value is invalid */
static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
{
         return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
                 (!wdd->max_hw_timeout_ms && wdd->max_timeout &&
                  t > wdd->max_timeout);
}

which is exactly the same (without the comments).

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1221061

FromUwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date2015-09-08 22:10 +0200
Message-ID<q6xnk-6ba-15@gated-at.bofh.it>
In reply to#1220801
Hello Guenter,

On Tue, Sep 08, 2015 at 06:47:26AM -0700, Guenter Roeck wrote:
> On 09/08/2015 03:33 AM, Uwe Kleine-König wrote:
> >Hello,
> >
> 
> >>[...]
> >>+static long watchdog_next_keepalive(struct watchdog_device *wdd)
> >>+{
> >>+	unsigned int hw_timeout_ms = wdd->timeout * 1000;
> >>+	unsigned long keepalive_interval;
> >>+	unsigned long last_heartbeat;
> >>+	unsigned long virt_timeout;
> >>+
> >>+	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(hw_timeout_ms);
> >
> >Just looking at this line this is wrong. It just happens to be correct
> >here because hw_timeout_ms non-intuitively is set to wdd->timeout * 1000
> >which might not reflect what is programmed into the hardware.
> >
> I don't see where the code is wrong. Sure, the variable name doesn't match
> its initial use, but that doesn't make it wrong. I can pick a different variable
> name if that helps (any suggested name ?).
> 
> >I'd write:
> >
> >	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(wdd->timeout * 1000);
> >
> >...
> >
> >>+	if (hw_timeout_ms > wdd->max_hw_timeout_ms)
> >>+		hw_timeout_ms = wdd->max_hw_timeout_ms;
> >
> >	hw_timeout_ms = min(wdd->timeout * 1000, wdd->max_hw_timeout_ms);
> >
> 
> The reason for writing the code as is was to avoid the double 'wdd->timeout * 1000'

The compile should be able to cope with that and only do the
multiplication once.

> (and to avoid a line > 80 columns in the first line).

unsigned timeout_ms = wdd->timeout * 1000; ?

> 
> >>[...]
> >>@@ -61,26 +143,27 @@ static struct watchdog_device *old_wdd;
> >>
> >>  static int watchdog_ping(struct watchdog_device *wdd)
> >>  {
> >>-	int err = 0;
> >>+	int err;
> >>
> >>  	mutex_lock(&wdd->lock);
> >>+	wdd->last_keepalive = jiffies;
> >>+	err = _watchdog_ping(wdd);
> >>+	watchdog_update_worker(wdd, false);
> >
> >Here the cancel argument could also be true, right? That's because after
> >a ping (that doesn't modify the timeout) the result of
> >watchdog_need_worker doesn't change and so either the worker isn't
> >running + stopping it again doesn't hurt, or the timer is running and so
> >it's not tried to be stopped.
> >
> Could, but it isn't necessary.
> 
> >>+	mutex_unlock(&wdd->lock);
> >>
> >>-	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> >>-		err = -ENODEV;
> >>-		goto out_ping;
> >>-	}
> >>+	return err;
> >>+}
> >>
> >>-	if (!watchdog_active(wdd))
> >>-		goto out_ping;
> >>+static void watchdog_ping_work(struct work_struct *work)
> >>+{
> >>+	struct watchdog_device *wdd;
> >>
> >>-	if (wdd->ops->ping)
> >>-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
> >>-	else
> >>-		err = wdd->ops->start(wdd);	/* restart watchdog */
> >>+	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
> >>
> >>-out_ping:
> >>+	mutex_lock(&wdd->lock);
> >>+	_watchdog_ping(wdd);
> >>+	watchdog_update_worker(wdd, false);
> >
> >Here for the same reason you could pass true. So there is no caller that
> >needs to pass false which allows to simplify the function. (i.e. drop
> >the cancel parameter and simplify it assuming cancel is true)
> >
> 
> There will be another call with 'false' added with a later patch, though
> that could live with 'true'.
> 
> The function is executed by the worker, and since it is already executing
> canceling it would not be necessary.
> 
> I don't know what happens if an attempt is made to cancel a worker from its
> work function. I seem to recall that it causes a stall, but I may be wrong.
> Any idea ?

No, I don't know if that works or not. But I would not expect any
problems.

> >>  	mutex_unlock(&wdd->lock);
> >>-	return err;
> >>  }
> >>
> >>  /*
> >>[...]
> >>@@ -119,8 +134,9 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
> >>  /* Use the following function to check if a timeout value is invalid */
> >>  static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> >>  {
> >>-	return ((wdd->max_timeout != 0) &&
> >>-		(t < wdd->min_timeout || t > wdd->max_timeout));
> >
> >Is this (old) code correct? watchdog_timeout_invalid returns false if
> >wdd->max_timeout == 0 && t < wdd->min_timeout. I would have expected:
> >
> >	return (wdd->max_timeout != 0 && t > wdd->max_timeout) ||
> >		t < wdd->min_timeout;
> >
> You are correct. However, that is a different problem, which I addressed in
> 'watchdog: Always evaluate new timeout against min_timeout'.

I usually consider it nice to have the fixes first in the series. I
didn't look into the later patches yet. This should be fixed for 4.3.

> >>+	return t > UINT_MAX / 1000 ||
> >>+		(!wdd->max_hw_timeout_ms && wdd->max_timeout &&
> >>+		 (t < wdd->min_timeout || t > wdd->max_timeout));
> >
> >So should this better be:
> >
> >	/* internal calculation is done in ms using unsigned variables */
> >	if (t > UINT_MAX / 1000)
> >		return 1;
> >
> >	/*
> >	 * compat code for drivers not being aware of framework pings to
> >	 * bridge timeouts longer than supported by the hardware.
> >	 */
> >	if (!wdd->max_hw_timeout && wdd->max_timeout && t > wdd->max_timeout)
> >		return 1;
> >
> >	if (t < wdd->min_timeout)
> >		return 1;
> >
> 
> After all patches are applied, my code is
> 
> /* Use the following function to check if a timeout value is invalid */
> static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> {
>         return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
>                 (!wdd->max_hw_timeout_ms && wdd->max_timeout &&
>                  t > wdd->max_timeout);
> }
> 
> which is exactly the same (without the comments).

The comments make it a tad nicer though :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1221087

FromGuenter Roeck <linux@roeck-us.net>
Date2015-09-08 23:10 +0200
Message-ID<q6yjp-7xa-25@gated-at.bofh.it>
In reply to#1221061
On Tue, Sep 08, 2015 at 10:03:32PM +0200, Uwe Kleine-König wrote:
> Hello Guenter,
> 
> On Tue, Sep 08, 2015 at 06:47:26AM -0700, Guenter Roeck wrote:
> > On 09/08/2015 03:33 AM, Uwe Kleine-König wrote:
> > >Hello,
> > >
> > 
> > >>[...]
> > >>+static long watchdog_next_keepalive(struct watchdog_device *wdd)
> > >>+{
> > >>+	unsigned int hw_timeout_ms = wdd->timeout * 1000;
> > >>+	unsigned long keepalive_interval;
> > >>+	unsigned long last_heartbeat;
> > >>+	unsigned long virt_timeout;
> > >>+
> > >>+	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(hw_timeout_ms);
> > >
> > >Just looking at this line this is wrong. It just happens to be correct
> > >here because hw_timeout_ms non-intuitively is set to wdd->timeout * 1000
> > >which might not reflect what is programmed into the hardware.
> > >
> > I don't see where the code is wrong. Sure, the variable name doesn't match
> > its initial use, but that doesn't make it wrong. I can pick a different variable
> > name if that helps (any suggested name ?).
> > 
> > >I'd write:
> > >
> > >	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(wdd->timeout * 1000);
> > >
> > >...
> > >
> > >>+	if (hw_timeout_ms > wdd->max_hw_timeout_ms)
> > >>+		hw_timeout_ms = wdd->max_hw_timeout_ms;
> > >
> > >	hw_timeout_ms = min(wdd->timeout * 1000, wdd->max_hw_timeout_ms);
> > >
> > 
> > The reason for writing the code as is was to avoid the double 'wdd->timeout * 1000'
> 
> The compile should be able to cope with that and only do the
> multiplication once.
> 
You sure ? msecs_to_jiffies() can be an external function.
I always thought that the compiler must not make such context
assumptions across function calls.

> > (and to avoid a line > 80 columns in the first line).
> 
> unsigned timeout_ms = wdd->timeout * 1000; ?
> 
Fine with me.

> > 
> > >>[...]
> > >>@@ -61,26 +143,27 @@ static struct watchdog_device *old_wdd;
> > >>
> > >>  static int watchdog_ping(struct watchdog_device *wdd)
> > >>  {
> > >>-	int err = 0;
> > >>+	int err;
> > >>
> > >>  	mutex_lock(&wdd->lock);
> > >>+	wdd->last_keepalive = jiffies;
> > >>+	err = _watchdog_ping(wdd);
> > >>+	watchdog_update_worker(wdd, false);
> > >
> > >Here the cancel argument could also be true, right? That's because after
> > >a ping (that doesn't modify the timeout) the result of
> > >watchdog_need_worker doesn't change and so either the worker isn't
> > >running + stopping it again doesn't hurt, or the timer is running and so
> > >it's not tried to be stopped.
> > >
> > Could, but it isn't necessary.
> > 
> > >>+	mutex_unlock(&wdd->lock);
> > >>
> > >>-	if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > >>-		err = -ENODEV;
> > >>-		goto out_ping;
> > >>-	}
> > >>+	return err;
> > >>+}
> > >>
> > >>-	if (!watchdog_active(wdd))
> > >>-		goto out_ping;
> > >>+static void watchdog_ping_work(struct work_struct *work)
> > >>+{
> > >>+	struct watchdog_device *wdd;
> > >>
> > >>-	if (wdd->ops->ping)
> > >>-		err = wdd->ops->ping(wdd);	/* ping the watchdog */
> > >>-	else
> > >>-		err = wdd->ops->start(wdd);	/* restart watchdog */
> > >>+	wdd = container_of(to_delayed_work(work), struct watchdog_device, work);
> > >>
> > >>-out_ping:
> > >>+	mutex_lock(&wdd->lock);
> > >>+	_watchdog_ping(wdd);
> > >>+	watchdog_update_worker(wdd, false);
> > >
> > >Here for the same reason you could pass true. So there is no caller that
> > >needs to pass false which allows to simplify the function. (i.e. drop
> > >the cancel parameter and simplify it assuming cancel is true)
> > >
> > 
> > There will be another call with 'false' added with a later patch, though
> > that could live with 'true'.
> > 
> > The function is executed by the worker, and since it is already executing
> > canceling it would not be necessary.
> > 
> > I don't know what happens if an attempt is made to cancel a worker from its
> > work function. I seem to recall that it causes a stall, but I may be wrong.
> > Any idea ?
> 
> No, I don't know if that works or not. But I would not expect any
> problems.
> 
I'll give it a try.

> > >>  	mutex_unlock(&wdd->lock);
> > >>-	return err;
> > >>  }
> > >>
> > >>  /*
> > >>[...]
> > >>@@ -119,8 +134,9 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
> > >>  /* Use the following function to check if a timeout value is invalid */
> > >>  static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> > >>  {
> > >>-	return ((wdd->max_timeout != 0) &&
> > >>-		(t < wdd->min_timeout || t > wdd->max_timeout));
> > >
> > >Is this (old) code correct? watchdog_timeout_invalid returns false if
> > >wdd->max_timeout == 0 && t < wdd->min_timeout. I would have expected:
> > >
> > >	return (wdd->max_timeout != 0 && t > wdd->max_timeout) ||
> > >		t < wdd->min_timeout;
> > >
> > You are correct. However, that is a different problem, which I addressed in
> > 'watchdog: Always evaluate new timeout against min_timeout'.
> 
> I usually consider it nice to have the fixes first in the series. I
> didn't look into the later patches yet. This should be fixed for 4.3.
> 
Not sure if it is a fix. It does change semantics, after all.
No problems reordering the sequence, though.

> > >>+	return t > UINT_MAX / 1000 ||
> > >>+		(!wdd->max_hw_timeout_ms && wdd->max_timeout &&
> > >>+		 (t < wdd->min_timeout || t > wdd->max_timeout));
> > >
> > >So should this better be:
> > >
> > >	/* internal calculation is done in ms using unsigned variables */
> > >	if (t > UINT_MAX / 1000)
> > >		return 1;
> > >
> > >	/*
> > >	 * compat code for drivers not being aware of framework pings to
> > >	 * bridge timeouts longer than supported by the hardware.
> > >	 */
> > >	if (!wdd->max_hw_timeout && wdd->max_timeout && t > wdd->max_timeout)
> > >		return 1;
> > >
> > >	if (t < wdd->min_timeout)
> > >		return 1;
> > >
> > 
> > After all patches are applied, my code is
> > 
> > /* Use the following function to check if a timeout value is invalid */
> > static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> > {
> >         return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
> >                 (!wdd->max_hw_timeout_ms && wdd->max_timeout &&
> >                  t > wdd->max_timeout);
> > }
> > 
> > which is exactly the same (without the comments).
> 
> The comments make it a tad nicer though :-)
> 
POV :-) I prefer to have a single expression. How about adding
the comments on top of it ? Would that be ok with you ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1221292

FromUwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date2015-09-09 10:00 +0200
Message-ID<q6Isq-5dz-17@gated-at.bofh.it>
In reply to#1221087
Hello Guenter,

On Tue, Sep 08, 2015 at 02:07:30PM -0700, Guenter Roeck wrote:
> On Tue, Sep 08, 2015 at 10:03:32PM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 08, 2015 at 06:47:26AM -0700, Guenter Roeck wrote:
> > > On 09/08/2015 03:33 AM, Uwe Kleine-König wrote:
> > > >>+	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(hw_timeout_ms);
> > > >
> > > >Just looking at this line this is wrong. It just happens to be correct
> > > >here because hw_timeout_ms non-intuitively is set to wdd->timeout * 1000
> > > >which might not reflect what is programmed into the hardware.
> > > >
> > > I don't see where the code is wrong. Sure, the variable name doesn't match
> > > its initial use, but that doesn't make it wrong. I can pick a different variable
> > > name if that helps (any suggested name ?).

As there are two different semantics for the variable there is no good
name.

> > > 
> > > >I'd write:
> > > >
> > > >	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(wdd->timeout * 1000);
> > > >
> > > >...
> > > >
> > > >>+	if (hw_timeout_ms > wdd->max_hw_timeout_ms)
> > > >>+		hw_timeout_ms = wdd->max_hw_timeout_ms;
> > > >
> > > >	hw_timeout_ms = min(wdd->timeout * 1000, wdd->max_hw_timeout_ms);
> > > >
> > > 
> > > The reason for writing the code as is was to avoid the double 'wdd->timeout * 1000'
> > 
> > The compile should be able to cope with that and only do the
> > multiplication once.
> > 
> You sure ? msecs_to_jiffies() can be an external function.
> I always thought that the compiler must not make such context
> assumptions across function calls.

The function with the helper variable timeout_ms looks as follows:

	unsigned timeout_ms = wdd->timeout * 1000;
	unsigned long virt_timeout;

	virt_timeout = wdd->last_keepalive + msecs_to_jiffies(timeout_ms);

	hw_timeout_ms = min(timeout_ms, wdd->max_hw_timeout_ms);

	...

so there is nothing done twice even in the source code. And otherwise
msecs_to_jiffies is inlined (alternatively it should be marked const).

> > > >>  	mutex_unlock(&wdd->lock);
> > > >>-	return err;
> > > >>  }
> > > >>
> > > >>  /*
> > > >>[...]
> > > >>@@ -119,8 +134,9 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway
> > > >>  /* Use the following function to check if a timeout value is invalid */
> > > >>  static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> > > >>  {
> > > >>-	return ((wdd->max_timeout != 0) &&
> > > >>-		(t < wdd->min_timeout || t > wdd->max_timeout));
> > > >
> > > >Is this (old) code correct? watchdog_timeout_invalid returns false if
> > > >wdd->max_timeout == 0 && t < wdd->min_timeout. I would have expected:
> > > >
> > > >	return (wdd->max_timeout != 0 && t > wdd->max_timeout) ||
> > > >		t < wdd->min_timeout;
> > > >
> > > You are correct. However, that is a different problem, which I addressed in
> > > 'watchdog: Always evaluate new timeout against min_timeout'.
> > 
> > I usually consider it nice to have the fixes first in the series. I
> > didn't look into the later patches yet. This should be fixed for 4.3.
> > 
> Not sure if it is a fix. It does change semantics, after all.
Right, fixing bugs usually introduces changes :-)

> > > >>+	return t > UINT_MAX / 1000 ||
> > > >>+		(!wdd->max_hw_timeout_ms && wdd->max_timeout &&
> > > >>+		 (t < wdd->min_timeout || t > wdd->max_timeout));
> > > >
> > > >So should this better be:
> > > >
> > > >	/* internal calculation is done in ms using unsigned variables */
> > > >	if (t > UINT_MAX / 1000)
> > > >		return 1;
> > > >
> > > >	/*
> > > >	 * compat code for drivers not being aware of framework pings to
> > > >	 * bridge timeouts longer than supported by the hardware.
> > > >	 */
> > > >	if (!wdd->max_hw_timeout && wdd->max_timeout && t > wdd->max_timeout)
> > > >		return 1;
> > > >
> > > >	if (t < wdd->min_timeout)
> > > >		return 1;
> > > >
> > > 
> > > After all patches are applied, my code is
> > > 
> > > /* Use the following function to check if a timeout value is invalid */
> > > static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> > > {
> > >         return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
> > >                 (!wdd->max_hw_timeout_ms && wdd->max_timeout &&
> > >                  t > wdd->max_timeout);
> > > }
> > > 
> > > which is exactly the same (without the comments).
> > 
> > The comments make it a tad nicer though :-)
> > 
> POV :-) I prefer to have a single expression. How about adding
> the comments on top of it ? Would that be ok with you ?

With separate expressions for each case the comments are attached to the
right line respectively which makes it more readable. For you while
writing the code it doesn't make much difference, but for someone who
should understand the code later it's easier to have several easy
expressions than a single more complicated one.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web