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

Groups > linux.kernel > #1345272 > unrolled thread

Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

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] watchdog: Add watchdog timer support for the WinSystems EBC-C384 Wim Van Sebroeck <wim@iguana.be> - 2016-02-28 15:10 +0100
    Re: [PATCH v3] watchdog: Add watchdog timer support for the  WinSystems EBC-C384 William Breathitt Gray <vilhelm.gray@gmail.com> - 2016-02-28 15:40 +0100
    Re: [PATCH v3] watchdog: Add watchdog timer support for the  WinSystems EBC-C384 Guenter Roeck <linux@roeck-us.net> - 2016-02-28 16:10 +0100
      Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384 Wim Van Sebroeck <wim@iguana.be> - 2016-02-28 17:30 +0100

#1345272 — Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

> > +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
> > +{
> > +	/* resolution is in minutes for timeouts greater than 255 seconds */
> > +	if (t > 255) {
> > +		/* truncate second resolution to minute resolution */
> > +		t /= 60;
> > +		wdev->timeout = t * 60;
> > +
> > +		/* set watchdog timer for minutes */
> > +		outb(0x00, CFG_ADDR);
> 
> If ask for 299 seconds surely I should get 300 not 240 ?
> (Whether to round off or round up is an interesting question for the
> middle range - does it go off early or late - I'd have said late but...)

This is my preference:
	if (t > 255)
		t = (((t - 1) / 60) + 1) * 60;

Which basically is a round-up to minutes
t = 256 -> gives 300
t = 299 -> gives 300 
t = 300 -> gives 300
t = 301 -> gives 360
t = 15299 -> gives 15300
t = 15300 -> gives 15300
t = 15301 -> gives 15360

So I am also for going late.

Kind regards,
Wim.

[toc] | [next] | [standalone]

#1345288 — Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

On Sun, Feb 28, 2016 at 03:07:39PM +0100, Wim Van Sebroeck wrote:
>> > +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
>> > +{
>> > +	/* resolution is in minutes for timeouts greater than 255 seconds */
>> > +	if (t > 255) {
>> > +		/* truncate second resolution to minute resolution */
>> > +		t /= 60;
>> > +		wdev->timeout = t * 60;
>> > +
>> > +		/* set watchdog timer for minutes */
>> > +		outb(0x00, CFG_ADDR);
>> 
>> If ask for 299 seconds surely I should get 300 not 240 ?
>> (Whether to round off or round up is an interesting question for the
>> middle range - does it go off early or late - I'd have said late but...)
>
>This is my preference:
>	if (t > 255)
>		t = (((t - 1) / 60) + 1) * 60;
>
>Which basically is a round-up to minutes
>t = 256 -> gives 300
>t = 299 -> gives 300 
>t = 300 -> gives 300
>t = 301 -> gives 360
>t = 15299 -> gives 15300
>t = 15300 -> gives 15300
>t = 15301 -> gives 15360
>
>So I am also for going late.
>
>Kind regards,
>Wim.

I decided to give this another consideration: perhaps we should allow it
to go late rather than early. I figure that when a user configures the
watchdog timer for a certain value in seconds they will have an
expectation that the watchdog timer will wait at least that amount of
time (with the understanding that it may be somewhat late due to the
granularity of the timer).

This behavior follows a lot of other timer standards (e.g. nanosleep)
so it's what the user expects and what we should follow for now. I'll
submit a version 5 patch with this change.

William Breathitt Gray

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

#1345302 — Re: [PATCH v3] watchdog: Add watchdog timer support for the WinSystems EBC-C384

On 02/28/2016 06:07 AM, Wim Van Sebroeck wrote:
>>> +static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
>>> +{
>>> +	/* resolution is in minutes for timeouts greater than 255 seconds */
>>> +	if (t > 255) {
>>> +		/* truncate second resolution to minute resolution */
>>> +		t /= 60;
>>> +		wdev->timeout = t * 60;
>>> +
>>> +		/* set watchdog timer for minutes */
>>> +		outb(0x00, CFG_ADDR);
>>
>> If ask for 299 seconds surely I should get 300 not 240 ?
>> (Whether to round off or round up is an interesting question for the
>> middle range - does it go off early or late - I'd have said late but...)
>
> This is my preference:
> 	if (t > 255)
> 		t = (((t - 1) / 60) + 1) * 60;
>

In case I am missing something: Why not just use DIV_ROUND_UP() ?

Thanks,
Guenter

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

#1345346

Hi Guenter,

> On 02/28/2016 06:07 AM, Wim Van Sebroeck wrote:
> >>>+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, 
> >>>unsigned t)
> >>>+{
> >>>+	/* resolution is in minutes for timeouts greater than 255 seconds */
> >>>+	if (t > 255) {
> >>>+		/* truncate second resolution to minute resolution */
> >>>+		t /= 60;
> >>>+		wdev->timeout = t * 60;
> >>>+
> >>>+		/* set watchdog timer for minutes */
> >>>+		outb(0x00, CFG_ADDR);
> >>
> >>If ask for 299 seconds surely I should get 300 not 240 ?
> >>(Whether to round off or round up is an interesting question for the
> >>middle range - does it go off early or late - I'd have said late but...)
> >
> >This is my preference:
> >	if (t > 255)
> >		t = (((t - 1) / 60) + 1) * 60;
> >
> 
> In case I am missing something: Why not just use DIV_ROUND_UP() ?

That's indeed also usable (from kernel.h: #define DIV_ROUND_UP(n,d) (((n) + (d) -1) / (d)) which is the same) and indeed preferable.

Kind regards,
Wim.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web