Re: [RFC PATCH 09/14] treewide: disable other rtc's in t2 macs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rtc: acpi_tad_probe() and cmos_do_probe() miss CONFIG_MACSMC_ACPI check
- [High] rtc: cmos_do_probe() leaves x86 CMOS wallclock callbacks active on T2 Macs
--

commit 8f32ab6b5995f6b8ccbfee083b09d400a56632a7
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

treewide: disable other rtc's in t2 macs

This commit disables the ACPI TAD real-time clock capability and aborts
probing the CMOS RTC driver on Apple T2 Macs when CONFIG_RTC_DRV_MACSMC is
enabled, leaving macsmc-rtc as the active RTC.

> diff --git a/drivers/acpi/acpi_tad.c b/drivers/acpi/acpi_tad.c
> index 45459a4cafae3..a137a39e5e67f 100644
> --- a/drivers/acpi/acpi_tad.c
> +++ b/drivers/acpi/acpi_tad.c
[ ... ]
> @@ -845,6 +846,9 @@ static int acpi_tad_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_RTC_DRV_MACSMC) && has_apple_t2_chip)
> +		caps &= ~ACPI_TAD_RT;

[Severity: High]
Could checking only IS_ENABLED(CONFIG_RTC_DRV_MACSMC) in acpi_tad_probe()
and cmos_do_probe() leave T2 Macs without any registered RTC device when
CONFIG_MACSMC_ACPI is disabled?

In drivers/rtc/Kconfig, RTC_DRV_MACSMC only depends on MFD_MACSMC_CORE:

config RTC_DRV_MACSMC
	tristate "Apple Mac System Management Controller RTC"
	depends on MFD_MACSMC_CORE

However, on x86 T2 Macs, the macsmc-rtc MFD child device is only registered
via apple_smc_acpi_probe() -> apple_smc_probe(), which requires
CONFIG_MACSMC_ACPI in drivers/platform/x86/Kconfig:

config MACSMC_ACPI
	tristate "Apple SMC (System Management Controller) ACPI Backend"
	depends on MFD_MACSMC_CORE && ACPI && !SENSORS_APPLESMC

When CONFIG_RTC_DRV_MACSMC is enabled alongside CONFIG_SENSORS_APPLESMC (or
when CONFIG_MACSMC_ACPI is otherwise disabled), macsmc-rtc is never
instantiated, yet acpi_tad_probe() and cmos_do_probe() will still disable
both the ACPI TAD RTC and the CMOS RTC.

Should both checks also verify IS_ENABLED(CONFIG_MACSMC_ACPI)?

> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index fa04ece151b8b..bdb436e24bb82 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
[ ... ]
> @@ -944,6 +945,9 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
>  		.priv = &cmos_rtc,
>  	};
>  
> +	if (IS_ENABLED(CONFIG_RTC_DRV_MACSMC) && has_apple_t2_chip)
> +		return -ENODEV;

[Severity: High]
When cmos_do_probe() returns -ENODEV here, what happens to the x86
architectural wallclock callbacks in x86_platform?

In arch/x86/kernel/x86_init.c, x86_platform defaults to the legacy CMOS
routines:

	.get_wallclock			= mach_get_cmos_time,
	.set_wallclock			= mach_set_cmos_time,

These are invoked by read_persistent_clock64() and
update_persistent_clock64() in arch/x86/kernel/rtc.c, which call
mc146818_get_time() and mc146818_set_time() directly on CMOS I/O ports
0x70/0x71 regardless of whether the rtc-cmos driver probed.

During NTP synchronization in kernel/time/ntp.c, sync_hw_clock() tries the
persistent clock before falling back to the RTC class device:

	/* Try the legacy RTC first. */
	res = update_persistent_clock64(to_set);
	if (res != -ENODEV)
		goto rearm;

	/* Try the RTC class */
	res = update_rtc(&to_set, &offset_nsec);

Because mach_set_cmos_time() -> mc146818_set_time() writes to the CMOS ports
and returns 0 (or -EINVAL, never -ENODEV), sync_hw_clock() will take the
goto rearm path and never call update_rtc() to update macsmc-rtc.

Similarly, timekeeping_init(), timekeeping_suspend(), and
timekeeping_resume() call read_persistent_clock64(), which will still read
the non-working CMOS clock via mach_get_cmos_time(). If that returns a
non-zero timestamp, persistent_clock_exists is set to true and
timekeeping_rtc_skipsuspend() causes rtc_suspend() to skip macsmc-rtc across
suspend and resume.

Should x86_platform.get_wallclock and x86_platform.set_wallclock also be
redirected to x86_init_noop / x86_wallclock_noop on T2 Macs when macsmc-rtc
is used?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=9