[PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[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 <gorcunov@gmail.com>
---

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

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Alexey Dobriyan]

On Sun, Jan 13, 2008 at 11:32:23PM +0300, 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@gmail.com>
> ---
> 
> 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.

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Jiri Slaby]

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@gmail.com>
> ---
> 
> 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

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Cyrill Gorcunov]

Thanks, Alexey for review, i'll make it updated tomorrow. (sorry for top-post)

On 1/14/08, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Sun, Jan 13, 2008 at 11:32:23PM +0300, 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@gmail.com>
> > ---
> >
> > 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.
>

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Jiri Slaby]

On 01/13/2008 10:22 PM, Jiri Slaby wrote:
> This routine seems to be re-entrant due to parameters on stack + the 
> spin lock, hence the lock is not needed here at all.

s/parameters/variables/; too sleeeepy.

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Cyrill Gorcunov]

Thanks Jiri. Will do that all tomorrow.

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Andi Kleen]

On Sunday 13 January 2008 21:32:23 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.

First please always send cleanup patches separately. I think you have more
than just the C99 conversion.

Were you even able to compile it? Since ip27 is ia64 only that would need 
an IA64 cross compiler or a native ia64 machine.

It's probably better to focus on drivers only that actually build
on x86 :- see http://www.halobates.de/allyes for a compile log
showing all buildable files for x86-64 and i386.

Other than that the patch looks ok to me, but I haven't tried to 
compile it either.

-Andi

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Cyrill Gorcunov]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

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

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
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)?

[-- Attachment #2: ip27-rtc-unlocked-ioctl.patch --]
[-- Type: application/octet-stream, Size: 1310 bytes --]

 drivers/char/ip27-rtc.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/char/ip27-rtc.c b/drivers/char/ip27-rtc.c
index 932264a..86e6538 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 *filp, unsigned int cmd,
+			unsigned long arg);
 
 static int rtc_read_proc(char *page, char **start, off_t off,
                          int count, int *eof, void *data);
@@ -75,8 +75,7 @@ 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 *filp, unsigned int cmd, unsigned long arg)
 {
 
 	struct rtc_time wtime;
@@ -197,7 +196,7 @@ 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,
 };

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Jiri Slaby]

On 01/14/2008 07:38 AM, Cyrill Gorcunov wrote:
> This patch converts ioctl call to unlocked_ioctl form. It's possible
> due to rtl_lock spinlock protection.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> 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.

> 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?

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Cyrill Gorcunov]

[Jiri Slaby - Mon, Jan 14, 2008 at 04:14:18PM +0100]
> On 01/14/2008 07:38 AM, Cyrill Gorcunov wrote:
>> This patch converts ioctl call to unlocked_ioctl form. It's possible
>> due to rtl_lock spinlock protection.
>> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>> ---
>> 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.
>

ok

>> 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.

		- Cyrill -

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Jiri Slaby]

On 01/14/2008 04:38 PM, Cyrill Gorcunov wrote:
> 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).

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Cyrill Gorcunov]

[Jiri Slaby - Mon, Jan 14, 2008 at 04:59:22PM +0100]
> On 01/14/2008 04:38 PM, Cyrill Gorcunov wrote:
>> 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?

		- Cyrill -

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Jiri Slaby]

On 01/14/2008 05:07 PM, Cyrill Gorcunov wrote:
> 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.

Re: [PATCH] driver: ip27-rtc - convert ioctl to unlocked_ioctl

[Cyrill Gorcunov]

[Jiri Slaby - Mon, Jan 14, 2008 at 05:27:00PM +0100]
> On 01/14/2008 05:07 PM, Cyrill Gorcunov wrote:
>> 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.
>

Thanks a lot Jiri, I'll resend improved patch.

		- Cyrill -