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 0114a78b7cbb..0a870b7e5c32 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 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/rtc-dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 0a870b7e5c32..ae3bd4de7675 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 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 ae3bd4de7675..856cc1af40df 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 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 856cc1af40df..35dcc06eb3e2 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 From 1d96469a34781340b2cc9bdf97d5913eecdaa038 Mon Sep 17 00:00:00 2001 From: Atsushi Nemoto Date: Wed, 20 Aug 2008 14:09:11 -0700 Subject: rtc: fix double lock on UIE emulation With commit 5ad31a575157147b43fa84ef1e21471661653878 ("rtc: remove BKL for ioctl()"), RTC_UIE_ON ioctl cause double lock on rtc->ops_lock. The ops_lock must not be held while set_uie() calls rtc_read_time() which takes the lock. Also clear_uie() does not need ops_lock. This patch fixes return value of RTC_UIE_OFF ioctl too. Signed-off-by: Atsushi Nemoto Cc: David Brownell Cc: Russell King Cc: Alessandro Zummo Cc: "Rafael J. Wysocki" Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/rtc/rtc-dev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 35dcc06eb3e2..f118252f3a9f 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -403,11 +403,14 @@ static long rtc_dev_ioctl(struct file *file, #ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL case RTC_UIE_OFF: + mutex_unlock(&rtc->ops_lock); clear_uie(rtc); - break; + return 0; case RTC_UIE_ON: + mutex_unlock(&rtc->ops_lock); err = set_uie(rtc); + return err; #endif default: err = -ENOTTY; -- cgit v1.2.3 From 2e4a75cdcb89ff53bb182dda3a6dcdc14befe007 Mon Sep 17 00:00:00 2001 From: Marcin Slusarz Date: Fri, 3 Oct 2008 15:23:36 -0700 Subject: rtc: fix kernel panic on second use of SIGIO nofitication When userspace uses SIGIO notification and forgets to disable it before closing file descriptor, rtc->async_queue contains stale pointer to struct file. When user space enables again SIGIO notification in different process, kernel dereferences this (poisoned) pointer and crashes. So disable SIGIO notification on close. Kernel panic: (second run of qemu (requires echo 1024 > /sys/class/rtc/rtc0/max_user_freq)) general protection fault: 0000 [1] PREEMPT CPU 0 Modules linked in: af_packet snd_pcm_oss snd_mixer_oss snd_seq_oss snd_seq_midi_event snd_seq usbhid tuner tea5767 tda8290 tuner_xc2028 xc5000 tda9887 tuner_simple tuner_types mt20xx tea5761 tda9875 uhci_hcd ehci_hcd usbcore bttv snd_via82xx snd_ac97_codec ac97_bus snd_pcm snd_timer ir_common compat_ioctl32 snd_page_alloc videodev v4l1_compat snd_mpu401_uart snd_rawmidi v4l2_common videobuf_dma_sg videobuf_core snd_seq_device snd btcx_risc soundcore tveeprom i2c_viapro Pid: 5781, comm: qemu-system-x86 Not tainted 2.6.27-rc6 #363 RIP: 0010:[] [] __lock_acquire+0x3db/0x73f RSP: 0000:ffffffff80674cb8 EFLAGS: 00010002 RAX: ffff8800224c62f0 RBX: 0000000000000046 RCX: 0000000000000002 RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8800224c62f0 RBP: ffffffff80674d08 R08: 0000000000000002 R09: 0000000000000001 R10: ffffffff80238941 R11: 0000000000000001 R12: 0000000000000000 R13: 6b6b6b6b6b6b6b6b R14: ffff88003a450080 R15: 0000000000000000 FS: 00007f98b69516f0(0000) GS:ffffffff80623200(0000) knlGS:00000000f7cc86d0 CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b CR2: 0000000000a87000 CR3: 0000000022598000 CR4: 00000000000006e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process qemu-system-x86 (pid: 5781, threadinfo ffff880028812000, task ffff88003a450080) Stack: ffffffff80674cf8 0000000180238440 0000000200000002 0000000000000000 ffff8800224c62f0 0000000000000046 0000000000000000 0000000000000002 0000000000000002 0000000000000000 ffffffff80674d68 ffffffff8024fc7a Call Trace: [] lock_acquire+0x85/0xa9 [] ? send_sigio+0x2a/0x184 [] _read_lock+0x3e/0x4a [] ? send_sigio+0x2a/0x184 [] send_sigio+0x2a/0x184 [] ? __lock_acquire+0x6e1/0x73f [] ? kill_fasync+0x2c/0x4e [] __kill_fasync+0x54/0x65 [] kill_fasync+0x3a/0x4e [] rtc_update_irq+0x9c/0xa5 [] cmos_interrupt+0xae/0xc0 [] handle_IRQ_event+0x25/0x5a [] handle_edge_irq+0xdd/0x123 [] do_IRQ+0xe4/0x144 [] ret_from_intr+0x0/0xf [] ? __alloc_pages_internal+0xe7/0x3ad [] ? clear_page_c+0x7/0x10 [] ? get_page_from_freelist+0x385/0x450 [] ? __alloc_pages_internal+0xe7/0x3ad [] ? anon_vma_prepare+0x2e/0xf6 [] ? handle_mm_fault+0x227/0x6a5 [] ? do_page_fault+0x494/0x83f [] ? error_exit+0x0/0xa9 Code: cc 41 39 45 28 74 24 e8 5e 1d 0f 00 85 c0 0f 84 6a 03 00 00 83 3d 8f a9 aa 00 00 be 47 03 00 00 0f 84 6a 02 00 00 e9 53 03 00 00 <41> ff 85 38 01 00 00 45 8b be 90 06 00 00 41 83 ff 2f 76 24 e8 RIP [] __lock_acquire+0x3db/0x73f RSP ---[ end trace 431877d860448760 ]--- Kernel panic - not syncing: Aiee, killing interrupt handler! Signed-off-by: Marcin Slusarz Acked-by: Alessandro Zummo Acked-by: David Brownell Cc: Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/rtc/rtc-dev.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index f118252f3a9f..52e2743b04ec 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -422,6 +422,12 @@ done: return err; } +static int rtc_dev_fasync(int fd, struct file *file, int on) +{ + struct rtc_device *rtc = file->private_data; + return fasync_helper(fd, file, on, &rtc->async_queue); +} + static int rtc_dev_release(struct inode *inode, struct file *file) { struct rtc_device *rtc = file->private_data; @@ -434,16 +440,13 @@ static int rtc_dev_release(struct inode *inode, struct file *file) if (rtc->ops->release) rtc->ops->release(rtc->dev.parent); + if (file->f_flags & FASYNC) + rtc_dev_fasync(-1, file, 0); + clear_bit_unlock(RTC_DEV_BUSY, &rtc->flags); return 0; } -static int rtc_dev_fasync(int fd, struct file *file, int on) -{ - struct rtc_device *rtc = file->private_data; - return fasync_helper(fd, file, on, &rtc->async_queue); -} - static const struct file_operations rtc_dev_fops = { .owner = THIS_MODULE, .llseek = no_llseek, -- cgit v1.2.3 From 743e6a504f81d1e2f086e726b69fb6631d11f820 Mon Sep 17 00:00:00 2001 From: David Brownell Date: Wed, 15 Oct 2008 22:03:04 -0700 Subject: rtc: file close() consistently disables repeating irqs Make the rtc framework consistent about disabling 1/second update IRQs that may have been activated through the /dev interface, when that /dev file is closed. (It may have closed because of coredump, etc.) This was previously done only for emulated update IRQs ... now, do it always. Also comment the current policy: repeating IRQs (periodic, update) that userspace enabled will be cleanly disabled, but alarms are left alone. Such repeating IRQs are a constant and pointless system load. Update some RTC drivers to remove now-needless release() methods. Most such methods just enforce that policy. The others all seem to be buggy, and mistreat in-kernel clients of periodic or alarm IRQs. Signed-off-by: David Brownell Acked-by: Andrew Sharp Cc: Angelo Castello Acked-by: Atsushi Nemoto Acked-by: Paul Mundt Cc: Thomas Hommel Acked-by: Alessandro Zummo Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- drivers/rtc/rtc-dev.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) (limited to 'drivers/rtc/rtc-dev.c') diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c index 52e2743b04ec..079e9ed907e0 100644 --- a/drivers/rtc/rtc-dev.c +++ b/drivers/rtc/rtc-dev.c @@ -432,9 +432,15 @@ static int rtc_dev_release(struct inode *inode, struct file *file) { struct rtc_device *rtc = file->private_data; -#ifdef CONFIG_RTC_INTF_DEV_UIE_EMUL - clear_uie(rtc); -#endif + /* We shut down the repeating IRQs that userspace enabled, + * since nothing is listening to them. + * - Update (UIE) ... currently only managed through ioctls + * - Periodic (PIE) ... also used through rtc_*() interface calls + * + * Leave the alarm alone; it may be set to trigger a system wakeup + * later, or be used by kernel code, and is a one-shot event anyway. + */ + rtc_dev_ioctl(file, RTC_UIE_OFF, 0); rtc_irq_set_state(rtc, NULL, 0); if (rtc->ops->release) -- cgit v1.2.3