Re: [PATCH v2] rtc: efi: restore alarm support with runtime capability probe

[Ard Biesheuvel]

On Mon, 31 Aug 2026, at 13:38, Johnny.Fan wrote:
> From: "Johnny.Fan" <Johnny.Fan@xxxxxxxxxxx>
>
> Restore the EFI RTC alarm functionality (read_alarm/set_alarm and
> procfs output) that was removed by commit 18a3510bc87d ("rtc: efi:
> Remove wakeup functionality").
>
> The removal was motivated by the observation that many EFI platforms
> lack RTC wakeup support while also omitting the RT_PROP table, causing
> the wakeup capability to be incorrectly advertised.  While that is
> true, removing the feature entirely also breaks platforms where the
> wakeup runtime service actually works.
>
> On CIX SoCs, the RTC is an I2C device owned by firmware, and in the
> ACPI boot configuration the I2C bus it sits on is not exposed to the
> OS, so the EFI runtime services are the only way Linux can access
> the RTC at all.  Both Linux and Windows use this same interface by
> design, so the RTC state is consistent across the operating systems
> installed on the machine, and Windows relies on the EFI wakeup
> services for its own RTC alarm support.
>
> With the alarm support removed from rtc-efi, Linux loses the
> equivalent capability entirely on these platforms: rtcwake fails
> for both suspend-to-RAM and power-off wakeup, as there is no other
> path to program the RTC alarm.  This is a real product requirement
> that CIX customers have explicitly requested, not a hypothetical
> use case.
>
> To avoid exposing a broken alarm on firmware that does not implement
> the wakeup runtime services, probe the capability at ->probe() time by
> actually calling GetWakeupTime() -- if it fails, clear RTC_FEATURE_ALARM
> and the device behaves as before the removal.
>
> Also set RTC_FEATURE_ALARM_WAKEUP_ONLY since EFI wakeup time is a
> wakeup-only alarm, not a periodic/update interrupt source.
>
> Reviewed-by: Feng Tang <feng.tang@xxxxxxxxxxxxxxxxx>
> Reviewed-by: Fugang Duan <fugang.duan@xxxxxxxxxxx>
> Tested-by: Johnny.Fan <Johnny.Fan@xxxxxxxxxxx>

Please drop this tested-by - it is implied.

With that fixed, and the tweak below,

Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx>


> Signed-off-by: Johnny.Fan <Johnny.Fan@xxxxxxxxxxx>
> ---
> v2:
>   - Expand the commit message with the actual product motivation,
>     as requested by Ard.
>   - Zero-initialize efi_time_t in convert_to_efi_time() to avoid
>     leaking stack garbage in pad1/pad2 fields to firmware.
>   - Zero-initialize the local eft in efi_read_alarm() so that
>     convert_from_efi_time() does not fail with -EIO on disabled
>     alarms, where the returned time is undefined per the UEFI spec.
>   - Normalize wkalrm->enabled to a strict boolean (!!) before
>     passing it to SetWakeupTime, as UEFI BOOLEAN must be 0 or 1.
>   - Use %d for the signed alm.timezone value in procfs output.
>
>  drivers/rtc/rtc-efi.c | 80 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rtc/rtc-efi.c b/drivers/rtc/rtc-efi.c
> index b4f44999ef0f..5276870b410e 100644
> --- a/drivers/rtc/rtc-efi.c
> +++ b/drivers/rtc/rtc-efi.c
> @@ -52,6 +52,7 @@ compute_wday(efi_time_t *eft, int yday)
>  static void
>  convert_to_efi_time(struct rtc_time *wtime, efi_time_t *eft)
>  {
> +	memset(eft, 0, sizeof(*eft));
>  	eft->year	= wtime->tm_year + 1900;
>  	eft->month	= wtime->tm_mon + 1;
>  	eft->day	= wtime->tm_mday;
> @@ -112,6 +113,35 @@ convert_from_efi_time(efi_time_t *eft, struct 
> rtc_time *wtime)
>  	return true;
>  }
> 
> +static int efi_read_alarm(struct device *dev, struct rtc_wkalrm 
> *wkalrm)
> +{
> +	efi_time_t eft;
> +	efi_status_t status;
> +
> +	memset(&eft, 0, sizeof(eft));
> +	status = efi.get_wakeup_time((efi_bool_t *)&wkalrm->enabled,
> +				     (efi_bool_t *)&wkalrm->pending, &eft);
> +	if (status != EFI_SUCCESS)
> +		return -EINVAL;
> +
> +	if (!convert_from_efi_time(&eft, &wkalrm->time))
> +		return -EIO;
> +
> +	return rtc_valid_tm(&wkalrm->time);
> +}
> +
> +static int efi_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> +{
> +	efi_time_t eft;
> +	efi_status_t status;
> +
> +	convert_to_efi_time(&wkalrm->time, &eft);
> +
> +	status = efi.set_wakeup_time((efi_bool_t)!!wkalrm->enabled, &eft);
> +
> +	return status == EFI_SUCCESS ? 0 : -EINVAL;
> +}
> +
>  static int efi_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	efi_status_t status;
> @@ -146,13 +176,21 @@ static int efi_set_time(struct device *dev, 
> struct rtc_time *tm)
> 
>  static int efi_procfs(struct device *dev, struct seq_file *seq)
>  {
> -	efi_time_t        eft;
> +	efi_time_t        eft, alm;
>  	efi_time_cap_t    cap;
> +	efi_bool_t        enabled, pending;
> +	struct rtc_device *rtc = dev_get_drvdata(dev);
> 
>  	memset(&eft, 0, sizeof(eft));
> +	memset(&alm, 0, sizeof(alm));
>  	memset(&cap, 0, sizeof(cap));
> 
>  	efi.get_time(&eft, &cap);
> +	if (test_bit(RTC_FEATURE_ALARM, rtc->features) &&
> +	    efi.get_wakeup_time(&enabled, &pending, &alm) != EFI_SUCCESS) {
> +		enabled = false;
> +		pending = false;

Assign 0 here not false - efi_bool_t is a u8.