Re: [PATCH v2] kern/efi: Adding efi-watchdog command

[Erwan Velu]

Thanks for your review, I'll rework my patch according to your comments.
I just kept the open points in this thread.

Le 09/09/2021 à 17:46, Daniel Kiper a écrit :
> +static grub_err_t grub_efi_set_watchdog_timer(unsigned long timeout)
> Please use grub_efi_uintn_t instead of unsigned long. However, please
> remember that its size depends on architecture. So, it can be 32-bits or
> 64-bits...
I'll follow the uefi spec here so grub_efi_uintn_ is fine.
> > +    The numeric code to log on a watchdog timer timeout event.
> > +    The firmware reserves codes 0x0000 to 0xFFFF.
> > +    Loaders and operating systems may use other timeout codes.
> This comment says you cannot use 0xb007. I would use 0xdeadb007deadb007
> by default. However, I would give an option to change the default by user.

Stupid me ....

I was wondering if the code should be configurable.

In the initial patch it was the case but though this would give an 
easier command line interface to users.

I mean, the aim of this code is to report mostly 'who' manipulated the 
watchdog, so sound good to me to get a value that is more grub depend 
than user centric.

> > +  */
> Please format comments according to [2].
>
> > +  grub_efi_uint64_t code = 0xB007;
> > +  grub_efi_status_t status = 0;
> > +
> > +  if (timeout >= 0xFFFF)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("efi-watchdog: timeout must be 
> > lower than 65535"));
> Why do you artificially limit the timeout value? Please do not do that.

Mistake of my own, make sense to remove that.


> > +  if (timeout > 0)
> > +    grub_printf("grub %s: Arming EFI watchdog at %lu seconds\n", 
> > PACKAGE_VERSION, timeout);
> If you want debug messages please use grub_dprintf() instead of grub_printf().

Maybe my print isn't ideal but I found important to warn the user a 
watchdog is armed.

If the user is not informed and the watchdog fires, the user could think 
there is a hardware issue on the host.

So to me, this message is mandatory to say users : "beware, the system 
will be reset within x seconds if you don't boot"