[PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm time in debugfs

[PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm time in debugfs

[Mateusz Jończyk]

Before Linux 5.17, there was a problem with the CMOS RTC driver:
cmos_read_alarm() and cmos_set_alarm() did not check for the UIP (Update
in progress) bit, which could have caused it to sometimes fail silently
and read bogus values or do not set the alarm correctly.
Luckily, this issue was masked by cmos_read_time() invocations in core
RTC code - see https://marc.info/?l=linux-rtc&m=164858416511425&w=4

To avoid such a problem in the future in some other driver, I wrote a
test unit that reads the alarm time many times in a row. As the alarm
time is usually read once and cached by the RTC core, this requires a
way for userspace to trigger direct alarm time read from hardware. I
think that debugfs is the natural choice for this.

So, introduce /sys/kernel/debug/rtc/rtcX/wakealarm_raw. This interface
as implemented here does not seem to be that useful to userspace, so
there is little risk that it will become kernel ABI.

Is this approach correct and worth it?

TODO:
- should I add a new Kconfig option (like CONFIG_RTC_INTF_DEBUGFS), or
  just use CONFIG_DEBUG_FS here? I wouldn't like to create unnecessary
  config options in the kernel.

Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Shuah Khan <shuah@kernel.org>
---
 drivers/rtc/Makefile    |   1 +
 drivers/rtc/class.c     |   3 ++
 drivers/rtc/debugfs.c   | 112 ++++++++++++++++++++++++++++++++++++++++
 drivers/rtc/interface.c |   3 +-
 include/linux/rtc.h     |  16 ++++++
 5 files changed, 133 insertions(+), 2 deletions(-)
 create mode 100644 drivers/rtc/debugfs.c

diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 678a8ef4abae..50e166a97f54 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -14,6 +14,7 @@ rtc-core-$(CONFIG_RTC_NVMEM)		+= nvmem.o
 rtc-core-$(CONFIG_RTC_INTF_DEV)		+= dev.o
 rtc-core-$(CONFIG_RTC_INTF_PROC)	+= proc.o
 rtc-core-$(CONFIG_RTC_INTF_SYSFS)	+= sysfs.o
+rtc-core-$(CONFIG_DEBUG_FS)		+= debugfs.o
 
 obj-$(CONFIG_RTC_LIB_KUNIT_TEST)	+= lib_test.o
 
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 4b460c61f1d8..5673b7b26c0d 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -334,6 +334,7 @@ static void devm_rtc_unregister_device(void *data)
 	 * Remove innards of this RTC, then disable it, before
 	 * letting any rtc_class_open() users access it again
 	 */
+	rtc_debugfs_del_device(rtc);
 	rtc_proc_del_device(rtc);
 	if (!test_bit(RTC_NO_CDEV, &rtc->flags))
 		cdev_device_del(&rtc->char_dev, &rtc->dev);
@@ -417,6 +418,7 @@ int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
 	}
 
 	rtc_proc_add_device(rtc);
+	rtc_debugfs_add_device(rtc);
 
 	dev_info(rtc->dev.parent, "registered as %s\n",
 		 dev_name(&rtc->dev));
@@ -476,6 +478,7 @@ static int __init rtc_init(void)
 	}
 	rtc_class->pm = RTC_CLASS_DEV_PM_OPS;
 	rtc_dev_init();
+	rtc_debugfs_init();
 	return 0;
 }
 subsys_initcall(rtc_init);
diff --git a/drivers/rtc/debugfs.c b/drivers/rtc/debugfs.c
new file mode 100644
index 000000000000..5ceed5504033
--- /dev/null
+++ b/drivers/rtc/debugfs.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Debugfs interface for testing RTC alarms.
+ */
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/rtc.h>
+
+static struct dentry *rtc_main_debugfs_dir;
+
+void rtc_debugfs_init(void)
+{
+	struct dentry *ret = debugfs_create_dir("rtc", NULL);
+
+	// No error is critical here
+	if (!IS_ERR(ret))
+		rtc_main_debugfs_dir = ret;
+}
+
+/*
+ * Handler for /sys/kernel/debug/rtc/rtcX/wakealarm_raw .
+ * This function reads the RTC alarm time directly from hardware. If the RTC
+ * alarm is enabled, this function returns the alarm time modulo 24h in seconds
+ * since midnight.
+ *
+ * Should be only used for testing of the RTC alarm read functionality in
+ * drivers - to make sure that the driver returns consistent values.
+ *
+ * Used in tools/testing/selftests/rtc/rtctest.c .
+ */
+static int rtc_debugfs_alarm_read(void *p, u64 *out)
+{
+	int ret;
+	struct rtc_device *rtc = p;
+	struct rtc_wkalrm alm;
+
+	/* Using rtc_read_alarm_internal() instead of __rtc_read_alarm() will
+	 * allow us to avoid any interaction with rtc_read_time() and possibly
+	 * see more issues.
+	 */
+	ret = rtc_read_alarm_internal(rtc, &alm);
+	if (ret != 0)
+		return ret;
+
+	if (!alm.enabled) {
+		*out = -1;
+		return 0;
+	}
+
+	/* It does not matter if the device does not support seconds resolution
+	 * of the RTC alarm.
+	 */
+	if (test_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->features))
+		alm.time.tm_sec = 0;
+
+	/* The selftest code works with fully defined alarms only.
+	 */
+	if (alm.time.tm_sec == -1 || alm.time.tm_min == -1 || alm.time.tm_hour == -1) {
+		*out = -2;
+		return 0;
+	}
+
+	/* Check if the alarm time is correct.
+	 * rtc_valid_tm() does not allow fields containing "-1", so put in
+	 * something to satisfy it.
+	 */
+	if (alm.time.tm_year == -1)
+		alm.time.tm_year = 100;
+	if (alm.time.tm_mon == -1)
+		alm.time.tm_mon = 0;
+	if (alm.time.tm_mday == -1)
+		alm.time.tm_mday = 1;
+	if (rtc_valid_tm(&alm.time))
+		return -EINVAL;
+
+	/* We do not duplicate the logic in __rtc_read_alarm() and instead only
+	 * return the alarm time modulo 24h, which all devices should support.
+	 * This should be enough for testing purposes.
+	 */
+	*out = alm.time.tm_hour * 3600 + alm.time.tm_min * 60 + alm.time.tm_sec;
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(rtc_alarm_raw, rtc_debugfs_alarm_read, NULL, "%lld\n");
+
+void rtc_debugfs_add_device(struct rtc_device *rtc)
+{
+	struct dentry *dev_dir;
+
+	if (!rtc_main_debugfs_dir)
+		return;
+
+	dev_dir = debugfs_create_dir(dev_name(&rtc->dev), rtc_main_debugfs_dir);
+
+	if (IS_ERR(dev_dir)) {
+		rtc->debugfs_dir = NULL;
+		return;
+	}
+	rtc->debugfs_dir = dev_dir;
+
+	if (test_bit(RTC_FEATURE_ALARM, rtc->features) && rtc->ops->read_alarm) {
+		debugfs_create_file("wakealarm_raw", 0444, dev_dir,
+				    rtc, &rtc_alarm_raw);
+	}
+}
+
+void rtc_debugfs_del_device(struct rtc_device *rtc)
+{
+	debugfs_remove_recursive(rtc->debugfs_dir);
+	rtc->debugfs_dir = NULL;
+}
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index d8e835798153..51c801c82472 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -175,8 +175,7 @@ int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
 }
 EXPORT_SYMBOL_GPL(rtc_set_time);
 
-static int rtc_read_alarm_internal(struct rtc_device *rtc,
-				   struct rtc_wkalrm *alarm)
+int rtc_read_alarm_internal(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
 {
 	int err;
 
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 47fd1c2d3a57..4665bc238a94 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -41,6 +41,7 @@ static inline time64_t rtc_tm_sub(struct rtc_time *lhs, struct rtc_time *rhs)
 #include <linux/mutex.h>
 #include <linux/timerqueue.h>
 #include <linux/workqueue.h>
+#include <linux/debugfs.h>
 
 extern struct class *rtc_class;
 
@@ -152,6 +153,10 @@ struct rtc_device {
 	time64_t offset_secs;
 	bool set_start_time;
 
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *debugfs_dir;
+#endif
+
 #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
 	struct work_struct uie_task;
 	struct timer_list uie_timer;
@@ -190,6 +195,7 @@ extern int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm);
 int __rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm);
 extern int rtc_read_alarm(struct rtc_device *rtc,
 			struct rtc_wkalrm *alrm);
+int rtc_read_alarm_internal(struct rtc_device *rtc, struct rtc_wkalrm *alarm);
 extern int rtc_set_alarm(struct rtc_device *rtc,
 				struct rtc_wkalrm *alrm);
 extern int rtc_initialize_alarm(struct rtc_device *rtc,
@@ -262,4 +268,14 @@ int rtc_add_groups(struct rtc_device *rtc, const struct attribute_group **grps)
 	return 0;
 }
 #endif
+
+#ifdef CONFIG_DEBUG_FS
+void rtc_debugfs_init(void);
+void rtc_debugfs_add_device(struct rtc_device *rtc);
+void rtc_debugfs_del_device(struct rtc_device *rtc);
+#else /* CONFIG_DEBUG_FS */
+static inline void rtc_debugfs_init(void) {}
+static inline void rtc_debugfs_add_device(struct rtc_device *rtc) {}
+static inline void rtc_debugfs_del_device(struct rtc_device *rtc) {}
+#endif /* CONFIG_DEBUG_FS */
 #endif /* _LINUX_RTC_H_ */
-- 
2.25.1

[PATCH 2/2] [RFC] selftests/rtc: read RTC alarm time many times in a row

[Mateusz Jończyk]

This is the userspace part of the test unit.

This patch depends on
commit 2aaa36e95ea5 ("selftests/rtc: continuously read RTC in a loop for 30s")

Existing code casts "struct rtc_time *" to "struct tm *", like so:

	gmtime_r(&secs, (struct tm *)&tm);

This is incorrect, because sizeof(struct tm) > sizeof(struct rtc_time)
(on Ubuntu 20.04 at least) and gmtime_r overwrites part of the stack.
I'll submit a fix for this in mainline soon, but for now I'm defining
timestamp_to_rtc_time() here.

TODO:
- some logic improvements, without much impact on the core algorithm.

Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Shuah Khan <shuah@kernel.org>
---
 tools/testing/selftests/rtc/rtctest.c | 81 ++++++++++++++++++++++++++-
 tools/testing/selftests/rtc/settings  |  2 +-
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
index 2b9d929a24ed..7e5f2f1c00ef 100644
--- a/tools/testing/selftests/rtc/rtctest.c
+++ b/tools/testing/selftests/rtc/rtctest.c
@@ -62,7 +62,21 @@ static time_t rtc_time_to_timestamp(struct rtc_time *rtc_time)
 	       .tm_year = rtc_time->tm_year,
 	};
 
-	return mktime(&tm_time);
+	return timegm(&tm_time);
+}
+
+static void timestamp_to_rtc_time(time_t timestamp, struct rtc_time *rtc_time)
+{
+	struct tm tm_time;
+
+	gmtime_r(&timestamp, &tm_time);
+
+	rtc_time->tm_sec = tm_time.tm_sec;
+	rtc_time->tm_min = tm_time.tm_min;
+	rtc_time->tm_hour = tm_time.tm_hour;
+	rtc_time->tm_mday = tm_time.tm_mday;
+	rtc_time->tm_mon = tm_time.tm_mon;
+	rtc_time->tm_year = tm_time.tm_year;
 }
 
 static void nanosleep_with_retries(long ns)
@@ -228,6 +242,71 @@ TEST_F(rtc, alarm_alm_set) {
 	ASSERT_EQ(new, secs);
 }
 
+TEST_F_TIMEOUT(rtc, alarm_read_loop, READ_LOOP_DURATION_SEC + 2) {
+	int rc;
+	long iter_count = 0;
+	struct rtc_time tm;
+	struct timeval start, curr;
+	time_t secs;
+
+	rc = ioctl(self->fd, RTC_RD_TIME, &tm);
+	ASSERT_NE(-1, rc);
+
+	// 60s is for clocks that only support minute resolution of RTC alarm
+	secs = rtc_time_to_timestamp(&tm) + READ_LOOP_DURATION_SEC + 60 + 2;
+	timestamp_to_rtc_time(secs, &tm);
+
+	rc = ioctl(self->fd, RTC_ALM_SET, &tm);
+	if (rc == -1) {
+		ASSERT_EQ(EINVAL, errno);
+		TH_LOG("skip alarms are not supported.");
+		return;
+	}
+
+	rc = ioctl(self->fd, RTC_AIE_ON, NULL);
+	ASSERT_NE(-1, rc);
+
+	TH_LOG("Continuously reading RTC alarm time for %ds (with %dms breaks after every read).",
+	       READ_LOOP_DURATION_SEC, READ_LOOP_SLEEP_MS);
+
+	gettimeofday(&start, NULL);
+
+	secs = 0;
+
+	do {
+		// TODO: use appropriate directory
+		// TODO: fail gracefully if the kernel support does not exist
+		FILE *wkalarm_file = fopen("/sys/kernel/debug/rtc/rtc0/wakealarm_raw", "r");
+		long long wkalarm_time;
+
+		ASSERT_NE(wkalarm_file, NULL);
+
+		rc = fscanf(wkalarm_file, "%lld", &wkalarm_time);
+		fclose(wkalarm_file);
+
+		ASSERT_EQ(rc, 1);
+		ASSERT_NE(secs, -1);
+		// TODO: use another value as placeholder,
+		// TODO: check if wkalarm_time matches alarm time we set
+		// previously
+		if (secs == 0)
+			secs = wkalarm_time;
+
+		ASSERT_EQ(wkalarm_time, secs);
+
+		// Sleep 11ms to avoid killing / overheating the RTC
+		nanosleep_with_retries(READ_LOOP_SLEEP_MS * 1000000);
+
+		gettimeofday(&curr, NULL);
+		iter_count++;
+	} while (curr.tv_sec <= start.tv_sec + READ_LOOP_DURATION_SEC);
+
+	TH_LOG("Performed %ld RTC alarm time reads.", iter_count);
+
+	rc = ioctl(self->fd, RTC_AIE_OFF, NULL);
+	ASSERT_NE(-1, rc);
+}
+
 TEST_F(rtc, alarm_wkalm_set) {
 	struct timeval tv = { .tv_sec = ALARM_DELTA + 2 };
 	struct rtc_wkalrm alarm = { 0 };
diff --git a/tools/testing/selftests/rtc/settings b/tools/testing/selftests/rtc/settings
index 0c1a2075d5f3..b478e684846a 100644
--- a/tools/testing/selftests/rtc/settings
+++ b/tools/testing/selftests/rtc/settings
@@ -1 +1 @@
-timeout=210
+timeout=240
-- 
2.25.1

Re: [PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm time in debugfs

[Greg KH]

On Thu, Mar 31, 2022 at 09:06:11PM +0200, Mateusz Jończyk wrote:
> Before Linux 5.17, there was a problem with the CMOS RTC driver:
> cmos_read_alarm() and cmos_set_alarm() did not check for the UIP (Update
> in progress) bit, which could have caused it to sometimes fail silently
> and read bogus values or do not set the alarm correctly.
> Luckily, this issue was masked by cmos_read_time() invocations in core
> RTC code - see https://marc.info/?l=linux-rtc&m=164858416511425&w=4
> 
> To avoid such a problem in the future in some other driver, I wrote a
> test unit that reads the alarm time many times in a row. As the alarm
> time is usually read once and cached by the RTC core, this requires a
> way for userspace to trigger direct alarm time read from hardware. I
> think that debugfs is the natural choice for this.
> 
> So, introduce /sys/kernel/debug/rtc/rtcX/wakealarm_raw. This interface
> as implemented here does not seem to be that useful to userspace, so
> there is little risk that it will become kernel ABI.
> 
> Is this approach correct and worth it?
> 
> TODO:
> - should I add a new Kconfig option (like CONFIG_RTC_INTF_DEBUGFS), or
>   just use CONFIG_DEBUG_FS here? I wouldn't like to create unnecessary
>   config options in the kernel.
> 
> Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
>  drivers/rtc/Makefile    |   1 +
>  drivers/rtc/class.c     |   3 ++
>  drivers/rtc/debugfs.c   | 112 ++++++++++++++++++++++++++++++++++++++++
>  drivers/rtc/interface.c |   3 +-
>  include/linux/rtc.h     |  16 ++++++
>  5 files changed, 133 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/rtc/debugfs.c
> 
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 678a8ef4abae..50e166a97f54 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -14,6 +14,7 @@ rtc-core-$(CONFIG_RTC_NVMEM)		+= nvmem.o
>  rtc-core-$(CONFIG_RTC_INTF_DEV)		+= dev.o
>  rtc-core-$(CONFIG_RTC_INTF_PROC)	+= proc.o
>  rtc-core-$(CONFIG_RTC_INTF_SYSFS)	+= sysfs.o
> +rtc-core-$(CONFIG_DEBUG_FS)		+= debugfs.o
>  
>  obj-$(CONFIG_RTC_LIB_KUNIT_TEST)	+= lib_test.o
>  
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 4b460c61f1d8..5673b7b26c0d 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -334,6 +334,7 @@ static void devm_rtc_unregister_device(void *data)
>  	 * Remove innards of this RTC, then disable it, before
>  	 * letting any rtc_class_open() users access it again
>  	 */
> +	rtc_debugfs_del_device(rtc);
>  	rtc_proc_del_device(rtc);
>  	if (!test_bit(RTC_NO_CDEV, &rtc->flags))
>  		cdev_device_del(&rtc->char_dev, &rtc->dev);
> @@ -417,6 +418,7 @@ int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
>  	}
>  
>  	rtc_proc_add_device(rtc);
> +	rtc_debugfs_add_device(rtc);
>  
>  	dev_info(rtc->dev.parent, "registered as %s\n",
>  		 dev_name(&rtc->dev));
> @@ -476,6 +478,7 @@ static int __init rtc_init(void)
>  	}
>  	rtc_class->pm = RTC_CLASS_DEV_PM_OPS;
>  	rtc_dev_init();
> +	rtc_debugfs_init();
>  	return 0;
>  }
>  subsys_initcall(rtc_init);
> diff --git a/drivers/rtc/debugfs.c b/drivers/rtc/debugfs.c
> new file mode 100644
> index 000000000000..5ceed5504033
> --- /dev/null
> +++ b/drivers/rtc/debugfs.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Debugfs interface for testing RTC alarms.
> + */
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +#include <linux/rtc.h>
> +
> +static struct dentry *rtc_main_debugfs_dir;
> +
> +void rtc_debugfs_init(void)
> +{
> +	struct dentry *ret = debugfs_create_dir("rtc", NULL);
> +
> +	// No error is critical here

What do you mean by this?

> +	if (!IS_ERR(ret))
> +		rtc_main_debugfs_dir = ret;

This should not be needed.

Just set the directory and away you go.

But why even save it?  If you need it you can always look it up, right?

And when are you removing this directory you created?

> +}
> +
> +/*
> + * Handler for /sys/kernel/debug/rtc/rtcX/wakealarm_raw .
> + * This function reads the RTC alarm time directly from hardware. If the RTC
> + * alarm is enabled, this function returns the alarm time modulo 24h in seconds
> + * since midnight.
> + *
> + * Should be only used for testing of the RTC alarm read functionality in
> + * drivers - to make sure that the driver returns consistent values.
> + *
> + * Used in tools/testing/selftests/rtc/rtctest.c .
> + */
> +static int rtc_debugfs_alarm_read(void *p, u64 *out)
> +{
> +	int ret;
> +	struct rtc_device *rtc = p;
> +	struct rtc_wkalrm alm;
> +
> +	/* Using rtc_read_alarm_internal() instead of __rtc_read_alarm() will
> +	 * allow us to avoid any interaction with rtc_read_time() and possibly
> +	 * see more issues.
> +	 */
> +	ret = rtc_read_alarm_internal(rtc, &alm);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (!alm.enabled) {
> +		*out = -1;
> +		return 0;
> +	}
> +
> +	/* It does not matter if the device does not support seconds resolution
> +	 * of the RTC alarm.
> +	 */
> +	if (test_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->features))
> +		alm.time.tm_sec = 0;
> +
> +	/* The selftest code works with fully defined alarms only.
> +	 */
> +	if (alm.time.tm_sec == -1 || alm.time.tm_min == -1 || alm.time.tm_hour == -1) {
> +		*out = -2;
> +		return 0;
> +	}
> +
> +	/* Check if the alarm time is correct.
> +	 * rtc_valid_tm() does not allow fields containing "-1", so put in
> +	 * something to satisfy it.
> +	 */
> +	if (alm.time.tm_year == -1)
> +		alm.time.tm_year = 100;
> +	if (alm.time.tm_mon == -1)
> +		alm.time.tm_mon = 0;
> +	if (alm.time.tm_mday == -1)
> +		alm.time.tm_mday = 1;
> +	if (rtc_valid_tm(&alm.time))
> +		return -EINVAL;
> +
> +	/* We do not duplicate the logic in __rtc_read_alarm() and instead only
> +	 * return the alarm time modulo 24h, which all devices should support.
> +	 * This should be enough for testing purposes.
> +	 */
> +	*out = alm.time.tm_hour * 3600 + alm.time.tm_min * 60 + alm.time.tm_sec;
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(rtc_alarm_raw, rtc_debugfs_alarm_read, NULL, "%lld\n");
> +
> +void rtc_debugfs_add_device(struct rtc_device *rtc)
> +{
> +	struct dentry *dev_dir;
> +
> +	if (!rtc_main_debugfs_dir)
> +		return;
> +
> +	dev_dir = debugfs_create_dir(dev_name(&rtc->dev), rtc_main_debugfs_dir);
> +
> +	if (IS_ERR(dev_dir)) {

Again, no need to check this.  You never need to test the return value
of a call to debugfs_*.

> +		rtc->debugfs_dir = NULL;
> +		return;
> +	}
> +	rtc->debugfs_dir = dev_dir;
> +
> +	if (test_bit(RTC_FEATURE_ALARM, rtc->features) && rtc->ops->read_alarm) {
> +		debugfs_create_file("wakealarm_raw", 0444, dev_dir,
> +				    rtc, &rtc_alarm_raw);
> +	}
> +}
> +
> +void rtc_debugfs_del_device(struct rtc_device *rtc)
> +{
> +	debugfs_remove_recursive(rtc->debugfs_dir);
> +	rtc->debugfs_dir = NULL;
> +}
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index d8e835798153..51c801c82472 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -175,8 +175,7 @@ int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>  }
>  EXPORT_SYMBOL_GPL(rtc_set_time);
>  
> -static int rtc_read_alarm_internal(struct rtc_device *rtc,
> -				   struct rtc_wkalrm *alarm)
> +int rtc_read_alarm_internal(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
>  {
>  	int err;
>  
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 47fd1c2d3a57..4665bc238a94 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -41,6 +41,7 @@ static inline time64_t rtc_tm_sub(struct rtc_time *lhs, struct rtc_time *rhs)
>  #include <linux/mutex.h>
>  #include <linux/timerqueue.h>
>  #include <linux/workqueue.h>
> +#include <linux/debugfs.h>
>  
>  extern struct class *rtc_class;
>  
> @@ -152,6 +153,10 @@ struct rtc_device {
>  	time64_t offset_secs;
>  	bool set_start_time;
>  
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *debugfs_dir;

No need to save this.  Just look it up if you need it.

thanks,

greg k-h

Re: [PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm time in debugfs

[Alexandre Belloni]

Hello,

On 31/03/2022 21:06:11+0200, Mateusz Jończyk wrote:
> Before Linux 5.17, there was a problem with the CMOS RTC driver:
> cmos_read_alarm() and cmos_set_alarm() did not check for the UIP (Update
> in progress) bit, which could have caused it to sometimes fail silently
> and read bogus values or do not set the alarm correctly.
> Luckily, this issue was masked by cmos_read_time() invocations in core
> RTC code - see https://marc.info/?l=linux-rtc&m=164858416511425&w=4
> 
> To avoid such a problem in the future in some other driver, I wrote a
> test unit that reads the alarm time many times in a row. As the alarm
> time is usually read once and cached by the RTC core, this requires a
> way for userspace to trigger direct alarm time read from hardware. I
> think that debugfs is the natural choice for this.
> 
> So, introduce /sys/kernel/debug/rtc/rtcX/wakealarm_raw. This interface
> as implemented here does not seem to be that useful to userspace, so
> there is little risk that it will become kernel ABI.
> 
> Is this approach correct and worth it?
> 

I'm not really in favor of adding another interface for very little
gain, you want to use this interface to exercise the API in a way that
will never happen in the real world, especially since __rtc_read_alarm
is only called once, at registration time.

I'm not sure the selftest is worth it then. You should better improve
the existing unit tests by exercising the ioctls a bit more. syzbot did
report interesting race conditions that were more severe.

> TODO:
> - should I add a new Kconfig option (like CONFIG_RTC_INTF_DEBUGFS), or
>   just use CONFIG_DEBUG_FS here? I wouldn't like to create unnecessary
>   config options in the kernel.
> 
> Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
>  drivers/rtc/Makefile    |   1 +
>  drivers/rtc/class.c     |   3 ++
>  drivers/rtc/debugfs.c   | 112 ++++++++++++++++++++++++++++++++++++++++
>  drivers/rtc/interface.c |   3 +-
>  include/linux/rtc.h     |  16 ++++++
>  5 files changed, 133 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/rtc/debugfs.c
> 
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 678a8ef4abae..50e166a97f54 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -14,6 +14,7 @@ rtc-core-$(CONFIG_RTC_NVMEM)		+= nvmem.o
>  rtc-core-$(CONFIG_RTC_INTF_DEV)		+= dev.o
>  rtc-core-$(CONFIG_RTC_INTF_PROC)	+= proc.o
>  rtc-core-$(CONFIG_RTC_INTF_SYSFS)	+= sysfs.o
> +rtc-core-$(CONFIG_DEBUG_FS)		+= debugfs.o
>  
>  obj-$(CONFIG_RTC_LIB_KUNIT_TEST)	+= lib_test.o
>  
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 4b460c61f1d8..5673b7b26c0d 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -334,6 +334,7 @@ static void devm_rtc_unregister_device(void *data)
>  	 * Remove innards of this RTC, then disable it, before
>  	 * letting any rtc_class_open() users access it again
>  	 */
> +	rtc_debugfs_del_device(rtc);
>  	rtc_proc_del_device(rtc);
>  	if (!test_bit(RTC_NO_CDEV, &rtc->flags))
>  		cdev_device_del(&rtc->char_dev, &rtc->dev);
> @@ -417,6 +418,7 @@ int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
>  	}
>  
>  	rtc_proc_add_device(rtc);
> +	rtc_debugfs_add_device(rtc);
>  
>  	dev_info(rtc->dev.parent, "registered as %s\n",
>  		 dev_name(&rtc->dev));
> @@ -476,6 +478,7 @@ static int __init rtc_init(void)
>  	}
>  	rtc_class->pm = RTC_CLASS_DEV_PM_OPS;
>  	rtc_dev_init();
> +	rtc_debugfs_init();
>  	return 0;
>  }
>  subsys_initcall(rtc_init);
> diff --git a/drivers/rtc/debugfs.c b/drivers/rtc/debugfs.c
> new file mode 100644
> index 000000000000..5ceed5504033
> --- /dev/null
> +++ b/drivers/rtc/debugfs.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Debugfs interface for testing RTC alarms.
> + */
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +#include <linux/rtc.h>
> +
> +static struct dentry *rtc_main_debugfs_dir;
> +
> +void rtc_debugfs_init(void)
> +{
> +	struct dentry *ret = debugfs_create_dir("rtc", NULL);
> +
> +	// No error is critical here
> +	if (!IS_ERR(ret))
> +		rtc_main_debugfs_dir = ret;
> +}
> +
> +/*
> + * Handler for /sys/kernel/debug/rtc/rtcX/wakealarm_raw .
> + * This function reads the RTC alarm time directly from hardware. If the RTC
> + * alarm is enabled, this function returns the alarm time modulo 24h in seconds
> + * since midnight.
> + *
> + * Should be only used for testing of the RTC alarm read functionality in
> + * drivers - to make sure that the driver returns consistent values.
> + *
> + * Used in tools/testing/selftests/rtc/rtctest.c .
> + */
> +static int rtc_debugfs_alarm_read(void *p, u64 *out)
> +{
> +	int ret;
> +	struct rtc_device *rtc = p;
> +	struct rtc_wkalrm alm;
> +
> +	/* Using rtc_read_alarm_internal() instead of __rtc_read_alarm() will
> +	 * allow us to avoid any interaction with rtc_read_time() and possibly
> +	 * see more issues.
> +	 */
> +	ret = rtc_read_alarm_internal(rtc, &alm);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (!alm.enabled) {
> +		*out = -1;
> +		return 0;
> +	}
> +
> +	/* It does not matter if the device does not support seconds resolution
> +	 * of the RTC alarm.
> +	 */
> +	if (test_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->features))
> +		alm.time.tm_sec = 0;
> +
> +	/* The selftest code works with fully defined alarms only.
> +	 */
> +	if (alm.time.tm_sec == -1 || alm.time.tm_min == -1 || alm.time.tm_hour == -1) {
> +		*out = -2;
> +		return 0;
> +	}
> +
> +	/* Check if the alarm time is correct.
> +	 * rtc_valid_tm() does not allow fields containing "-1", so put in
> +	 * something to satisfy it.
> +	 */
> +	if (alm.time.tm_year == -1)
> +		alm.time.tm_year = 100;
> +	if (alm.time.tm_mon == -1)
> +		alm.time.tm_mon = 0;
> +	if (alm.time.tm_mday == -1)
> +		alm.time.tm_mday = 1;
> +	if (rtc_valid_tm(&alm.time))
> +		return -EINVAL;
> +
> +	/* We do not duplicate the logic in __rtc_read_alarm() and instead only
> +	 * return the alarm time modulo 24h, which all devices should support.
> +	 * This should be enough for testing purposes.
> +	 */
> +	*out = alm.time.tm_hour * 3600 + alm.time.tm_min * 60 + alm.time.tm_sec;
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(rtc_alarm_raw, rtc_debugfs_alarm_read, NULL, "%lld\n");
> +
> +void rtc_debugfs_add_device(struct rtc_device *rtc)
> +{
> +	struct dentry *dev_dir;
> +
> +	if (!rtc_main_debugfs_dir)
> +		return;
> +
> +	dev_dir = debugfs_create_dir(dev_name(&rtc->dev), rtc_main_debugfs_dir);
> +
> +	if (IS_ERR(dev_dir)) {
> +		rtc->debugfs_dir = NULL;
> +		return;
> +	}
> +	rtc->debugfs_dir = dev_dir;
> +
> +	if (test_bit(RTC_FEATURE_ALARM, rtc->features) && rtc->ops->read_alarm) {
> +		debugfs_create_file("wakealarm_raw", 0444, dev_dir,
> +				    rtc, &rtc_alarm_raw);
> +	}
> +}
> +
> +void rtc_debugfs_del_device(struct rtc_device *rtc)
> +{
> +	debugfs_remove_recursive(rtc->debugfs_dir);
> +	rtc->debugfs_dir = NULL;
> +}
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index d8e835798153..51c801c82472 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -175,8 +175,7 @@ int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>  }
>  EXPORT_SYMBOL_GPL(rtc_set_time);
>  
> -static int rtc_read_alarm_internal(struct rtc_device *rtc,
> -				   struct rtc_wkalrm *alarm)
> +int rtc_read_alarm_internal(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
>  {
>  	int err;
>  
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 47fd1c2d3a57..4665bc238a94 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -41,6 +41,7 @@ static inline time64_t rtc_tm_sub(struct rtc_time *lhs, struct rtc_time *rhs)
>  #include <linux/mutex.h>
>  #include <linux/timerqueue.h>
>  #include <linux/workqueue.h>
> +#include <linux/debugfs.h>
>  
>  extern struct class *rtc_class;
>  
> @@ -152,6 +153,10 @@ struct rtc_device {
>  	time64_t offset_secs;
>  	bool set_start_time;
>  
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *debugfs_dir;
> +#endif
> +
>  #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL
>  	struct work_struct uie_task;
>  	struct timer_list uie_timer;
> @@ -190,6 +195,7 @@ extern int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm);
>  int __rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm);
>  extern int rtc_read_alarm(struct rtc_device *rtc,
>  			struct rtc_wkalrm *alrm);
> +int rtc_read_alarm_internal(struct rtc_device *rtc, struct rtc_wkalrm *alarm);
>  extern int rtc_set_alarm(struct rtc_device *rtc,
>  				struct rtc_wkalrm *alrm);
>  extern int rtc_initialize_alarm(struct rtc_device *rtc,
> @@ -262,4 +268,14 @@ int rtc_add_groups(struct rtc_device *rtc, const struct attribute_group **grps)
>  	return 0;
>  }
>  #endif
> +
> +#ifdef CONFIG_DEBUG_FS
> +void rtc_debugfs_init(void);
> +void rtc_debugfs_add_device(struct rtc_device *rtc);
> +void rtc_debugfs_del_device(struct rtc_device *rtc);
> +#else /* CONFIG_DEBUG_FS */
> +static inline void rtc_debugfs_init(void) {}
> +static inline void rtc_debugfs_add_device(struct rtc_device *rtc) {}
> +static inline void rtc_debugfs_del_device(struct rtc_device *rtc) {}
> +#endif /* CONFIG_DEBUG_FS */
>  #endif /* _LINUX_RTC_H_ */
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm time in debugfs

[Mateusz Jończyk]

W dniu 31.03.2022 o 21:21, Greg KH pisze:
> On Thu, Mar 31, 2022 at 09:06:11PM +0200, Mateusz Jończyk wrote:
>> Before Linux 5.17, there was a problem with the CMOS RTC driver:
>> cmos_read_alarm() and cmos_set_alarm() did not check for the UIP (Update
>> in progress) bit, which could have caused it to sometimes fail silently
>> and read bogus values or do not set the alarm correctly.
>> Luckily, this issue was masked by cmos_read_time() invocations in core
>> RTC code - see https://marc.info/?l=linux-rtc&m=164858416511425&w=4
[snip]
>> diff --git a/drivers/rtc/debugfs.c b/drivers/rtc/debugfs.c
>> new file mode 100644
>> index 000000000000..5ceed5504033
>> --- /dev/null
>> +++ b/drivers/rtc/debugfs.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +/*
>> + * Debugfs interface for testing RTC alarms.
>> + */
>> +#include <linux/debugfs.h>
>> +#include <linux/err.h>
>> +#include <linux/rtc.h>
>> +
>> +static struct dentry *rtc_main_debugfs_dir;
>> +
>> +void rtc_debugfs_init(void)
>> +{
>> +	struct dentry *ret = debugfs_create_dir("rtc", NULL);
>> +
>> +	// No error is critical here
> What do you mean by this?

I meant that even if debugfs_create_dir() fails, this does not matter here and the
caller of rtc_debugfs_init() may continue execution.

>> +	if (!IS_ERR(ret))
>> +		rtc_main_debugfs_dir = ret;
> This should not be needed.
>
> Just set the directory and away you go.
>
> But why even save it?  If you need it you can always look it up, right?
OK, I did not know about easy lookup of debugfs directories.
> And when are you removing this directory you created?
Never, as the RTC subsystem cannot be compiled as a module.
This is the main debugfs directory of RTC subsystem, directories for specific devices
(rtc0, etc.) are created below it.
Please correct me if I'm wrong, but I thought that empty debugfs directories do not hurt.


Thanks for reviewing,

Mateusz

Re: [PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm time in debugfs

[Mateusz Jończyk]

W dniu 31.03.2022 o 21:36, Alexandre Belloni pisze:
> Hello,
>
> On 31/03/2022 21:06:11+0200, Mateusz Jończyk wrote:
>> Before Linux 5.17, there was a problem with the CMOS RTC driver:
>> cmos_read_alarm() and cmos_set_alarm() did not check for the UIP (Update
>> in progress) bit, which could have caused it to sometimes fail silently
>> and read bogus values or do not set the alarm correctly.
>> Luckily, this issue was masked by cmos_read_time() invocations in core
>> RTC code - see https://marc.info/?l=linux-rtc&m=164858416511425&w=4
>>
>> To avoid such a problem in the future in some other driver, I wrote a
>> test unit that reads the alarm time many times in a row. As the alarm
>> time is usually read once and cached by the RTC core, this requires a
>> way for userspace to trigger direct alarm time read from hardware. I
>> think that debugfs is the natural choice for this.
>>
>> So, introduce /sys/kernel/debug/rtc/rtcX/wakealarm_raw. This interface
>> as implemented here does not seem to be that useful to userspace, so
>> there is little risk that it will become kernel ABI.
>>
>> Is this approach correct and worth it?
>>
> I'm not really in favor of adding another interface for very little
> gain, you want to use this interface to exercise the API in a way that
> will never happen in the real world, especially since __rtc_read_alarm
> is only called once, at registration time.
>
> I'm not sure the selftest is worth it then. You should better improve
> the existing unit tests by exercising the ioctls a bit more. syzbot did
> report interesting race conditions that were more severe.

OK, I did not know if other RTC drivers are likely to suffer from this kind of bugs.
I also thought that the bugs in cmos_read_alarm() / cmos_set_alarm() were more severe and
likely to affect existing users.

I had doubts if it's worth it, so I didn't finish the patches and sent it as RFC. It was a nice project, though.

Would you point to these race conditions reported by syzbot? I cannot find them.

Greetings,

Mateusz

Re: [PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm time in debugfs

[Alexandre Belloni]

On 31/03/2022 21:52:09+0200, Mateusz Jończyk wrote:
> W dniu 31.03.2022 o 21:36, Alexandre Belloni pisze:
> > Hello,
> >
> > On 31/03/2022 21:06:11+0200, Mateusz Jończyk wrote:
> >> Before Linux 5.17, there was a problem with the CMOS RTC driver:
> >> cmos_read_alarm() and cmos_set_alarm() did not check for the UIP (Update
> >> in progress) bit, which could have caused it to sometimes fail silently
> >> and read bogus values or do not set the alarm correctly.
> >> Luckily, this issue was masked by cmos_read_time() invocations in core
> >> RTC code - see https://marc.info/?l=linux-rtc&m=164858416511425&w=4
> >>
> >> To avoid such a problem in the future in some other driver, I wrote a
> >> test unit that reads the alarm time many times in a row. As the alarm
> >> time is usually read once and cached by the RTC core, this requires a
> >> way for userspace to trigger direct alarm time read from hardware. I
> >> think that debugfs is the natural choice for this.
> >>
> >> So, introduce /sys/kernel/debug/rtc/rtcX/wakealarm_raw. This interface
> >> as implemented here does not seem to be that useful to userspace, so
> >> there is little risk that it will become kernel ABI.
> >>
> >> Is this approach correct and worth it?
> >>
> > I'm not really in favor of adding another interface for very little
> > gain, you want to use this interface to exercise the API in a way that
> > will never happen in the real world, especially since __rtc_read_alarm
> > is only called once, at registration time.
> >
> > I'm not sure the selftest is worth it then. You should better improve
> > the existing unit tests by exercising the ioctls a bit more. syzbot did
> > report interesting race conditions that were more severe.
> 
> OK, I did not know if other RTC drivers are likely to suffer from this kind of bugs.
> I also thought that the bugs in cmos_read_alarm() / cmos_set_alarm() were more severe and
> likely to affect existing users.
> 
> I had doubts if it's worth it, so I didn't finish the patches and sent it as RFC. It was a nice project, though.
> 

Really, it is nice to see someone wanting to improve testing but I
really believe that we would benefit more from unit tests for the
actually userspace API.

> Would you point to these race conditions reported by syzbot? I cannot find them.
> 

It was that one:
https://groups.google.com/g/syzkaller-bugs/c/K1FV5LBwSgM/m/hhC_DciwBAAJ?pli=1

> Greetings,
> 
> Mateusz
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com