Re: [PATCH, RFC] Stop using tasklet in ds1374 RTC driver

[Randy Vinson <rvinson@mvista.com>]

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

Jean Delvare wrote:
> 
> Well I already have a patch for ds1374. This is basically a mix between
> Randy's and my original patch: I've kept the dedicated workqueue as my
> patch had, but preserved the in_interrupt() call as in Randy's. Here's
> the result:
> 
> * * * * *
> 
> A tasklet is not suitable for what the ds1374 driver does: neither sleeping
> nor mutex operations are allowed in tasklets, and ds1374_set_tlet may do
> both.
> 
> We can use a workqueue instead, where both sleeping and mutex operations
> are allowed.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>  drivers/i2c/chips/ds1374.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> --- linux-2.6.16-git.orig/drivers/i2c/chips/ds1374.c	2006-03-27 18:25:17.000000000 +0200
> +++ linux-2.6.16-git/drivers/i2c/chips/ds1374.c	2006-03-27 18:59:05.000000000 +0200
> @@ -27,6 +27,7 @@
>  #include <linux/rtc.h>
>  #include <linux/bcd.h>
>  #include <linux/mutex.h>
> +#include <linux/workqueue.h>
>  
>  #define DS1374_REG_TOD0		0x00
>  #define DS1374_REG_TOD1		0x01
> @@ -139,7 +140,7 @@
>  	return t1;
>  }
>  
> -static void ds1374_set_tlet(ulong arg)
> +static void ds1374_set_work(void *arg)
>  {
>  	ulong t1, t2;
>  	int limit = 10;		/* arbitrary retry limit */
> @@ -168,17 +169,18 @@
>  
>  static ulong new_time;
>  
> -static DECLARE_TASKLET_DISABLED(ds1374_tasklet, ds1374_set_tlet,
> -				(ulong) & new_time);
> +static struct workqueue_struct *ds1374_workqueue;
> +
> +static DECLARE_WORK(ds1374_work, ds1374_set_work, &new_time);
>  
>  int ds1374_set_rtc_time(ulong nowtime)
>  {
>  	new_time = nowtime;
>  
>  	if (in_interrupt())
> -		tasklet_schedule(&ds1374_tasklet);
> +		queue_work(ds1374_workqueue, &ds1374_work);
>  	else
> -		ds1374_set_tlet((ulong) & new_time);
> +		ds1374_set_work(&new_time);
>  
>  	return 0;
>  }
> @@ -204,6 +206,8 @@
>  	client->adapter = adap;
>  	client->driver = &ds1374_driver;
>  
> +	ds1374_workqueue = create_singlethread_workqueue("ds1374");
> +
>  	if ((rc = i2c_attach_client(client)) != 0) {
>  		kfree(client);
>  		return rc;
> @@ -227,7 +231,7 @@
>  
>  	if ((rc = i2c_detach_client(client)) == 0) {
>  		kfree(i2c_get_clientdata(client));
> -		tasklet_kill(&ds1374_tasklet);
> +		destroy_workqueue(ds1374_workqueue);
>  	}
>  	return rc;
>  }
> 
> * * * * *
> 
> If it's OK, then Randy can just sign it off and I'll push it to Greg
> quickly. If it's not OK for some reason, I'll just wait for a new patch
> from Randy.
I've tested the patch. It works fine for me.

	Randy V.

[-- Attachment #2: ds1374_final.patch --]
[-- Type: text/plain, Size: 1942 bytes --]

A tasklet is not suitable for what the ds1374 driver does: neither sleeping
nor mutex operations are allowed in tasklets, and ds1374_set_tlet may do
both.

We can use a workqueue instead, where both sleeping and mutex operations
are allowed.

Acked-by: Randy Vinson <rvinson@mvista.com>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/chips/ds1374.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

--- linux-2.6.16-git.orig/drivers/i2c/chips/ds1374.c	2006-03-27 18:25:17.000000000 +0200
+++ linux-2.6.16-git/drivers/i2c/chips/ds1374.c	2006-03-27 18:59:05.000000000 +0200
@@ -27,6 +27,7 @@
 #include <linux/rtc.h>
 #include <linux/bcd.h>
 #include <linux/mutex.h>
+#include <linux/workqueue.h>
 
 #define DS1374_REG_TOD0		0x00
 #define DS1374_REG_TOD1		0x01
@@ -139,7 +140,7 @@
 	return t1;
 }
 
-static void ds1374_set_tlet(ulong arg)
+static void ds1374_set_work(void *arg)
 {
 	ulong t1, t2;
 	int limit = 10;		/* arbitrary retry limit */
@@ -168,17 +169,18 @@
 
 static ulong new_time;
 
-static DECLARE_TASKLET_DISABLED(ds1374_tasklet, ds1374_set_tlet,
-				(ulong) & new_time);
+static struct workqueue_struct *ds1374_workqueue;
+
+static DECLARE_WORK(ds1374_work, ds1374_set_work, &new_time);
 
 int ds1374_set_rtc_time(ulong nowtime)
 {
 	new_time = nowtime;
 
 	if (in_interrupt())
-		tasklet_schedule(&ds1374_tasklet);
+		queue_work(ds1374_workqueue, &ds1374_work);
 	else
-		ds1374_set_tlet((ulong) & new_time);
+		ds1374_set_work(&new_time);
 
 	return 0;
 }
@@ -204,6 +206,8 @@
 	client->adapter = adap;
 	client->driver = &ds1374_driver;
 
+	ds1374_workqueue = create_singlethread_workqueue("ds1374");
+
 	if ((rc = i2c_attach_client(client)) != 0) {
 		kfree(client);
 		return rc;
@@ -227,7 +231,7 @@
 
 	if ((rc = i2c_detach_client(client)) == 0) {
 		kfree(i2c_get_clientdata(client));
-		tasklet_kill(&ds1374_tasklet);
+		destroy_workqueue(ds1374_workqueue);
 	}
 	return rc;
 }