* Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter [not found] <20220918140829.443722-1-git@vladimir.panteleev.md> @ 2022-09-19 4:17 ` Guenter Roeck 2022-09-19 5:58 ` Vladimir Panteleev 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2022-09-19 4:17 UTC (permalink / raw) To: Vladimir Panteleev, Wim Van Sebroeck; +Cc: linux-watchdog, linux-kernel On 9/18/22 07:08, Vladimir Panteleev wrote: > Allow configuring the "action" bit, as documented in [1]. > > Previously, the only action supported by this module was to reset the > system (0). It can now be configured to power off (1) instead. > > [1]: https://www.amd.com/system/files/TechDocs/44413.pdf > > Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md> > --- > drivers/watchdog/sp5100_tco.c | 13 +++++++++++-- > drivers/watchdog/sp5100_tco.h | 2 +- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > index ae54dd33e233..802f12e8fd76 100644 > --- a/drivers/watchdog/sp5100_tco.c > +++ b/drivers/watchdog/sp5100_tco.c > @@ -65,6 +65,12 @@ static struct pci_dev *sp5100_tco_pci; > > /* module parameters */ > > +#define WATCHDOG_ACTION 0 > +static bool action = WATCHDOG_ACTION; > +module_param(action, bool, 0444); > +MODULE_PARM_DESC(action, "Action taken when watchdog expires, 0 to reset, 1 to poweroff (default=" > + __MODULE_STRING(WATCHDOG_ACTION) ")"); > + Other module parameters are not visible. I do not see the benefit of having this one visible. > #define WATCHDOG_HEARTBEAT 60 /* 60 sec default heartbeat. */ > static int heartbeat = WATCHDOG_HEARTBEAT; /* in seconds */ > module_param(heartbeat, int, 0); > @@ -297,8 +303,11 @@ static int sp5100_tco_timer_init(struct sp5100_tco *tco) > if (val & SP5100_WDT_FIRED) > wdd->bootstatus = WDIOF_CARDRESET; > > - /* Set watchdog action to reset the system */ > - val &= ~SP5100_WDT_ACTION_RESET; > + /* Set watchdog action */ > + if (action) > + val |= SP5100_WDT_ACTION; > + else > + val &= ~SP5100_WDT_ACTION; > writel(val, SP5100_WDT_CONTROL(tco->tcobase)); > > /* Set a reasonable heartbeat before we stop the timer */ > diff --git a/drivers/watchdog/sp5100_tco.h b/drivers/watchdog/sp5100_tco.h > index 6a0986d2c94b..9752ab2b0130 100644 > --- a/drivers/watchdog/sp5100_tco.h > +++ b/drivers/watchdog/sp5100_tco.h > @@ -18,7 +18,7 @@ > > #define SP5100_WDT_START_STOP_BIT BIT(0) > #define SP5100_WDT_FIRED BIT(1) > -#define SP5100_WDT_ACTION_RESET BIT(2) > +#define SP5100_WDT_ACTION BIT(2) I do not see the point of renaming this define. > #define SP5100_WDT_DISABLED BIT(3) > #define SP5100_WDT_TRIGGER_BIT BIT(7) > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter 2022-09-19 4:17 ` [PATCH] watchdog: sp5100_tco: Add "action" module parameter Guenter Roeck @ 2022-09-19 5:58 ` Vladimir Panteleev 2022-09-19 12:29 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Vladimir Panteleev @ 2022-09-19 5:58 UTC (permalink / raw) To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel On Mon, 19 Sept 2022 at 04:17, Guenter Roeck <linux@roeck-us.net> wrote: > > On 9/18/22 07:08, Vladimir Panteleev wrote: > > +MODULE_PARM_DESC(action, "Action taken when watchdog expires, 0 to reset, 1 to poweroff (default=" > > + __MODULE_STRING(WATCHDOG_ACTION) ")"); > > + > > Other module parameters are not visible. I do not see the benefit of > having this one visible. My bad > > -#define SP5100_WDT_ACTION_RESET BIT(2) > > +#define SP5100_WDT_ACTION BIT(2) > > I do not see the point of renaming this define. The bit is just called "action" ("WatchDogAction") in the technical documentation. I figure that the original author chose to name the define "ACTION_RESET" instead of just "ACTION" because the original implementation only ever cleared this bit, therefore only setting the action to "reset". Now that this is no longer true, calling it simply "action" to match the spec seemed more appropriate. What do you think? Thanks! - Vladimir ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter 2022-09-19 5:58 ` Vladimir Panteleev @ 2022-09-19 12:29 ` Guenter Roeck 2022-09-19 13:18 ` Vladimir Panteleev 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2022-09-19 12:29 UTC (permalink / raw) To: Vladimir Panteleev; +Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel On 9/18/22 22:58, Vladimir Panteleev wrote: > On Mon, 19 Sept 2022 at 04:17, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 9/18/22 07:08, Vladimir Panteleev wrote: >>> +MODULE_PARM_DESC(action, "Action taken when watchdog expires, 0 to reset, 1 to poweroff (default=" >>> + __MODULE_STRING(WATCHDOG_ACTION) ")"); >>> + >> >> Other module parameters are not visible. I do not see the benefit of >> having this one visible. > > My bad > >>> -#define SP5100_WDT_ACTION_RESET BIT(2) >>> +#define SP5100_WDT_ACTION BIT(2) >> >> I do not see the point of renaming this define. > > The bit is just called "action" ("WatchDogAction") in the technical > documentation. I figure that the original author chose to name the > define "ACTION_RESET" instead of just "ACTION" because the original > implementation only ever cleared this bit, therefore only setting the > action to "reset". Now that this is no longer true, calling it simply > "action" to match the spec seemed more appropriate. What do you think? > I am not getting into define name editing wars. The define is named as it is. There is never a good reason to rename it. If I'd accept your change, someone else might come tomorrow and want it renamed to "SP5100_WDT_ACTION_POWEROFF" because setting the bit to 1 causes the system to power off. No, I am not getting into that. Guenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter 2022-09-19 12:29 ` Guenter Roeck @ 2022-09-19 13:18 ` Vladimir Panteleev 2022-09-19 13:27 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Vladimir Panteleev @ 2022-09-19 13:18 UTC (permalink / raw) To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel Hi Guenter, On Mon, 19 Sept 2022 at 12:29, Guenter Roeck <linux@roeck-us.net> wrote: > I am not getting into define name editing wars. The define is named as > it is. There is never a good reason to rename it. If I'd accept your > change, someone else might come tomorrow and want it renamed to > "SP5100_WDT_ACTION_POWEROFF" because setting the bit to 1 causes > the system to power off. Ah, sorry - this is one of my first attempts at contributing to the kernel, and as such I of course fully defer to you. In case I was misunderstood, I was just trying to explain my line of reasoning at the time, which is what I thought you asked for in your previous message. Thank you for the explanation, I was not aware of the high cost of renaming defines. I will send a V2 if this is all? Thank you for your time :) Best regards, - Vladimir ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter 2022-09-19 13:18 ` Vladimir Panteleev @ 2022-09-19 13:27 ` Guenter Roeck 0 siblings, 0 replies; 5+ messages in thread From: Guenter Roeck @ 2022-09-19 13:27 UTC (permalink / raw) To: Vladimir Panteleev; +Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel On Mon, Sep 19, 2022 at 01:18:53PM +0000, Vladimir Panteleev wrote: > Hi Guenter, > > On Mon, 19 Sept 2022 at 12:29, Guenter Roeck <linux@roeck-us.net> wrote: > > I am not getting into define name editing wars. The define is named as > > it is. There is never a good reason to rename it. If I'd accept your > > change, someone else might come tomorrow and want it renamed to > > "SP5100_WDT_ACTION_POWEROFF" because setting the bit to 1 causes > > the system to power off. > > Ah, sorry - this is one of my first attempts at contributing to the > kernel, and as such I of course fully defer to you. In case I was > misunderstood, I was just trying to explain my line of reasoning at > the time, which is what I thought you asked for in your previous > message. Thank you for the explanation, I was not aware of the high > cost of renaming defines. > > I will send a V2 if this is all? > Sounds good. Thanks, Guenter ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-19 13:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220918140829.443722-1-git@vladimir.panteleev.md>
2022-09-19 4:17 ` [PATCH] watchdog: sp5100_tco: Add "action" module parameter Guenter Roeck
2022-09-19 5:58 ` Vladimir Panteleev
2022-09-19 12:29 ` Guenter Roeck
2022-09-19 13:18 ` Vladimir Panteleev
2022-09-19 13:27 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).