Message-ID: <YkYCqI0HEJvi2VjV@piout.net>
Date:   Thu, 31 Mar 2022 21:36:08 +0200
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Mateusz Jończyk <mat.jonczyk@...pl>
Cc:     linux-kernel@...r.kernel.org, linux-rtc@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH 1/2] [RFC] rtc: expose direct access to hardware alarm
 time in debugfs

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@...pl>
> Cc: Alessandro Zummo <a.zummo@...ertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>
> Cc: Shuah Khan <shuah@...nel.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

Powered by blists - more mailing lists