Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl
From: Jiri Slaby
Date: Sun Jan 13 2008 - 16:23:24 EST
On 01/13/2008 09:32 PM, Cyrill Gorcunov wrote:
This patch converts ioctl call to unlocked_ioctl form with
explicit big-kernel-lock. Also it makes a bit of cleanup
converting miscdevice structure initialization to C99 form.
Signed-off-by: Cyrill Gorcunov <gorcunov@xxxxxxxxx>
---
Any comments are welcome.
This is untested code - i've no such chip on my laptop.
Andi, i think we could use mutex to eliminate BKL, but not sure.
drivers/char/ip27-rtc.c | 86 ++++++++++++++++++++++++++++-------------------
1 files changed, 51 insertions(+), 35 deletions(-)
diff --git a/drivers/char/ip27-rtc.c b/drivers/char/ip27-rtc.c
index 932264a..1d83e16 100644
--- a/drivers/char/ip27-rtc.c
+++ b/drivers/char/ip27-rtc.c
@@ -46,8 +46,8 @@
#include <asm/sn/sn0/hub.h>
#include <asm/sn/sn_private.h>
-static int rtc_ioctl(struct inode *inode, struct file *file,
- unsigned int cmd, unsigned long arg);
+static long rtc_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg);
static int rtc_read_proc(char *page, char **start, off_t off,
int count, int *eof, void *data);
@@ -62,7 +62,7 @@ static void get_rtc_time(struct rtc_time *rtc_tm);
#define RTC_TIMER_ON 0x02 /* missed irq timer active */
static unsigned char rtc_status; /* bitmapped status byte. */
-static unsigned long rtc_freq; /* Current periodic IRQ rate */
+static unsigned long rtc_freq; /* Current periodic IRQ rate */
static struct m48t35_rtc *rtc;
/*
@@ -75,30 +75,34 @@ static unsigned long epoch = 1970; /* year corresponding to 0x00 */
static const unsigned char days_in_mo[] =
{0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
-static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
- unsigned long arg)
+static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
-
struct rtc_time wtime;
+ int err;
+
+ lock_kernel();
This routine seems to be re-entrant due to parameters on stack + the spin lock,
hence the lock is not needed here at all.
switch (cmd) {
case RTC_RD_TIME: /* Read the time/date from RTC */
- {
get_rtc_time(&wtime);
+ err = copy_to_user((void *)arg, &wtime, sizeof wtime) ? -EFAULT : 0;
break;
- }
case RTC_SET_TIME: /* Set the RTC */
{
struct rtc_time rtc_tm;
unsigned char mon, day, hrs, min, sec, leap_yr;
unsigned int yrs;
- if (!capable(CAP_SYS_TIME))
- return -EACCES;
+ if (!capable(CAP_SYS_TIME)) {
+ err = -EACCES;
+ goto unlock;
+ }
if (copy_from_user(&rtc_tm, (struct rtc_time*)arg,
- sizeof(struct rtc_time)))
- return -EFAULT;
+ sizeof(struct rtc_time))) {
+ err = -EFAULT;
+ goto unlock;
+ }
yrs = rtc_tm.tm_year + 1900;
mon = rtc_tm.tm_mon + 1; /* tm_mon starts at zero */
@@ -107,25 +111,27 @@ static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
min = rtc_tm.tm_min;
sec = rtc_tm.tm_sec;
+ err = -EINVAL;
+
if (yrs < 1970)
- return -EINVAL;
+ goto unlock;
leap_yr = ((!(yrs % 4) && (yrs % 100)) || !(yrs % 400));
if ((mon > 12) || (day == 0))
- return -EINVAL;
+ goto unlock;
if (day > (days_in_mo[mon] + ((mon == 2) && leap_yr)))
- return -EINVAL;
+ goto unlock;
if ((hrs >= 24) || (min >= 60) || (sec >= 60))
- return -EINVAL;
+ goto unlock;
if ((yrs -= epoch) > 255) /* They are unsigned */
- return -EINVAL;
+ goto unlock;
if (yrs > 169)
- return -EINVAL;
+ goto unlock;
if (yrs >= 100)
yrs -= 100;
@@ -148,12 +154,18 @@ static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
rtc->control &= ~M48T35_RTC_SET;
spin_unlock_irq(&rtc_lock);
- return 0;
+ err = 0;
+ break;
}
default:
- return -EINVAL;
+ err = -EINVAL;
+ break;
}
- return copy_to_user((void *)arg, &wtime, sizeof wtime) ? -EFAULT : 0;
+
+ unlock:
+ unlock_kernel();
+
+ return err;
}
/*
@@ -170,8 +182,8 @@ static int rtc_open(struct inode *inode, struct file *file)
spin_unlock_irq(&rtc_lock);
return -EBUSY;
}
-
rtc_status |= RTC_IS_OPEN;
+
spin_unlock_irq(&rtc_lock);
return 0;
@@ -197,16 +209,15 @@ static int rtc_release(struct inode *inode, struct file *file)
static const struct file_operations rtc_fops = {
.owner = THIS_MODULE,
- .ioctl = rtc_ioctl,
+ .unlocked_ioctl = rtc_ioctl,
.open = rtc_open,
.release = rtc_release,
};
-static struct miscdevice rtc_dev=
-{
- RTC_MINOR,
- "rtc",
- &rtc_fops
+static struct miscdevice rtc_dev = {
+ .minor = RTC_MINOR,
+ .name = "rtc",
+ .fops = &rtc_fops,
};
static int __init rtc_init(void)
@@ -229,16 +240,15 @@ static int __init rtc_init(void)
return 0;
}
+module_init(rtc_init);
-static void __exit rtc_exit (void)
+static void __exit rtc_exit(void)
{
/* interrupts and timer disabled at this point by rtc_release */
remove_proc_entry ("rtc", NULL);
misc_deregister(&rtc_dev);
}
-
-module_init(rtc_init);
module_exit(rtc_exit);
/*
@@ -274,14 +284,20 @@ static int rtc_get_status(char *buf)
}
static int rtc_read_proc(char *page, char **start, off_t off,
- int count, int *eof, void *data)
+ int count, int *eof, void *data)
{
int len = rtc_get_status(page);
- if (len <= off+count) *eof = 1;
+
+ if (len <= off + count)
+ *eof = 1;
+
*start = page + off;
len -= off;
- if (len>count) len = count;
- if (len<0) len = 0;
+ if (len > count)
+ len = count;
+ if (len < 0)
+ len = 0;
+
return len;
}
Post these cleanup things as a separate patch, please.
regards,
--
Jiri Slaby
Faculty of Informatics, Masaryk University
Suse Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/