Re: [PATCH] rtc: rtc-efi: Add an enable parameter and make it false for X86 by default

[Matt Fleming]

On Tue, 02 May, at 08:51:47AM, Ocean HY1 He wrote:
> The commit 7efe665903d0 ("rtc: Disable EFI rtc for x86") turns off rtc-efi
> option completely for x86 in rtc/Kconfig, to avoid possible crash caused by
> buggy implementations of the time-related EFI runtime services.
> 
> In fact, there are more and more UEFI firmware has time-related EFI runtime
> services well done. To meet EFI rtc request for X86, here adds an enable
> parameter which could be explicitly set as true when load rtc-efi module.
> To keep consistency, make this enable parameter false for X86 by default.
> 
> The test passes on Lenovo ThinkServer RD350 while the UEFI/BIOS version is
> VB3TS424 and processor is Intel(R) Xeon(R) CPU E5-2660 v3.
> #modprobe rtc_efi enable=1
> #cat /sys/class/rtc/rtc1/name
> rtc-efi
> #hwclock --rtc=/dev/rtc1 -w
> #hwclock --rtc=/dev/rtc1 -r
> 
> Signed-off-by: Ocean He <hehy1@xxxxxxxxxx>
> ---
>  drivers/rtc/Kconfig   |  2 +-
>  drivers/rtc/rtc-efi.c | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
 
The key piece of info missing from this changelog is: why?

Why does it make sense to allow this driver to be enabled for x86? All
the RTC functions should be available via other, better features on
x86.
 
There's no justification for enabling it just because it works on some
platforms, you need to show it is *essential* somehow.

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index ee1b0e9..482200a 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1037,7 +1037,7 @@ config RTC_DRV_DA9063
>  
>  config RTC_DRV_EFI
>  	tristate "EFI RTC"
> -	depends on EFI && !X86
> +	depends on EFI
>  	help
>  	  If you say yes here you will get support for the EFI
>  	  Real Time Clock.
> diff --git a/drivers/rtc/rtc-efi.c b/drivers/rtc/rtc-efi.c
> index 0130afd..5a8427f 100644
> --- a/drivers/rtc/rtc-efi.c
> +++ b/drivers/rtc/rtc-efi.c
> @@ -23,6 +23,14 @@
>  #include <linux/rtc.h>
>  #include <linux/efi.h>
>  
> +/*
> + * Disable the use of rtc-efi as a RTC for X86 by default. This setting can be
> + * overridden using this module's enable parameter.
> + */
> +static bool efi_rtc_enable = !IS_ENABLED(CONFIG_X86);
> +
> +module_param_named(enable, efi_rtc_enable, bool, 0644);
> +
>  #define EFI_ISDST (EFI_TIME_ADJUST_DAYLIGHT|EFI_TIME_IN_DAYLIGHT)
>  
>  /*
> @@ -262,6 +270,9 @@ static int __init efi_rtc_probe(struct platform_device *dev)
>  	efi_time_t eft;
>  	efi_time_cap_t cap;
>  
> +	if (!efi_rtc_enable)
> +		return -ENODEV;
> +
>  	/* First check if the RTC is usable */
>  	if (efi.get_time(&eft, &cap) != EFI_SUCCESS)
>  		return -ENODEV;
> -- 
> 1.8.3.1