Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter

Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter

[Guenter Roeck]

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)
>   

Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter

[Vladimir Panteleev]

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

Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter

[Guenter Roeck]

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

Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter

[Vladimir Panteleev]

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

Re: [PATCH] watchdog: sp5100_tco: Add "action" module parameter

[Guenter Roeck]

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