driver: ip27-rtc - convert ioctl to unlocked_ioctl

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 <[email protected]>

---

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.

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 <[email protected]>

---

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(-)

drivers/char/ip27-rtc.c +3 -3

index 932264a..1d83e16 100644
@@ -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};

drivers/char/ip27-rtc.c +39 -27

 	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;

Quoted 11 lines · Cyrill Gorcunov

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 <[email protected]>

---

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.

Looks like it can be dropped here. All usage of rtc-> is done under rtc_lock. Every other variable there is thread-local.

This routine seems to be re-entrant due to parameters on stack + the spin lock, hence the lock is not needed here at all.

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.

This patch converts ioctl call to unlocked_ioctl form. It's possible due to rtl_lock spinlock protection.

Signed-off-by Cyrill Gorcunov <[email protected]>

---
The patch is *not* tested but the patch does not bring _much_ changes so it wouldn't break the compilation procedure.

If there is problem with attachment - i could send it as inline form today evening.

Andi, Jiri, Alexey the only thing I do complain about - is time set/read from several user threads that uses same (duplicated) file descriptor. Could there be useless thread spins instead of sleep (in case if this unlocked_ioctl were protected by mutex)?

Quoted 8 lines

This patch converts ioctl call to unlocked_ioctl form. It's possible due to rtl_lock spinlock protection.

Signed-off-by Cyrill Gorcunov <[email protected]>

---
The patch is *not* tested but the patch does not bring _much_ changes so it wouldn't break the compilation procedure.
If there is problem with attachment - i could send it as inline form today evening.

Yes, please, especially if it is app/octet-stream (base64 encoded plaintext). Also Cc akpm or somebody who will pick your patch up.

Quoted 5 lines

Andi, Jiri, Alexey the only thing I do complain about - is time set/read from several user threads that uses same (duplicated) file descriptor. Could there be useless thread spins instead of sleep (in case if this unlocked_ioctl were protected by mutex)?

Sorry, what?

Jiri, I mean rtc_open() is protected by spinlock+status from being opened simultaneously by a few processes. *But* lets imagine the following situation - this fd (file descriptor) is opened by one multithreaded application so all threads have an access to this fd. Then one thread reads rtc periodically thru unlocked_ioctl and another thread set new time from time to time. So the question I have - is it possible to get second thread stopped at attemption to get rtc spinlock while another thread is setting the new time? Or this situation never-ever could be? i'm not really familiar with process management in Linux and as result could be wrong.

Quoted 10 lines

Jiri, I mean rtc_open() is protected by spinlock+status from being opened simultaneously by a few processes. *But* lets imagine the following situation - this fd (file descriptor) is opened by one multithreaded application so all threads have an access to this fd. Then one thread reads rtc periodically thru unlocked_ioctl and another thread set new time from time to time. So the question I have - is it possible to get second thread stopped at attemption to get rtc spinlock while another thread is setting the new time? Or this situation never-ever could be? i'm not really familiar with process management in Linux and as result could be wrong.

Access to global variable 'rtc' (the rtc itself) is serialized through the spinlock, I see no problem there. If you call read-read-write-read from 4 tasks in userspace, it might be _still_ (no change) reordered to e.g. write-read-read-read by the scheduler.

In fact, the reading process is stopped while the another one is writing the time (due to spinlock).

Yes, process would be stopped, and not *just* stopped but could spend all his cpu time-slice in attempt to get spinlock (espec if set time is much longer than read), but if we use mutex here the process could just sleep instead of trying to get spinlock granted. Am I wrong? Or this is not worth to do it?

Quoted 5 lines

Yes, process would be stopped, and not *just* stopped but could spend all his cpu time-slice in attempt to get spinlock (espec if set time is much longer than read), but if we use mutex here the process could just sleep instead of trying to get spinlock granted. Am I wrong? Or this is not worth to do it?

I would say no. It'll spin only for nanoseconds there.