Re: [PATCH] Watchdog: fix pretimeout noop governor logging and description

[Guenter Roeck]

On 6/16/26 10:19, Lorenzo Egidio wrote:
> Overview
> This review is dedicated to a pretimeout governor implementation of Linux kernel watchdog ("noop"). The overall module organization and the registration mechanism are done rightly and in accordance with the kernel standards. But, two points were noticed that could result in either compilation errors or difficulties in maintainability.
> 1. Potential problem: pr_alert usage with the wdd->id
> Within the callback function:
> static void pretimeout_noop(struct watchdog_device *wdd) { pr_alert("watchdog%d: pretimeout event\n", wdd->id); }
> Problem
> The id field of the struct watchdog_device is not ensured to be present in all Linux kernel versions. Actually, in many recent kernel versions, this field is either missing or not exposed in the public structure API.
> Consequence
> Compilation may fail with the following error at compile time:error: 'struct watchdog_device' has no member named 'id'
> Portability of the code to different kernel versions is lessened as it depends on the details of the internal structure.

Your AI doesn't know what it is talking about.

> Suggestions
> Do not directly access wdd->id. It is better to use a general message or a stable field which is safe across versions if available.
> An example of a safe alternative:
> pr_alert("watchdog: pretimeout event\n");
> 2. Mismatch in MODULE_DESCRIPTION
> The present module declares:
> MODULE_DESCRIPTION("Panic watchdog pretimeout governor");
> Problem
> There is a difference between the module description and the very name of the governor:
> .name = "noop"
> The code clearly states a "noop" governor, but the description talks about a "panic" governor, thereby causing misunderstanding.
> Result
> Developers and maintainers get confused
> The module metadata is not correctly described
> There is a decrease in clarity in logs and documentation
>
> Suggestion
> Correct the module description to be in line with the actual operation of the implementation.
> Example:
> MODULE_DESCRIPTION("Noop watchdog pretimeout governor");
> Conclusion
> The implementation is good structurally and it fits well with the Linux kernel watchdog governor framework. On the other hand, the usage of wdd->id could result in the incompatibility problem with the different kernel versions and the module metadata carries a description inconsistency that has to be fixed for the better clarity and maintainability.
>
> Signed-off-by: Lorenzo Egidio <lorenzoegidioms@xxxxxxxxx>
> ---
>   drivers/watchdog/pretimeout_noop.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/pretimeout_noop.c b/drivers/watchdog/pretimeout_noop.c
> index 74ec02b9f..ddb5192fb 100644
> --- a/drivers/watchdog/pretimeout_noop.c
> +++ b/drivers/watchdog/pretimeout_noop.c
> @@ -17,7 +17,7 @@
>    */
>   static void pretimeout_noop(struct watchdog_device *wdd)
>   {
> -	pr_alert("watchdog%d: pretimeout event\n", wdd->id);
> +	pr_alert("watchdog%d: pretimeout event\n");

Seriously ? Your commit description is obviously AI generated,
and (hopefully) so is this code. And, even more obviously,
you did not even try to compile it, or you ignored the error/warning
produced by the compile attempt. And the reasoning is completely
wrong.

Please stop wasting my time.

Guenter

>   }
>   
>   static struct watchdog_governor watchdog_gov_noop = {
> @@ -38,5 +38,5 @@ module_init(watchdog_gov_noop_register);
>   module_exit(watchdog_gov_noop_unregister);
>   
>   MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>");
> -MODULE_DESCRIPTION("Panic watchdog pretimeout governor");
> +MODULE_DESCRIPTION("Noop watchdog pretimeout governor");
>   MODULE_LICENSE("GPL");