| Message ID | 1432197156-16947-6-git-send-email-fu.wei@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Guenter, Thanks for review. :-) feedback inline below On 21 May 2015 at 17:04, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote: >> >> From: Fu Wei <fu.wei@linaro.org> >> >> Also update Documentation/watchdog/watchdog-kernel-api.txt to >> introduce: >> (1)the new elements in the watchdog_device and watchdog_ops struct; >> (2)the new API "watchdog_init_timeouts". >> >> Reasons: >> (1)kernel already has two watchdog drivers are using "pretimeout": >> drivers/char/ipmi/ipmi_watchdog.c >> drivers/watchdog/kempld_wdt.c(but the definition is different) >> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- > > > [ ... ] > >> +extern int watchdog_init_timeouts(struct watchdog_device *wdd, >> + unsigned int pretimeout_parm, >> + unsigned int timeout_parm, >> + void (*update_limits)(struct >> watchdog_device *), >> + struct device *dev); >> >> -The watchdog_init_timeout function allows you to initialize the timeout >> field >> -using the module timeout parameter or by retrieving the timeout-sec >> property from >> -the device tree (if the module timeout parameter is invalid). Best >> practice is >> -to set the default timeout value as timeout value in the watchdog_device >> and >> -then use this function to set the user "preferred" timeout value. >> +The watchdog_init_timeouts function allows you to initialize the >> pretimeout and >> +timeout fields using the module pretimeout and timeout parameter or by >> +retrieving the elements in the timeout-sec property(the first element is >> for >> +timeout, the second one is for pretimeout) from the device tree(if the >> module >> +pretimeout and timeout parameter are invalid). >> +Normally, the pretimeout value will affect the limitation of timeout, and >> it >> +is also hardware related. So you can write a function in your driver to >> update >> +the limitation of timeout, according to the pretimeout value. Then pass >> the >> +function pointer by the update_limits parameter. If you driver doesn't >> +need this adjustment, just pass NULL to the update_limits parameter. > > > You've lost me a bit with the update_limits function. > watchdog_init_timeouts() > is called from the driver. yes, that is the help function which will be called from watchdog driver, like SBSA watchdog driver > Why should the function have to call back into > the > driver to update the parameters which are passed from the driver ? Let me explain this, please correct me if I misunderstand something. According to the concept of "pretimeout" in kernel, the timeout contains the pretimeout, like * Kernel/API: P---------| pretimeout * |-------------------------------T timeout If you set up the value of pretimeout, that means pretimeout <min_timeout < timeout < max_timeout < (pretimeout + max_timeout_for_1th_stage) For min_timeout > pretimeout. if some one setup a timeout like : pretimeout > timeout > min_timeout, I think that could be a problem For max_timeout < (pretimeout + max_timeout_for_1th_stage), if some one setup a timeout like (pretimeout + max_timeout_for_1th_stage) < timeout > max_timeout . I have explained a little in doc, but the adjustment may have something to do with hardware, like max_timeout_for_1th_stage(in SBSA watchdog , limited by WCV) maybe this problem wouldn't happen ,if you set up max_timeout to a small number. so you can pass NULL to the pointer. but I think maybe for other device , that may happen. > Seems to me the driver can do that calculation first, then call > watchdog_init_timeouts() with the result. Am I missing something ? maybe I am overthinking it :-) please correct me > > Thanks, > Guenter >
Hi Guenter, Great thanks, I have got your review feedback, I will fix the problems :-) For update_limits, I also don't want to have that in the watchdog driver, and you can't find it in my last version. And this version, I integrate the watchdog_init_timeout and watchdog_init_pretimeout. so I can not add a driver specific "update_limits" between them. I think maybe we can not make this "update_limits" after calling init_timeouts, because we need to test and verify the timeout setting right after init_pretimeout by max_timeout and min_timeout. that is why I call update_limits right after init_pretimeout and before init_timeout. any suggestion ? Great thanks ! :-) On 21 May 2015 at 18:17, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/21/2015 03:05 AM, Fu Wei wrote: >> >> Hi Guenter, >> >> Thanks for review. :-) >> feedback inline below >> >> On 21 May 2015 at 17:04, Guenter Roeck <linux@roeck-us.net> wrote: >>> >>> On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote: >>>> >>>> >>>> From: Fu Wei <fu.wei@linaro.org> >>>> >>>> Also update Documentation/watchdog/watchdog-kernel-api.txt to >>>> introduce: >>>> (1)the new elements in the watchdog_device and watchdog_ops struct; >>>> (2)the new API "watchdog_init_timeouts". >>>> >>>> Reasons: >>>> (1)kernel already has two watchdog drivers are using "pretimeout": >>>> drivers/char/ipmi/ipmi_watchdog.c >>>> drivers/watchdog/kempld_wdt.c(but the definition is different) >>>> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog >>>> >>>> Signed-off-by: Fu Wei <fu.wei@linaro.org> >>>> --- >>> >>> >>> >>> [ ... ] >>> >>>> +extern int watchdog_init_timeouts(struct watchdog_device *wdd, >>>> + unsigned int pretimeout_parm, >>>> + unsigned int timeout_parm, >>>> + void (*update_limits)(struct >>>> watchdog_device *), >>>> + struct device *dev); >>>> >>>> -The watchdog_init_timeout function allows you to initialize the timeout >>>> field >>>> -using the module timeout parameter or by retrieving the timeout-sec >>>> property from >>>> -the device tree (if the module timeout parameter is invalid). Best >>>> practice is >>>> -to set the default timeout value as timeout value in the >>>> watchdog_device >>>> and >>>> -then use this function to set the user "preferred" timeout value. >>>> +The watchdog_init_timeouts function allows you to initialize the >>>> pretimeout and >>>> +timeout fields using the module pretimeout and timeout parameter or by >>>> +retrieving the elements in the timeout-sec property(the first element >>>> is >>>> for >>>> +timeout, the second one is for pretimeout) from the device tree(if the >>>> module >>>> +pretimeout and timeout parameter are invalid). >>>> +Normally, the pretimeout value will affect the limitation of timeout, >>>> and >>>> it >>>> +is also hardware related. So you can write a function in your driver to >>>> update >>>> +the limitation of timeout, according to the pretimeout value. Then pass >>>> the >>>> +function pointer by the update_limits parameter. If you driver doesn't >>>> +need this adjustment, just pass NULL to the update_limits parameter. >>> >>> >>> >>> You've lost me a bit with the update_limits function. >>> watchdog_init_timeouts() >>> is called from the driver. >> >> >> yes, that is the help function which will be called from watchdog >> driver, like SBSA watchdog driver >> >>> Why should the function have to call back into >>> the >>> driver to update the parameters which are passed from the driver ? >> >> >> Let me explain this, please correct me if I misunderstand something. >> According to the concept of "pretimeout" in kernel, the timeout >> contains the pretimeout, like >> >> * Kernel/API: P---------| pretimeout >> * |-------------------------------T timeout >> >> If you set up the value of pretimeout, that means pretimeout >> <min_timeout < timeout < max_timeout < (pretimeout + >> max_timeout_for_1th_stage) >> For min_timeout > pretimeout. if some one setup a timeout like : >> pretimeout > timeout > min_timeout, I think that could be a problem >> For max_timeout < (pretimeout + max_timeout_for_1th_stage), if some >> one setup a timeout like (pretimeout + max_timeout_for_1th_stage) < >> timeout > max_timeout . >> >> I have explained a little in doc, but the adjustment may have >> something to do with hardware, like max_timeout_for_1th_stage(in SBSA >> watchdog , limited by WCV) >> >> maybe this problem wouldn't happen ,if you set up max_timeout to a >> small number. so you can pass NULL to the pointer. >> but I think maybe for other device , that may happen. >> >>> Seems to me the driver can do that calculation first, then call >>> watchdog_init_timeouts() with the result. Am I missing something ? >> >> >> maybe I am overthinking it :-) >> please correct me >> > > I just sent a more complete review. In general I think this problem > (where the driver needs to update timeout limits based on the value of > pretimeout) is very driver specific, and should be kept in the driver. > I would prefer to keep it out of the API if possible. > > Unless I am missing something, it should be possible to call the > update_limits function in the driver after calling init_timeouts. > > Thanks, > Guenter >
Hi Guenter, On 21 May 2015 at 23:32, Guenter Roeck <linux@roeck-us.net> wrote: > On Thu, May 21, 2015 at 04:32:34PM +0800, fu.wei@linaro.org wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> Also update Documentation/watchdog/watchdog-kernel-api.txt to >> introduce: >> (1)the new elements in the watchdog_device and watchdog_ops struct; >> (2)the new API "watchdog_init_timeouts". >> >> Reasons: >> (1)kernel already has two watchdog drivers are using "pretimeout": >> drivers/char/ipmi/ipmi_watchdog.c >> drivers/watchdog/kempld_wdt.c(but the definition is different) >> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- > > [ ... ] > >> >> +/* 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->max_pretimeout != 0) && >> + (t < wdd->min_pretimeout || t > wdd->max_pretimeout)); >> +} > > Should this function also enforce "t < wdd->timeout", and > should watchdog_timeout_invalid() enforce "t > wdd->pretimeout" ? yes, you are right , thanks for the correction !! :-) > > Thanks, > Guenter
Hi Timo, On 22 May 2015 at 14:30, Timo Kokkonen <timo.kokkonen@offcode.fi> wrote: > On 21.05.2015 11:32, fu.wei@linaro.org wrote: >> >> From: Fu Wei <fu.wei@linaro.org> >> >> Also update Documentation/watchdog/watchdog-kernel-api.txt to >> introduce: >> (1)the new elements in the watchdog_device and watchdog_ops struct; >> (2)the new API "watchdog_init_timeouts". >> >> Reasons: >> (1)kernel already has two watchdog drivers are using "pretimeout": >> drivers/char/ipmi/ipmi_watchdog.c >> drivers/watchdog/kempld_wdt.c(but the definition is different) >> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog >> > > Hi, > > As I was proposing some other API changes with my early-timeout-sec work, I > can see my work is going to collide with your API change proposal a bit. So > maybe I should ask your opinion as well.. Thank you for reminding me of that, I saw "early-timeout-sec", but because I don't get it in kernel API or watchdog_core.c, so I did not pay attention to it. Sorry. > > Could this pretimeout feature be something that other drivers could benefit > too? yes , as you may know, I am making a patch for pretimeout support in watchdog framework > I can see that it does not do anything else except call panic() before > letting the watchdog expire. This is something that could be emulated easily > by the watchdog core for drivers that don't know anything about pretimeouts > at all. For SBSA watchdog, there are two stage timeout, and according to kernel doc: ---------------------- Pretimeouts: Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets. ---------------------- I think panic() is the right way to do now. but people may also need to config : PANIC_TIMEOUT [!=0] KEXEC KDUMP for debug reason > > The way I was planning the API change there would need to be a small change > with each watchdog driver in order to let the watchdog core take over > generic behaviour on behalf of the driver. My goal was to make the change so > that each driver that gets converted to the new API extensions gets a > support for early-timeout-sec for free, without needing to enable support > for it any way. If the API was designed properly, also pretimeouts could be > handled easily and maybe even so that other drivers could have that feature > even though their hardware does not explicitly give any support for it. could you please point out your patch , then I can learn your idea :-) For now , I am working on a common "Pretimeouts" following the concept in kernel doc. > > Any thoughts? my thoughts is in my pretimeout patch , would you provide some suggestion ? Great thanks ! > > Thanks, > -Timo
Hi Timo, On 22 May 2015 at 16:59, Timo Kokkonen <timo.kokkonen@offcode.fi> wrote: > On 22.05.2015 11:23, Fu Wei wrote: >> >> Hi Timo, >> >> >> On 22 May 2015 at 14:30, Timo Kokkonen <timo.kokkonen@offcode.fi> wrote: >>> >>> On 21.05.2015 11:32, fu.wei@linaro.org wrote: >>>> >>>> >>>> From: Fu Wei <fu.wei@linaro.org> >>>> >>>> Also update Documentation/watchdog/watchdog-kernel-api.txt to >>>> introduce: >>>> (1)the new elements in the watchdog_device and watchdog_ops struct; >>>> (2)the new API "watchdog_init_timeouts". >>>> >>>> Reasons: >>>> (1)kernel already has two watchdog drivers are using "pretimeout": >>>> drivers/char/ipmi/ipmi_watchdog.c >>>> drivers/watchdog/kempld_wdt.c(but the definition is different) >>>> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog >>>> >>> >>> Hi, >>> >>> As I was proposing some other API changes with my early-timeout-sec work, >>> I >>> can see my work is going to collide with your API change proposal a bit. >>> So >>> maybe I should ask your opinion as well.. >> >> >> Thank you for reminding me of that, I saw "early-timeout-sec", but >> because I don't get it in kernel API or watchdog_core.c, so I did not >> pay attention to it. >> Sorry. >> >>> >>> Could this pretimeout feature be something that other drivers could >>> benefit >>> too? >> >> >> yes , as you may know, I am making a patch for pretimeout support in >> watchdog framework >> >>> I can see that it does not do anything else except call panic() before >>> letting the watchdog expire. This is something that could be emulated >>> easily >>> by the watchdog core for drivers that don't know anything about >>> pretimeouts >>> at all. >> >> >> For SBSA watchdog, there are two stage timeout, and according to kernel >> doc: >> ---------------------- >> Pretimeouts: >> >> Some watchdog timers can be set to have a trigger go off before the >> actual time they will reset the system. This can be done with an NMI, >> interrupt, or other mechanism. This allows Linux to record useful >> information (like panic information and kernel coredumps) before it >> resets. >> ---------------------- >> >> I think panic() is the right way to do now. but people may also need >> to config : >> PANIC_TIMEOUT [!=0] >> KEXEC >> KDUMP >> for debug reason >> > > Yes, so basically if we hit pretimeout, we probably have already crashed. yes, for now , I only use panic(), but at the beginning, I offer user some options: https://lists.linaro.org/pipermail/linaro-acpi/2015-April/004350.html > The only difference is that we now have some seconds time to dump out useful > data and then either reboot or let the actual watchdog reset take care of > resetting the device. panic() sounds like a good thing to do. Maybe you > could also dump registers on other CPUs too or try to get out some other > useful information about the kernel in case it has crashed or something. But > I'm just guessing. yes, that is my thought. because there is the kdump support in panic(), so that can give use a chance to figure out why the watchdog wasn't fed. > >>> >>> The way I was planning the API change there would need to be a small >>> change >>> with each watchdog driver in order to let the watchdog core take over >>> generic behaviour on behalf of the driver. My goal was to make the change >>> so >>> that each driver that gets converted to the new API extensions gets a >>> support for early-timeout-sec for free, without needing to enable support >>> for it any way. If the API was designed properly, also pretimeouts could >>> be >>> handled easily and maybe even so that other drivers could have that >>> feature >>> even though their hardware does not explicitly give any support for it. >> >> >> could you please point out your patch , then I can learn your idea :-) >> For now , I am working on a common "Pretimeouts" following the concept >> in kernel doc. >> >>> >>> Any thoughts? >> >> >> my thoughts is in my pretimeout patch , would you provide some suggestion >> ? >> > > Here is an archive link to my patch set: > > http://www.spinics.net/lists/linux-watchdog/msg06473.html Ah , cool, I can see some common in your patch. Thanks :-) See if I can learn something from your patches > > Your patch set is adding a new call to the core for adjusting the > pretimoeut, which is probably a good thing in case the driver needs special > handling for it anyway. But if the core had capability to emulate pretimeout > for drivers that can't support it in hardware, it would be good if there was > a way for the core to support it even though the driver had zero code for > it. The core also has watchdog_init_timeout() right now but even that is not > called from that many drivers. I would like to fix that too so that drivers > would not need to bother so much about it but let core take care of it more. > This is why I proposed the watchdog_init_params() call that could dig all > the generic parameters from device tree on behalf of the driver. This is > where I though timeout and early-timeout-sec parameters would be parsed from > device tree, but it could also parse pretimeout parameter as well. > Apparently Guenter did not like my approach so we might need some other way > to do it. I am using pretimeout, because SBSA watchdog hardware support two stages timeout, I am reusing the existing kernel concept, but your early_timeout_sec is a new concept. If you check my previous patchset , you will see : at the beginning, pretimeout support is not a generic features in my patchset. Then Arnd suggest that I can try to make pretimeout into watchdog framework, and Guenter said :"Makes sense." So I am still trying to improve pretimeout support :-) If I can make pretimeout merged, may be you can try pretimeout to implement early_timeout_sec function? It is up to the maintainers, I will try my best. Thanks :-) > > I don't get to say how this should be done, I'm just throwing ideas here. > But I think the watchdog core would be a lot better place for implementing > the generic features than drivers. That way a lot of drivers could enable > support the new features for free. > > Thanks, > -Timo
Hi Guenter. Sorry for my poor English . let me explain this : On 22 May 2015 at 21:23, Guenter Roeck <linux@roeck-us.net> wrote: > On 05/22/2015 03:46 AM, Fu Wei wrote: >> >> Hi Timo, >> > [ ... ] > >> So I am still trying to improve pretimeout support :-) > > > Is there anything still missing from it ? > >> If I can make pretimeout merged, may be you can try pretimeout to >> implement early_timeout_sec function? > > > Not sure how one would or even could do that. > > Do you mean "implement early_pretimeout_sec", by any chance ? I mean: using pretimeout to implement the function you want, instead of early_pretimeout_sec Hope I say the right word this time :-) > >> It is up to the maintainers, I will try my best. >> > > Please don't make the pretimeout concept more complicated than necessary. > > The smaller the patch, the more likely it is to get accepted. > The more you change, the more difficult it is for the maintainer to, > for example, back-port later bug fixes into earlier kernel releases > when needed. This is why it is, for example, better to keep the > existing watchdog_init_timeout() function instead of just replacing > it with watchdog_init_timeouts(). > > Try to put yourself into the maintainer's perspective: If you were > the maintainer, would you rather accept a patch or patch set which > maintains the existing API and doesn't require any changes to existing > drivers, or would you accept one that changes, say, some function > or variable names and will require manual back-ports later on if > there is a bug fix ? Would you rather accept a patch that adds 50 lines > of code, or one that changes another 100+ lines and rearranges everything > along the line ? > > Thanks, > Guenter >
Hi Guenter, On 22 May 2015 at 23:05, Guenter Roeck <linux@roeck-us.net> wrote: > On Fri, May 22, 2015 at 10:38:32PM +0800, Fu Wei wrote: >> Hi Guenter. >> >> Sorry for my poor English . >> let me explain this : >> >> On 22 May 2015 at 21:23, Guenter Roeck <linux@roeck-us.net> wrote: >> > On 05/22/2015 03:46 AM, Fu Wei wrote: >> >> >> >> Hi Timo, >> >> >> > [ ... ] >> > >> >> So I am still trying to improve pretimeout support :-) >> > >> > >> > Is there anything still missing from it ? >> > >> >> If I can make pretimeout merged, may be you can try pretimeout to >> >> implement early_timeout_sec function? >> > >> > >> > Not sure how one would or even could do that. >> > >> > Do you mean "implement early_pretimeout_sec", by any chance ? >> >> I mean: using pretimeout to implement the function you want, instead >> of early_pretimeout_sec >> > How would this work if the watchdog hardware doesn't support pretimeout ? > > Pretimeout and early timeout are two logically different functions, with > different goals, so I don't entirely (if at all) understand why it would > make sense to tie them together. > > Can you elaborate why you think this would be a good idea ? sorry, my apology. forget about this, :-) I think we should focus on SBSA watchdog patch here, but not a early_timeout_sec. > > Thanks, > Guenter > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, Great thanks for your suggestion, I have put this kind of validation into watchdog_pretimeout_invalid and watchdog_timeout_invalid. So : (1) set_timeout(10); ------> if this setting is successful set_pretimeout(20); -----> return fail (-EINVAL) (2) set_timeout(10); ------> if this setting is successful set_pretimeout(10); -----> return fail (-EINVAL) this kind of situation will not result in an invalid / unexpected timeout value. you will see this change in my next patchset On 21 May 2015 at 23:32, Guenter Roeck <linux@roeck-us.net> wrote: > On Thu, May 21, 2015 at 04:32:34PM +0800, fu.wei@linaro.org wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> Also update Documentation/watchdog/watchdog-kernel-api.txt to >> introduce: >> (1)the new elements in the watchdog_device and watchdog_ops struct; >> (2)the new API "watchdog_init_timeouts". >> >> Reasons: >> (1)kernel already has two watchdog drivers are using "pretimeout": >> drivers/char/ipmi/ipmi_watchdog.c >> drivers/watchdog/kempld_wdt.c(but the definition is different) >> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- > > [ ... ] > >> >> +/* 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->max_pretimeout != 0) && >> + (t < wdd->min_pretimeout || t > wdd->max_pretimeout)); >> +} > > Should this function also enforce "t < wdd->timeout", and > should watchdog_timeout_invalid() enforce "t > wdd->pretimeout" ? > > Thanks, > Guenter
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index a0438f3..43900df 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -49,6 +49,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int pretimeout; + unsigned int min_pretimeout; + unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status; @@ -70,6 +73,9 @@ It contains following fields: * timeout: the watchdog timer's timeout value (in seconds). * min_timeout: the watchdog timer's minimum timeout value (in seconds). * max_timeout: the watchdog timer's maximum timeout value (in seconds). +* pretimeout: the watchdog timer's pretimeout value (in seconds). +* min_pretimeout: the watchdog timer's minimum pretimeout value (in seconds). +* max_pretimeout: the watchdog timer's maximum pretimeout value (in 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. @@ -92,6 +98,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 *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *); @@ -153,9 +160,19 @@ they are supported. These optional routines/operations are: and -EIO for "could not write value to the watchdog". On success this routine should set the timeout value of the watchdog_device to the achieved timeout value (which may be different from the requested one - because the watchdog does not necessarily has a 1 second resolution). + because the watchdog does not necessarily has a 1 second resolution; + If the driver supports pretimeout, then the timeout value must be greater + than that). (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the watchdog's info structure). +* set_pretimeout: this routine checks and changes the pretimeout of the + watchdog timer device. It returns 0 on success, -EINVAL for "parameter out of + range" and -EIO for "could not write value to the watchdog". On success this + routine should set the pretimeout value of the watchdog_device to the + achieved pretimeout value (which may be different from the requested one + because the watchdog does not necessarily has a 1 second resolution). + (Note: the WDIOF_PRETIMEOUT needs to be set in the options field of the + watchdog's info structure). * get_timeleft: this routines returns the time that's left before a reset. * ref: the operation that calls kref_get on the kref of a dynamically allocated watchdog_device struct. @@ -213,14 +230,41 @@ The watchdog_get_drvdata function allows you to retrieve driver specific data. The argument of this function is the watchdog device where you want to retrieve data from. The function returns the pointer to the driver specific data. -To initialize the timeout field, the following function can be used: +To initialize the timeout and pretimeout fields, the following function can be +used: -extern int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev); +extern int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + void (*update_limits)(struct watchdog_device *), + struct device *dev); -The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +The watchdog_init_timeouts function allows you to initialize the pretimeout and +timeout fields using the module pretimeout and timeout parameter or by +retrieving the elements in the timeout-sec property(the first element is for +timeout, the second one is for pretimeout) from the device tree(if the module +pretimeout and timeout parameter are invalid). +Normally, the pretimeout value will affect the limitation of timeout, and it +is also hardware related. So you can write a function in your driver to update +the limitation of timeout, according to the pretimeout value. Then pass the +function pointer by the update_limits parameter. If you driver doesn't +need this adjustment, just pass NULL to the update_limits parameter. +Most of watchdog timers have not pretimeout as the warning signal. They just +reset the system, once the timeout watch period expires. In this case, we can +pass 0 to the pretimeout_parm, and pass NULL to the update_limits. Or use the +old API: watchdog_init_timeout(see below) +Best practice is to set the default pretimeout and timeout value as pretimeout +and timeout value in the watchdog_device and then use this function to set the +user "preferred" pretimeout value. This routine returns zero on success and a negative errno code for failure. + +For backward compatibility, we also support the timeout initialization API: + +static inline int watchdog_init_timeout(struct watchdog_device *wdd, + unsigned int timeout_parm, + struct device *dev); + +Because of the introduction of pretimeout and watchdog_init_timeouts, this +function has become a small wrapper function of watchdog_init_timeouts. + + diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cec9b55..460796e 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -43,60 +43,115 @@ static DEFINE_IDA(watchdog_ida); static struct class *watchdog_class; -static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) +static void watchdog_check_min_max_timeouts(struct watchdog_device *wdd) { /* - * Check that we have valid min and max timeout values, if - * not reset them both to 0 (=not used or unknown) + * Check that we have valid min and max pretimeout and timeout values, + * if not reset them both to 0 (=not used or unknown) */ + if (wdd->min_pretimeout > wdd->max_pretimeout) { + pr_info("Invalid min and max pretimeout, resetting to 0!\n"); + wdd->min_pretimeout = 0; + wdd->max_pretimeout = 0; + } if (wdd->min_timeout > wdd->max_timeout) { pr_info("Invalid min and max timeout values, resetting to 0!\n"); wdd->min_timeout = 0; wdd->max_timeout = 0; } + /* + * Check that we have valid min and max timeout values, + * if not reset them both to pretimeout limits + */ + if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout) { + pr_info("Invalid min timeout, resetting to min pretimeout!\n"); + wdd->min_timeout = wdd->min_pretimeout; + } + if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout) { + pr_info("Invalid max timeout, resetting to max pretimeout!\n"); + wdd->max_timeout = wdd->max_pretimeout; + } } /** - * watchdog_init_timeout() - initialize the timeout field + * watchdog_init_timeouts() - initialize the pretimeout and timeout field + * @pretimeout_parm: pretimeout module parameter * @timeout_parm: timeout module parameter * @dev: Device that stores the timeout-sec property * - * Initialize the timeout field of the watchdog_device struct with either the - * timeout module parameter (if it is valid value) or the timeout-sec property - * (only if it is a valid value and the timeout_parm is out of bounds). - * If none of them are valid then we keep the old value (which should normally - * be the default timeout value. + * Initialize the pretimeout and timeout field of the watchdog_device struct + * with either the pretimeout and timeout module parameter (if it is valid + * value) or the timeout-sec property (only if it is a valid value and the + * pretimeout_parm and timeout_parm is out of bounds). If none of them are + * valid then we keep the old value (which should normally be the default + * timeout value. * * A zero is returned on success and -EINVAL for failure. */ -int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev) +int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + void (*update_limits)(struct watchdog_device *), + struct device *dev) { - unsigned int t = 0; + unsigned int timeout = 0, pretimeout = 0; + const __be32 *ppretimeout; int ret = 0; + struct property *timeout_sec; + int length; - watchdog_check_min_max_timeout(wdd); + watchdog_check_min_max_timeouts(wdd); - /* try to get the timeout module parameter first */ - if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) { - wdd->timeout = timeout_parm; - return ret; + /* try to get the timeout and pretimeout module parameter first */ + if (pretimeout_parm) { + if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm)) { + wdd->pretimeout = pretimeout_parm; + if (update_limits) + update_limits(wdd); + } else { + pr_warn("Invalid pretimeout parameter!\n"); + ret = -EINVAL; + } } - if (timeout_parm) + + if (timeout_parm) { + if (!watchdog_timeout_invalid(wdd, timeout_parm)) { + wdd->timeout = timeout_parm; + return ret; + } + pr_warn("Invalid timeout parameter!\n"); ret = -EINVAL; + } /* try to get the timeout_sec property */ if (dev == NULL || dev->of_node == NULL) return ret; - of_property_read_u32(dev->of_node, "timeout-sec", &t); - if (!watchdog_timeout_invalid(wdd, t) && t) - wdd->timeout = t; - else + + timeout_sec = of_find_property(dev->of_node, "timeout-sec", &length); + if (timeout_sec) { + ppretimeout = of_prop_next_u32(timeout_sec, NULL, &timeout); + if (length == 2) { + of_prop_next_u32(timeout_sec, ppretimeout, &pretimeout); + if (!watchdog_pretimeout_invalid(wdd, pretimeout) && + pretimeout) { + wdd->pretimeout = pretimeout; + if (update_limits) + update_limits(wdd); + } else { + ret = -EINVAL; + } + } + if (!watchdog_timeout_invalid(wdd, timeout) && timeout) + wdd->timeout = timeout; + else + ret = -EINVAL; + } else { ret = -EINVAL; + } return ret; } -EXPORT_SYMBOL_GPL(watchdog_init_timeout); +EXPORT_SYMBOL_GPL(watchdog_init_timeouts); /** * watchdog_register_device() - register a watchdog device @@ -119,7 +174,7 @@ int watchdog_register_device(struct watchdog_device *wdd) if (wdd->ops->start == NULL || wdd->ops->stop == NULL) return -EINVAL; - watchdog_check_min_max_timeout(wdd); + watchdog_check_min_max_timeouts(wdd); /* * Note: now that all watchdog_device data has been verified, we diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 6aaefba..b519257 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -218,6 +218,38 @@ out_timeout: } /* + * watchdog_set_pretimeout: set the watchdog timer pretimeout + * @wddev: the watchdog device to set the timeout for + * @pretimeout: pretimeout to set in seconds + */ + +static int watchdog_set_pretimeout(struct watchdog_device *wddev, + unsigned int pretimeout) +{ + int err; + + if (!wddev->ops->set_pretimeout || + !(wddev->info->options & WDIOF_PRETIMEOUT)) + return -EOPNOTSUPP; + + if (watchdog_pretimeout_invalid(wddev, pretimeout)) + return -EINVAL; + + mutex_lock(&wddev->lock); + + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_pretimeout; + } + + err = wddev->ops->set_pretimeout(wddev, pretimeout); + +out_pretimeout: + mutex_unlock(&wddev->lock); + return err; +} + +/* * watchdog_get_timeleft: wrapper to get the time left before a reboot * @wddev: the watchdog device to get the remaining time from * @timeleft: the time that's left @@ -388,6 +420,22 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd, if (wdd->timeout == 0) return -EOPNOTSUPP; return put_user(wdd->timeout, p); + case WDIOC_SETPRETIMEOUT: + if (get_user(val, p)) + return -EFAULT; + err = watchdog_set_pretimeout(wdd, val); + if (err < 0) + return err; + /* If the watchdog is active then we send a keepalive ping + * to make sure that the watchdog keep's running (and if + * possible that it takes the new timeout) */ + watchdog_ping(wdd); + /* Fall */ + case WDIOC_GETPRETIMEOUT: + /* timeout == 0 means that we don't use the pretimeout */ + if (wdd->pretimeout == 0) + return -EOPNOTSUPP; + return put_user(wdd->pretimeout, p); case WDIOC_GETTIMELEFT: err = watchdog_get_timeleft(wdd, &val); if (err) diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index a746bf5..df430a3 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -25,6 +25,7 @@ struct watchdog_device; * @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. + * @set_pretimeout:The routine for setting the watchdog devices pretimeout value * @get_timeleft:The routine that get's the time that's left before a reset. * @ref: The ref operation for dyn. allocated watchdog_device structs * @unref: The unref operation for dyn. allocated watchdog_device structs @@ -44,6 +45,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 *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *); @@ -62,6 +64,9 @@ struct watchdog_ops { * @timeout: The watchdog devices timeout value. * @min_timeout:The watchdog devices minimum timeout value. * @max_timeout:The watchdog devices maximum timeout value. + * @pretimeout: The watchdog devices pretimeout value. + * @min_pretimeout:The watchdog devices minimum pretimeout value. + * @max_pretimeout:The watchdog devices maximum pretimeout value. * @driver-data:Pointer to the drivers private data. * @lock: Lock for watchdog core internal use only. * @status: Field that contains the devices internal status bits. @@ -86,6 +91,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int pretimeout; + unsigned int min_pretimeout; + unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status; @@ -120,6 +128,14 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne (t < wdd->min_timeout || 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->max_pretimeout != 0) && + (t < wdd->min_pretimeout || t > wdd->max_pretimeout)); +} + /* Use the following functions to manipulate watchdog driver specific data */ static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data) { @@ -132,11 +148,21 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd) } /* drivers/watchdog/watchdog_core.c */ -extern int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev); +extern int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + void (*update_limits)(struct watchdog_device *), + struct device *dev); extern int watchdog_register_device(struct watchdog_device *); extern void watchdog_unregister_device(struct watchdog_device *); +static inline int watchdog_init_timeout(struct watchdog_device *wdd, + unsigned int timeout_parm, + struct device *dev) +{ + return watchdog_init_timeouts(wdd, 0, timeout_parm, NULL, dev); +} + #ifdef CONFIG_HARDLOCKUP_DETECTOR void watchdog_nmi_disable_all(void); void watchdog_nmi_enable_all(void);