From 5ad31a575157147b43fa84ef1e21471661653878 Mon Sep 17 00:00:00 2001 From: David Brownell Date: Wed, 23 Jul 2008 21:30:33 -0700 Subject: rtc: remove BKL for ioctl() Remove implicit use of BKL in ioctl() from the RTC framework. Instead, the rtc->ops_lock is used. That's the same lock that already protects the RTC operations when they're issued through the exported rtc_*() calls in drivers/rtc/interface.c ... making this a bugfix, not just a cleanup, since both ioctl calls and set_alarm() need to update IRQ enable flags and that implies a common lock (which RTC drivers as a rule do not provide on their own). A new comment at the declaration of "struct rtc_class_ops" summarizes current locking rules. It's not clear to me that the exceptions listed there should exist ... if not, those are pre-existing problems which can be fixed in a patch that doesn't relate to BKL removal. Signed-off-by: David Brownell Cc: Alan Cox Cc: Jonathan Corbet Acked-by: Alessandro Zummo Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/rtc/rtc-dev.c | 58 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 20 deletions(-) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 0114a78b7cb..0a870b7e5c3 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -209,7 +209,7 @@ static unsigned int rtc_dev_poll(struct file *file, poll_table *wait) return (data != 0) ? (POLLIN | POLLRDNORM) : 0; } -static int rtc_dev_ioctl(struct inode *inode, struct file *file, +static long rtc_dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { int err = 0; @@ -219,6 +219,10 @@ static int rtc_dev_ioctl(struct inode *inode, struct file *file, struct rtc_wkalrm alarm; void __user *uarg = (void __user *) arg; + err = mutex_lock_interruptible(&rtc->ops_lock); + if (err) + return -EBUSY; + /* check that the calling task has appropriate permissions * for certain ioctls. doing this check here is useful * to avoid duplicate code in each driver. @@ -227,26 +231,31 @@ static int rtc_dev_ioctl(struct inode *inode, struct file *file, case RTC_EPOCH_SET: case RTC_SET_TIME: if (!capable(CAP_SYS_TIME)) - return -EACCES; + err = -EACCES; break; case RTC_IRQP_SET: if (arg > rtc->max_user_freq && !capable(CAP_SYS_RESOURCE)) - return -EACCES; + err = -EACCES; break; case RTC_PIE_ON: if (rtc->irq_freq > rtc->max_user_freq && !capable(CAP_SYS_RESOURCE)) - return -EACCES; + err = -EACCES; break; } + if (err) + goto done; + /* try the driver's ioctl interface */ if (ops->ioctl) { err = ops->ioctl(rtc->dev.parent, cmd, arg); - if (err != -ENOIOCTLCMD) + if (err != -ENOIOCTLCMD) { + mutex_unlock(&rtc->ops_lock); return err; + } } /* if the driver does not provide the ioctl interface @@ -265,15 +274,19 @@ static int rtc_dev_ioctl(struct inode *inode, struct file *file, switch (cmd) { case RTC_ALM_READ: + mutex_unlock(&rtc->ops_lock); + err = rtc_read_alarm(rtc, &alarm); if (err < 0) return err; if (copy_to_user(uarg, &alarm.time, sizeof(tm))) - return -EFAULT; - break; + err = -EFAULT; + return err; case RTC_ALM_SET: + mutex_unlock(&rtc->ops_lock); + if (copy_from_user(&alarm.time, uarg, sizeof(tm))) return -EFAULT; @@ -321,24 +334,26 @@ static int rtc_dev_ioctl(struct inode *inode, struct file *file, } } - err = rtc_set_alarm(rtc, &alarm); - break; + return rtc_set_alarm(rtc, &alarm); case RTC_RD_TIME: + mutex_unlock(&rtc->ops_lock); + err = rtc_read_time(rtc, &tm); if (err < 0) return err; if (copy_to_user(uarg, &tm, sizeof(tm))) - return -EFAULT; - break; + err = -EFAULT; + return err; case RTC_SET_TIME: + mutex_unlock(&rtc->ops_lock); + if (copy_from_user(&tm, uarg, sizeof(tm))) return -EFAULT; - err = rtc_set_time(rtc, &tm); - break; + return rtc_set_time(rtc, &tm); case RTC_PIE_ON: err = rtc_irq_set_state(rtc, NULL, 1); @@ -376,34 +391,37 @@ static int rtc_dev_ioctl(struct inode *inode, struct file *file, break; #endif case RTC_WKALM_SET: + mutex_unlock(&rtc->ops_lock); if (copy_from_user(&alarm, uarg, sizeof(alarm))) return -EFAULT; - err = rtc_set_alarm(rtc, &alarm); - break; + return rtc_set_alarm(rtc, &alarm); case RTC_WKALM_RD: + mutex_unlock(&rtc->ops_lock); err = rtc_read_alarm(rtc, &alarm); if (err < 0) return err; if (copy_to_user(uarg, &alarm, sizeof(alarm))) - return -EFAULT; - break; + err = -EFAULT; + return err; #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL case RTC_UIE_OFF: clear_uie(rtc); - return 0; + break; case RTC_UIE_ON: - return set_uie(rtc); + err = set_uie(rtc); #endif default: err = -ENOTTY; break; } +done: + mutex_unlock(&rtc->ops_lock); return err; } @@ -432,7 +450,7 @@ static const struct file_operations rtc_dev_fops = { .llseek = no_llseek, .read = rtc_dev_read, .poll = rtc_dev_poll, - .ioctl = rtc_dev_ioctl, + .unlocked_ioctl = rtc_dev_ioctl, .open = rtc_dev_open, .release = rtc_dev_release, .fasync = rtc_dev_fasync, -- cgit v1.2.3-70-g09d2 From b68bb2632453a9ca7d10a00d79adf60968cb4c05 Mon Sep 17 00:00:00 2001 From: David Brownell Date: Tue, 29 Jul 2008 22:33:30 -0700 Subject: rtc: don't return -EBUSY when mutex_lock_interruptible() fails It was pointed out that the RTC framework handles its mutex locks oddly ... returning -EBUSY when interrupted. This fixes that by returning the value of mutex_lock_interruptible() (i.e. -EINTR). Signed-off-by: David Brownell Acked-by: Alessandro Zummo Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/rtc/interface.c | 10 +++++----- drivers/rtc/rtc-dev.c | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index d397fa5f3a9..7af60b98d8a 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -20,7 +20,7 @@ int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm) err = mutex_lock_interruptible(&rtc->ops_lock); if (err) - return -EBUSY; + return err; if (!rtc->ops) err = -ENODEV; @@ -46,7 +46,7 @@ int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm) err = mutex_lock_interruptible(&rtc->ops_lock); if (err) - return -EBUSY; + return err; if (!rtc->ops) err = -ENODEV; @@ -66,7 +66,7 @@ int rtc_set_mmss(struct rtc_device *rtc, unsigned long secs) err = mutex_lock_interruptible(&rtc->ops_lock); if (err) - return -EBUSY; + return err; if (!rtc->ops) err = -ENODEV; @@ -106,7 +106,7 @@ static int rtc_read_alarm_internal(struct rtc_device *rtc, struct rtc_wkalrm *al err = mutex_lock_interruptible(&rtc->ops_lock); if (err) - return -EBUSY; + return err; if (rtc->ops == NULL) err = -ENODEV; @@ -293,7 +293,7 @@ int rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm) err = mutex_lock_interruptible(&rtc->ops_lock); if (err) - return -EBUSY; + return err; if (!rtc->ops) err = -ENODEV; diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 0a870b7e5c3..ae3bd4de767 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -221,7 +221,7 @@ static long rtc_dev_ioctl(struct file *file, err = mutex_lock_interruptible(&rtc->ops_lock); if (err) - return -EBUSY; + return err; /* check that the calling task has appropriate permissions * for certain ioctls. doing this check here is useful -- cgit v1.2.3-70-g09d2 From 5cdc98b8f51310f7cca05ad780f18f80dd9571de Mon Sep 17 00:00:00 2001 From: Tomas Janousek Date: Tue, 29 Jul 2008 22:33:51 -0700 Subject: rtc-dev: stop periodic interrupts on device release Solves http://bugzilla.kernel.org/show_bug.cgi?id=11127 The old rtc.c driver did it and some drivers (like rtc-sh) do it in their release function, though they should not -- because they should provide the irq_set_state op and the rtc framework itself should care about it. This patch makes it do so. I am aware that some drivers, like rtc-sh, handle userspace PIE sets in their ioctl op (instead of having the framework call the op), exporting the irq_set_state op at the same time. The logic in rtc_irq_set_state should make sure it doesn't matter and the driver should not need to care stopping periodic interrupts in its release routine any more. The correct way, in my opinion, should be this: 1) The driver provides the irq_set_state op and does not care closing the interrupts in its release op. 2) If the driver does not provide the op and handles PIE in the ioctl op, it's reponsible for closing them in its release op. 3) Something similar for other IRQs, like UIE -- if there's no in-kernel API like irq_set_state, handle it in ioctl and release ops. The framework will be responsible either for everything or for nothing. (This will probably change later.) Signed-off-by: Tomas Janousek Acked-by: David Brownell Acked-by: Alessandro Zummo Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/rtc/rtc-dev.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index ae3bd4de767..856cc1af40d 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -432,6 +432,8 @@ static int rtc_dev_release(struct inode *inode, struct file *file) #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL clear_uie(rtc); #endif + rtc_irq_set_state(rtc, NULL, 0); + if (rtc->ops->release) rtc->ops->release(rtc->dev.parent); -- cgit v1.2.3-70-g09d2 From b1c3c898274334a9255445ba0636d13eda8399d7 Mon Sep 17 00:00:00 2001 From: David Brownell Date: Tue, 12 Aug 2008 15:08:41 -0700 Subject: revert "rtc: cdev lock_kernel() pushdown" Revert commit 51a776fa7a7997e726d4a478eda0854c6f9143bd ("rtc: cdev lock_kernel() pushdown"). The RTC framework does not need BKL protection. Signed-off-by: David Brownell Cc: Jonathan Corbet Cc: Alessandro Zummo Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/rtc/rtc-dev.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 856cc1af40d..35dcc06eb3e 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -13,7 +13,6 @@ #include #include -#include #include "rtc-core.h" static dev_t rtc_devt; @@ -27,11 +26,8 @@ static int rtc_dev_open(struct inode *inode, struct file *file) struct rtc_device, char_dev); const struct rtc_class_ops *ops = rtc->ops; - lock_kernel(); - if (test_and_set_bit_lock(RTC_DEV_BUSY, &rtc->flags)) { - err = -EBUSY; - goto out; - } + if (test_and_set_bit_lock(RTC_DEV_BUSY, &rtc->flags)) + return -EBUSY; file->private_data = rtc; @@ -41,13 +37,11 @@ static int rtc_dev_open(struct inode *inode, struct file *file) rtc->irq_data = 0; spin_unlock_irq(&rtc->irq_lock); - goto out; + return 0; } /* something has gone wrong */ clear_bit_unlock(RTC_DEV_BUSY, &rtc->flags); -out: - unlock_kernel(); return err; } -- cgit v1.2.3-70-g09d2