[RFC][PATCH 1/6] RTC subsystem, class

[RFC][PATCH 1/6] RTC subsystem, class

[Alessandro Zummo]

This patch adds the basic RTC subsytem infrastructure
to the kernel.

rtc/class.c - registration facilities for RTC drivers
rtc/intf.c - kernel/rtc interface functions 
rtc/utils.c - misc rtc-related utility functions

Signed-off-by: Alessandro Zummo <a.zummo@towertech.it>
--
 drivers/Kconfig      |    2 
 drivers/Makefile     |    2 
 drivers/rtc/Kconfig  |   25 +++++++++++
 drivers/rtc/Makefile |    8 +++
 drivers/rtc/class.c  |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/rtc/intf.c   |   67 +++++++++++++++++++++++++++++++
 drivers/rtc/utils.c  |   99 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/rtc.h  |   30 +++++++++++++
 8 files changed, 343 insertions(+)

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-nslu2/drivers/rtc/class.c	2005-12-15 10:22:20.000000000 +0100
@@ -0,0 +1,110 @@
+/*
+ * rtc-class.c - rtc subsystem, base class
+ *
+ * Copyright (C) 2005 Tower Technologies
+ * Author: Alessandro Zummo <a.zummo@towertech.it>
+ *
+ * class skeleton from drivers/hwmon/hwmon.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+*/
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kdev_t.h>
+#include <linux/idr.h>
+#include <linux/rtc.h>
+
+#define RTC_ID_PREFIX "rtc"
+#define RTC_ID_FORMAT RTC_ID_PREFIX "%d"
+
+static struct class *rtc_class;
+
+static DEFINE_IDR(rtc_idr);
+
+/**
+ * rtc_device_register - register w/ hwmon sysfs class
+ * @dev: the device to register
+ *
+ * rtc_device_unregister() must be called when the class device is no
+ * longer needed.
+ *
+ * Returns the pointer to the new struct class device.
+ */
+struct class_device *rtc_device_register(struct device *dev,
+					struct rtc_class_ops *ops)
+{
+	struct class_device *cdev;
+	int id;
+
+	if (idr_pre_get(&rtc_idr, GFP_KERNEL) == 0)
+		return ERR_PTR(-ENOMEM);
+
+	if (idr_get_new(&rtc_idr, NULL, &id) < 0)
+		return ERR_PTR(-ENOMEM);
+
+	id = id & MAX_ID_MASK;
+	cdev = class_device_create(rtc_class, NULL, MKDEV(0,0), dev,
+					RTC_ID_FORMAT, id);
+
+	if (IS_ERR(cdev))
+		idr_remove(&rtc_idr, id);
+	else
+		dev_info(dev, "rtc core: registered\n");
+
+	class_set_devdata(cdev, ops);
+
+	return cdev;
+}
+
+/**
+ * rtc_device_unregister - removes the previously registered class device
+ *
+ * @cdev: the class device to destroy
+ */
+void rtc_device_unregister(struct class_device *cdev)
+{
+	int id;
+
+	if (sscanf(cdev->class_id, RTC_ID_FORMAT, &id) == 1) {
+		class_device_unregister(cdev);
+		idr_remove(&rtc_idr, id);
+	} else
+		dev_dbg(cdev->dev,
+			"rtc_device_unregister() failed: bad class ID!\n");
+}
+
+int rtc_interface_register(struct class_interface *intf)
+{
+	intf->class = rtc_class;
+        return class_interface_register(intf);
+}
+EXPORT_SYMBOL(rtc_interface_register);
+
+static int __init rtc_init(void)
+{
+	rtc_class = class_create(THIS_MODULE, "rtc");
+	if (IS_ERR(rtc_class)) {
+		printk(KERN_ERR "rtc-class.c: couldn't create class\n");
+		return PTR_ERR(rtc_class);
+	}
+	return 0;
+}
+
+static void __exit rtc_exit(void)
+{
+	class_destroy(rtc_class);
+}
+
+module_init(rtc_init);
+module_exit(rtc_exit);
+
+EXPORT_SYMBOL_GPL(rtc_device_register);
+EXPORT_SYMBOL_GPL(rtc_device_unregister);
+
+MODULE_AUTHOR("Alessandro Zummo <a.zummo@towerteh.it>");
+MODULE_DESCRIPTION("RTC class support");
+MODULE_LICENSE("GPL");
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-nslu2/drivers/rtc/intf.c	2005-12-15 09:28:14.000000000 +0100
@@ -0,0 +1,67 @@
+/*
+ * rtc-intf.c - rtc subsystem, interface functions
+ *
+ * Copyright (C) 2005 Tower Technologies
+ * Author: Alessandro Zummo <a.zummo@towertech.it>
+ *
+ * based on arch/arm/common/rtctime.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+*/
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/rtc.h>
+
+int rtc_read_time(struct class_device *class_dev, struct rtc_time *tm)
+{
+	int err = -EINVAL;
+	struct rtc_class_ops *ops = class_get_devdata(class_dev);
+
+	if (ops->read_time) {
+		memset(tm, 0, sizeof(struct rtc_time));
+		err = ops->read_time(class_dev->dev, tm);
+	}
+	return err;
+}
+EXPORT_SYMBOL(rtc_read_time);
+
+int rtc_set_time(struct class_device *class_dev, struct rtc_time *tm)
+{
+	int err = -EINVAL;
+	struct rtc_class_ops *ops = class_get_devdata(class_dev);
+
+	err = rtc_valid_tm(tm);
+	if (err == 0 && ops->set_time)
+		err = ops->set_time(class_dev->dev, tm);
+
+	return err;
+}
+EXPORT_SYMBOL(rtc_set_time);
+
+int rtc_read_alarm(struct class_device *class_dev, struct rtc_wkalrm *alrm)
+{
+	struct rtc_class_ops *ops = class_get_devdata(class_dev);
+	int err = -EINVAL;
+
+	if (ops->read_alarm) {
+		memset(alrm, 0, sizeof(struct rtc_wkalrm));
+		err = ops->read_alarm(class_dev->dev, alrm);
+	}
+	return err;
+}
+EXPORT_SYMBOL(rtc_read_alarm);
+
+int rtc_set_alarm(struct class_device *class_dev, struct rtc_wkalrm *alrm)
+{
+	int err = -EINVAL;
+	struct rtc_class_ops *ops = class_get_devdata(class_dev);
+
+	if (ops->set_alarm)
+		err = ops->set_alarm(class_dev->dev, alrm);
+	return err;
+}
+EXPORT_SYMBOL(rtc_set_alarm);
+
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-nslu2/drivers/rtc/Kconfig	2005-12-20 19:52:39.000000000 +0100
@@ -0,0 +1,25 @@
+#
+# RTC class/drivers configuration
+#
+
+menu "Real Time Clock"
+
+config RTC_CLASS
+	tristate "RTC class"
+	depends on EXPERIMENTAL
+	default y
+	help
+	  Generic RTC class support. If you say yes here, you will
+ 	  be allowed to plug one or more RTCs to your system. You will
+	  probably want to enable one of more of the interfaces below.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-class.
+
+comment "RTC interfaces"
+	depends on RTC_CLASS
+
+comment "RTC drivers"
+	depends on RTC_CLASS
+
+endmenu
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-nslu2/drivers/rtc/Makefile	2005-12-20 19:52:39.000000000 +0100
@@ -0,0 +1,8 @@
+#
+# Makefile for RTC class/drivers.
+#
+
+obj-y				+= utils.o
+obj-$(CONFIG_RTC_CLASS)		+= rtc-core.o
+rtc-core-y			:= class.o intf.o
+rtc-core-objs			:= $(rtc-core-y)
--- linux-nslu2.orig/include/linux/rtc.h	2005-12-15 09:28:09.000000000 +0100
+++ linux-nslu2/include/linux/rtc.h	2005-12-15 09:28:14.000000000 +0100
@@ -93,6 +93,36 @@ struct rtc_pll_info {
 
 #ifdef __KERNEL__
 
+#include <linux/device.h>
+
+struct rtc_class_ops {
+	struct module *owner;
+	int (*open)(struct device *);
+	void (*release)(struct device *);
+	int (*ioctl)(struct device *, unsigned int, unsigned long);
+	int (*read_time)(struct device *, struct rtc_time *);
+	int (*set_time)(struct device *, struct rtc_time *);
+	int (*read_alarm)(struct device *, struct rtc_wkalrm *);
+	int (*set_alarm)(struct device *, struct rtc_wkalrm *);
+	int (*proc)(struct device *, char *buf);
+	int (*set_mmss)(struct device *, unsigned long secs);
+};
+
+extern int rtc_interface_register(struct class_interface *intf);
+extern struct class_device *rtc_device_register(struct device *dev,
+					struct rtc_class_ops *ops);
+extern void rtc_device_unregister(struct class_device *cdev);
+
+
+extern int rtc_month_days(unsigned int month, unsigned int year);
+extern int rtc_valid_tm(struct rtc_time *tm);
+extern int rtc_tm_to_time(struct rtc_time *tm, unsigned long *time);
+
+extern int rtc_read_time(struct class_device *class_dev, struct rtc_time *tm);
+extern int rtc_set_time(struct class_device *class_dev, struct rtc_time *tm);
+extern int rtc_read_alarm(struct class_device *class_dev, struct rtc_wkalrm *alrm);
+extern int rtc_set_alarm(struct class_device *class_dev, struct rtc_wkalrm *alrm);
+
 typedef struct rtc_task {
 	void (*func)(void *private_data);
 	void *private_data;
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-nslu2/drivers/rtc/utils.c	2005-12-15 09:28:14.000000000 +0100
@@ -0,0 +1,99 @@
+/*
+ * rtc-utils.c - rtc subsystem, utility functions
+ *
+ * Copyright (C) 2005 Tower Technologies
+ * Author: Alessandro Zummo <a.zummo@towertech.it>
+ *
+ * based on arch/arm/common/rtctime.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+*/
+#include <linux/module.h>
+#include <linux/rtc.h>
+
+static const unsigned char rtc_days_in_month[] = {
+	31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
+};
+EXPORT_SYMBOL(rtc_days_in_month);
+
+#define LEAPS_THRU_END_OF(y) ((y)/4 - (y)/100 + (y)/400)
+#define LEAP_YEAR(year) ((!(year % 4) && (year % 100)) || !(year % 400))
+
+int rtc_month_days(unsigned int month, unsigned int year)
+{
+	return rtc_days_in_month[month] + (LEAP_YEAR(year) && month == 1);
+}
+EXPORT_SYMBOL(rtc_month_days);
+
+/*
+ * Convert seconds since 01-01-1970 00:00:00 to Gregorian date.
+ */
+void rtc_time_to_tm(unsigned long time, struct rtc_time *tm)
+{
+	int days, month, year;
+
+	days = time / 86400;
+	time -= days * 86400;
+
+	tm->tm_wday = (days + 4) % 7;
+
+	year = 1970 + days / 365;
+	days -= (year - 1970) * 365
+	        + LEAPS_THRU_END_OF(year - 1)
+	        - LEAPS_THRU_END_OF(1970 - 1);
+	if (days < 0) {
+		year -= 1;
+		days += 365 + LEAP_YEAR(year);
+	}
+	tm->tm_year = year - 1900;
+	tm->tm_yday = days + 1;
+
+	for (month = 0; month < 11; month++) {
+		int newdays;
+
+		newdays = days - rtc_month_days(month, year);
+		if (newdays < 0)
+			break;
+		days = newdays;
+	}
+	tm->tm_mon = month;
+	tm->tm_mday = days + 1;
+
+	tm->tm_hour = time / 3600;
+	time -= tm->tm_hour * 3600;
+	tm->tm_min = time / 60;
+	tm->tm_sec = time - tm->tm_min * 60;
+}
+EXPORT_SYMBOL(rtc_time_to_tm);
+
+/*
+ * Does the rtc_time represent a valid date/time?
+ */
+int rtc_valid_tm(struct rtc_time *tm)
+{
+	if (tm->tm_year < 70 ||
+	    tm->tm_mon >= 12 ||
+	    tm->tm_mday < 1 ||
+	    tm->tm_mday > rtc_month_days(tm->tm_mon, tm->tm_year + 1900) ||
+	    tm->tm_hour >= 24 ||
+	    tm->tm_min >= 60 ||
+	    tm->tm_sec >= 60)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL(rtc_valid_tm);
+
+/*
+ * Convert Gregorian date to seconds since 01-01-1970 00:00:00.
+ */
+int rtc_tm_to_time(struct rtc_time *tm, unsigned long *time)
+{
+	*time = mktime(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday,
+		       tm->tm_hour, tm->tm_min, tm->tm_sec);
+
+	return 0;
+}
+EXPORT_SYMBOL(rtc_tm_to_time);
--- linux-nslu2.orig/drivers/Kconfig	2005-12-15 09:28:09.000000000 +0100
+++ linux-nslu2/drivers/Kconfig	2005-12-15 09:28:14.000000000 +0100
@@ -66,4 +66,6 @@ source "drivers/infiniband/Kconfig"
 
 source "drivers/sn/Kconfig"
 
+source "drivers/rtc/Kconfig"
+
 endmenu
--- linux-nslu2.orig/drivers/Makefile	2005-12-15 09:28:09.000000000 +0100
+++ linux-nslu2/drivers/Makefile	2005-12-15 09:51:34.000000000 +0100
@@ -54,6 +54,8 @@ obj-$(CONFIG_USB_GADGET)	+= usb/gadget/
 obj-$(CONFIG_GAMEPORT)		+= input/gameport/
 obj-$(CONFIG_INPUT)		+= input/
 obj-$(CONFIG_I2O)		+= message/
+# rtc should be before i2c for now.
+obj-y				+= rtc/
 obj-$(CONFIG_I2C)		+= i2c/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_HWMON)		+= hwmon/

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Christoph Hellwig]

On Tue, Dec 20, 2005 at 09:45:11PM +0100, Alessandro Zummo wrote:
> This patch adds the basic RTC subsytem infrastructure
> to the kernel.

Whee, very cool.  We've needed something like that for a long time.

> rtc/class.c - registration facilities for RTC drivers
> rtc/intf.c - kernel/rtc interface functions 
> rtc/utils.c - misc rtc-related utility functions
> 
> Signed-off-by: Alessandro Zummo <a.zummo@towertech.it>
> --
>  drivers/Kconfig      |    2 
>  drivers/Makefile     |    2 
>  drivers/rtc/Kconfig  |   25 +++++++++++
>  drivers/rtc/Makefile |    8 +++
>  drivers/rtc/class.c  |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/rtc/intf.c   |   67 +++++++++++++++++++++++++++++++
>  drivers/rtc/utils.c  |   99 +++++++++++++++++++++++++++++++++++++++++++++


Given that the files are really tiny I'd suggest to put everything into
a single file (driver/char/rtc.c) instead of some arbitrary split.

> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-nslu2/drivers/rtc/class.c	2005-12-15 10:22:20.000000000 +0100
> @@ -0,0 +1,110 @@
> +/*
> + * rtc-class.c - rtc subsystem, base class

no need to put the file name into a comment.  it gets out of date far
too easily (it already is in this case ;-))


> +#define RTC_ID_PREFIX "rtc"
> +#define RTC_ID_FORMAT RTC_ID_PREFIX "%d"

Having a format specifier hidden in a macro makes reading code very
difficult, please just remove this.

> +
> +static struct class *rtc_class;
> +
> +static DEFINE_IDR(rtc_idr);
> +
> +/**
> + * rtc_device_register - register w/ hwmon sysfs class
> + * @dev: the device to register
> + *
> + * rtc_device_unregister() must be called when the class device is no
> + * longer needed.
> + *
> + * Returns the pointer to the new struct class device.
> + */
> +struct class_device *rtc_device_register(struct device *dev,
> +					struct rtc_class_ops *ops)
> +{
> +	struct class_device *cdev;
> +	int id;
> +
> +	if (idr_pre_get(&rtc_idr, GFP_KERNEL) == 0)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (idr_get_new(&rtc_idr, NULL, &id) < 0)
> +		return ERR_PTR(-ENOMEM);
> +
> +	id = id & MAX_ID_MASK;
> +	cdev = class_device_create(rtc_class, NULL, MKDEV(0,0), dev,
> +					RTC_ID_FORMAT, id);
> +
> +	if (IS_ERR(cdev))
> +		idr_remove(&rtc_idr, id);
> +	else
> +		dev_info(dev, "rtc core: registered\n");
> +
> +	class_set_devdata(cdev, ops);
> +
> +	return cdev;
> +}

> +void rtc_device_unregister(struct class_device *cdev)
> +{
> +	int id;
> +
> +	if (sscanf(cdev->class_id, RTC_ID_FORMAT, &id) == 1) {
> +		class_device_unregister(cdev);
> +		idr_remove(&rtc_idr, id);
> +	} else
> +		dev_dbg(cdev->dev,
> +			"rtc_device_unregister() failed: bad class ID!\n");
> +}

The scanf looks really fragile.  Can't you just have a rtc_device structure
that the cdev and id are embedded into that can be passed to the
unregistration function?

> +
> +int rtc_interface_register(struct class_interface *intf)
> +{
> +	intf->class = rtc_class;
> +        return class_interface_register(intf);

spaces instead of a tab for indentation here.

> +int rtc_read_time(struct class_device *class_dev, struct rtc_time *tm)
> +{
> +	int err = -EINVAL;
> +	struct rtc_class_ops *ops = class_get_devdata(class_dev);
> +
> +	if (ops->read_time) {
> +		memset(tm, 0, sizeof(struct rtc_time));

do we really need the memset?

> +#
> +# Makefile for RTC class/drivers.
> +#
> +
> +obj-y				+= utils.o

why is this always built?
> +obj-$(CONFIG_RTC_CLASS)		+= rtc-core.o
> +rtc-core-y			:= class.o intf.o
> +rtc-core-objs			:= $(rtc-core-y)

no need for this last line

> +struct rtc_class_ops {

What about just rtc_ops?

> +	int (*proc)(struct device *, char *buf);

this should be seq_file based.

> +static const unsigned char rtc_days_in_month[] = {
> +	31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
> +};
> +EXPORT_SYMBOL(rtc_days_in_month);

exporting static symbols is pretty wrong.  Exporting tables is pretty
bad style aswell.

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Alessandro Zummo]

On Tue, 20 Dec 2005 21:13:45 +0000
Christoph Hellwig <hch@infradead.org> wrote:

> >  drivers/rtc/class.c  |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/rtc/intf.c   |   67 +++++++++++++++++++++++++++++++
> >  drivers/rtc/utils.c  |   99 +++++++++++++++++++++++++++++++++++++++++++++
> 
> 
> Given that the files are really tiny I'd suggest to put everything into
> a single file (driver/char/rtc.c) instead of some arbitrary split.

 I can merge. It was easier to develop them this way.

> > + * rtc-class.c - rtc subsystem, base class
> 
> no need to put the file name into a comment.  it gets out of date far
> too easily (it already is in this case ;-))

 oooooops :)

> > +#define RTC_ID_PREFIX "rtc"
> > +#define RTC_ID_FORMAT RTC_ID_PREFIX "%d"
> 
> Having a format specifier hidden in a macro makes reading code very
> difficult, please just remove this.

 ack.

> > +	if (sscanf(cdev->class_id, RTC_ID_FORMAT, &id) == 1) {
> > +		class_device_unregister(cdev);
> > +		idr_remove(&rtc_idr, id);
> > +	} else
> > +		dev_dbg(cdev->dev,
> > +			"rtc_device_unregister() failed: bad class ID!\n");
> > +}
> 
> The scanf looks really fragile.  Can't you just have a rtc_device structure
> that the cdev and id are embedded into that can be passed to the
> unregistration function?

 I admit I copied from hwmon here :) I'll check into that.

> > +	if (ops->read_time) {
> > +		memset(tm, 0, sizeof(struct rtc_time));
> 
> do we really need the memset?

 It's a kind of protection in case something
 goes wrong in the driver. can be probably removed.

> > +obj-y				+= utils.o
> 
> why is this always built?

 because it exports utility functions used
 elsewhere in the kernel, some of them have been removed
 from the ARM part. Until everything gets cleaned up,
 it's better to always build those few funcs.

> > +obj-$(CONFIG_RTC_CLASS)		+= rtc-core.o
> > +rtc-core-y			:= class.o intf.o
> > +rtc-core-objs			:= $(rtc-core-y)
> 
> no need for this last line

 I remember I had compilation problems without it,
 I'll check again.

> > +struct rtc_class_ops {
> 
> What about just rtc_ops?

it's already used under ARM and i didn't wanted
to break it. 

 
> > +	int (*proc)(struct device *, char *buf);
> 
> this should be seq_file based.

 ack.

> > +static const unsigned char rtc_days_in_month[] = {
> > +	31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
> > +};
> > +EXPORT_SYMBOL(rtc_days_in_month);
> 
> exporting static symbols is pretty wrong.  Exporting tables is pretty
> bad style aswell.

 Tables like this one are often used in rtc drivers. What
 can I use instead?

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

h

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Russell King]

On Tue, Dec 20, 2005 at 09:13:45PM +0000, Christoph Hellwig wrote:
> > +int rtc_read_time(struct class_device *class_dev, struct rtc_time *tm)
> > +{
> > +	int err = -EINVAL;
> > +	struct rtc_class_ops *ops = class_get_devdata(class_dev);
> > +
> > +	if (ops->read_time) {
> > +		memset(tm, 0, sizeof(struct rtc_time));
> 
> do we really need the memset?

Absolutely yes, otherwise if 'tm' is on the stack and it ultimately
gets copied to userspace, it will leak kernel memory.  Why?
Unfortunately, not all elements of 'tm' are written to by RTC
drivers.

You can argue that the RTC drivers need fixing, but since this bug
has gone completely unnoticed in _all_ kernels which have an RTC
driver up until I discovered it and reported it to vendor-sec
during the 2.5 cycle, I think a little bit of cheap protection
against buggy drivers when security leaks are concerned is not
unreasonable.  Especially when they don't get found in the normal
run of things.

(you could make a case for eliminating it _if_ there was a RTC
subsystem maintainer who knew the code and therefore knew what
to look out for.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Mitchell Blank Jr]

Alessandro Zummo wrote:
> > > +static const unsigned char rtc_days_in_month[] = {
> > > +	31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
> > > +};
> > > +EXPORT_SYMBOL(rtc_days_in_month);
> > 
> > exporting static symbols is pretty wrong.  Exporting tables is pretty
> > bad style aswell.
> 
>  Tables like this one are often used in rtc drivers. What
>  can I use instead?

Why not just provide a macro in a .h file?  Something like:

/* Days in month -- month is in the range (0..11), no leap year */
#define rtc_days_in_month(mon)	(28 + ((0xEEFBB3 >> ((mon) * 2)) & 3))

-Mitch

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Dmitry Torokhov]

Hi,

On Tuesday 20 December 2005 15:45, Alessandro Zummo wrote:
> +int rtc_read_time(struct class_device *class_dev, struct rtc_time *tm)
> +{
> +       int err = -EINVAL;
> +       struct rtc_class_ops *ops = class_get_devdata(class_dev);
> +
> +       if (ops->read_time) {
> +               memset(tm, 0, sizeof(struct rtc_time));
> 

What guarantees that ops is not NULL here? Userspace can keep the
attribute (file) open and issue read after class_device was unregistered
and devdata set to NULL.

-- 
Dmitry

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Alessandro Zummo]

On Tue, 20 Dec 2005 17:50:12 -0800
Mitchell Blank Jr <mitch@sfgoth.com> wrote:

> > > exporting static symbols is pretty wrong.  Exporting tables is pretty
> > > bad style aswell.
> > 
> >  Tables like this one are often used in rtc drivers. What
> >  can I use instead?
> 
> Why not just provide a macro in a .h file?  Something like:
> 
> /* Days in month -- month is in the range (0..11), no leap year */
> #define rtc_days_in_month(mon)	(28 + ((0xEEFBB3 >> ((mon) * 2)) & 3))

 I just noticed i was exporting a similar functions, so
the table export can be removed.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Alessandro Zummo]

On Tue, 20 Dec 2005 21:01:39 -0500
Dmitry Torokhov <dtor_core@ameritech.net> wrote:


> > +       if (ops->read_time) {
> > +               memset(tm, 0, sizeof(struct rtc_time));
> > 
> 
> What guarantees that ops is not NULL here? Userspace can keep the
> attribute (file) open and issue read after class_device was unregistered
> and devdata set to NULL.

 Right. For /proc and /dev there's a try_module_get(ops->owner) in place. 

 Should I add it to every rtc_sysfs_show_xxx or there's
 a better way to do it?

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Dmitry Torokhov]

On 12/21/05, Alessandro Zummo <alessandro.zummo@towertech.it> wrote:
> On Tue, 20 Dec 2005 21:01:39 -0500
> Dmitry Torokhov <dtor_core@ameritech.net> wrote:
>
>
> > > +if (ops->read_time) {
> > > +memset(tm, 0, sizeof(struct rtc_time));
> > >
> >
> > What guarantees that ops is not NULL here? Userspace can keep the
> > attribute (file) open and issue read after class_device was unregistered
> > and devdata set to NULL.
>
>  Right. For /proc and /dev there's a try_module_get(ops->owner) in place.
>
>  Should I add it to every rtc_sysfs_show_xxx or there's
>  a better way to do it?
>

Well, I don't know what will it buy you: if ops is NULL
try_module_get(ops->owner) will OOPS just as happily as original code.

Your class_device has to hold on to all data structures that are
referenced from sysfs attributes untils its ->release() function is
called. Alternatively you could stuck a mutex and a flag somewhere in
driver data and take it when unregistering class device and also in
all attributes (and chech the flag there).

--
Dmitry

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Alessandro Zummo]

On Wed, 21 Dec 2005 14:43:01 -0500
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Well, I don't know what will it buy you: if ops is NULL
> try_module_get(ops->owner) will OOPS just as happily as original code.
> 
> Your class_device has to hold on to all data structures that are
> referenced from sysfs attributes untils its ->release() function is
> called. Alternatively you could stuck a mutex and a flag somewhere in
> driver data and take it when unregistering class device and also in
> all attributes (and chech the flag there).

 Thanks for your help.. I will try to implement a solution
 ans post it as soon as possible. 


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Pavel Machek]

Hi!

> rtc/class.c - registration facilities for RTC drivers
> rtc/intf.c - kernel/rtc interface functions 
> rtc/utils.c - misc rtc-related utility functions

> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-nslu2/drivers/rtc/intf.c	2005-12-15 09:28:14.000000000 +0100
> @@ -0,0 +1,67 @@

interface.c, please.


> +/*
> + * rtc-intf.c - rtc subsystem, interface functions

Wrong filename.

> +int rtc_set_alarm(struct class_device *class_dev, struct rtc_wkalrm*alrm)

Struct rtc_wake_alarm *alarm, those wovels are there for readability.

-- 
Thanks, Sharp!

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Alessandro Zummo]

On Thu, 22 Dec 2005 14:35:07 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-nslu2/drivers/rtc/intf.c	2005-12-15 09:28:14.000000000 +0100
> > @@ -0,0 +1,67 @@
> 
> interface.c, please.

 ack.

> > +/*
> > + * rtc-intf.c - rtc subsystem, interface functions
> 
> Wrong filename.

 ok, I removed all filenames :)
 
> > +int rtc_set_alarm(struct class_device *class_dev, struct rtc_wkalrm*alrm)
> 
> Struct rtc_wake_alarm *alarm, those wovels are there for readability.

 I'm not the one who created this structure. It's defined
 in linux/rtc.h since a long time. I can only change alrm
 to alarm.

 thanks!

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Pavel Machek]

Hi!

> > > +int rtc_set_alarm(struct class_device *class_dev, struct rtc_wkalrm*alrm)
> > 
> > Struct rtc_wake_alarm *alarm, those wovels are there for readability.
> 
>  I'm not the one who created this structure. It's defined
>  in linux/rtc.h since a long time. I can only change alrm
>  to alarm.

Sorry if you are not responsible... Yes, alarm would be better.

How does this relate to /proc/acpi/alarm, btw? Do they use same RTC
alarm?


							Pavel
-- 
Thanks, Sharp!

Re: [RFC][PATCH 1/6] RTC subsystem, class

[Alessandro Zummo]

On Mon, 26 Dec 2005 21:16:57 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> > > > +int rtc_set_alarm(struct class_device *class_dev, struct rtc_wkalrm*alrm)
> > > 
> > > Struct rtc_wake_alarm *alarm, those wovels are there for readability.
> > 
> >  I'm not the one who created this structure. It's defined
> >  in linux/rtc.h since a long time. I can only change alrm
> >  to alarm.
> 
> Sorry if you are not responsible... Yes, alarm would be better.
> 
> How does this relate to /proc/acpi/alarm, btw? Do they use same RTC
> alarm?

 Yes, they do. The ACPI subsystem does direct I/O on the cmos clock.

 Given the complexity of the ACPI code, when/if the x86 rtc driver
 will be ported to the RTC subsystem, it would be wise
 to disable the alarm part if ACPI is compiled in.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it