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

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

[Jean Delvare]

Hi all,

I have the following patch, which addresses a might-sleep-in-tasklet
issue in the ds1374 driver. I'm not too sure if the new code is right
or not, as I have never been using workqueues before, and I also don't
have a DS1374 chip to test my changes on.

Can anyone comment on the patch and tell me if anything is wrong?

Can anyone with a DS1374 chip please test it?

I want this to be fixed now, because in -mm the ds1374 driver also uses
the new mutex implementation, which is not allowed in tasklets, but is
OK in workqueues.

Thanks.

* * * * *

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 |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

--- linux-2.6.16-git.orig/drivers/i2c/chips/ds1374.c	2006-03-23 10:21:48.000000000 +0100
+++ linux-2.6.16-git/drivers/i2c/chips/ds1374.c	2006-03-23 19:37:25.000000000 +0100
@@ -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,19 +169,15 @@
 
 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);
-	else
-		ds1374_set_tlet((ulong) & new_time);
-
-	return 0;
+	return queue_work(ds1374_workqueue, &ds1374_work);
 }
 
 /*
@@ -204,6 +201,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 +226,7 @@
 
 	if ((rc = i2c_detach_client(client)) == 0) {
 		kfree(i2c_get_clientdata(client));
-		tasklet_kill(&ds1374_tasklet);
+		destroy_workqueue(ds1374_workqueue);
 	}
 	return rc;
 }

-- 
Jean Delvare

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

[Randy Vinson]

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

Jean Delvare wrote:
> Hi all,
> 
> I have the following patch, which addresses a might-sleep-in-tasklet
> issue in the ds1374 driver. I'm not too sure if the new code is right
> or not, as I have never been using workqueues before, and I also don't
> have a DS1374 chip to test my changes on.
> 
> Can anyone comment on the patch and tell me if anything is wrong?
> 
> Can anyone with a DS1374 chip please test it?

I've attached a similar patch that has been tested using the DS1374 on the Freescale MPC8349MDS reference system. It is patterned after a similar change made to the m41t00 driver. The changes work, but I am also unfamiliar with workqueues, so my patch may not be any better.

> 
> I want this to be fixed now, because in -mm the ds1374 driver also uses
> the new mutex implementation, which is not allowed in tasklets, but is
> OK in workqueues.
> 
> Thanks.
> 


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

This patch changes the DS1374 driver to use workqueues (taken from a
similar change to the m41t00.c driver.) This patch has been tested on the
Freescale MPC8349MDS.

Signed-off-by: Randy Vinson <rvinson@mvista.com>
Index: linux-2.6.10_wrk/drivers/i2c/chips/ds1374.c
===================================================================
--- linux-2.6.10_wrk.orig/drivers/i2c/chips/ds1374.c
+++ linux-2.6.10_wrk/drivers/i2c/chips/ds1374.c
@@ -26,6 +26,7 @@
 #include <linux/i2c.h>
 #include <linux/rtc.h>
 #include <linux/bcd.h>
+#include <linux/workqueue.h>
 
 #include <asm/time.h>
 
@@ -51,6 +52,8 @@ static struct i2c_client *save_client;
 static unsigned short ignore[] = { I2C_CLIENT_END };
 static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
 
+static struct work_struct set_rtc_time_task;
+
 static struct i2c_client_address_data addr_data = {
 	.normal_i2c = normal_addr,
 	.normal_i2c_range = ignore,
@@ -180,7 +183,7 @@ int ds1374_set_rtc_time(ulong nowtime)
 	new_time = nowtime;
 
 	if (in_interrupt())
-		tasklet_schedule(&ds1374_tasklet);
+		schedule_work(&set_rtc_time_task);
 	else
 		ds1374_set_tlet((ulong) & new_time);
 
@@ -215,6 +218,9 @@ static int ds1374_probe(struct i2c_adapt
 		return rc;
 	}
 
+	INIT_WORK(&set_rtc_time_task,
+			(void (*)(void *))&ds1374_set_tlet, &new_time);
+
 	save_client = client;
 
 	ds1374_check_rtc_status();
@@ -233,7 +239,7 @@ static int ds1374_detach(struct i2c_clie
 
 	if ((rc = i2c_detach_client(client)) == 0) {
 		kfree(i2c_get_clientdata(client));
-		tasklet_kill(&ds1374_tasklet);
+		flush_scheduled_work();
 	}
 	return rc;
 }

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

[Mark A. Greer]

On Thu, Mar 23, 2006 at 01:42:51PM -0700, Randy Vinson wrote:
> Jean Delvare wrote:
> >Hi all,
> >
> >I have the following patch, which addresses a might-sleep-in-tasklet
> >issue in the ds1374 driver. I'm not too sure if the new code is right
> >or not, as I have never been using workqueues before, and I also don't
> >have a DS1374 chip to test my changes on.
> >
> >Can anyone comment on the patch and tell me if anything is wrong?
> >
> >Can anyone with a DS1374 chip please test it?
> 
> I've attached a similar patch that has been tested using the DS1374 on the 
> Freescale MPC8349MDS reference system. It is patterned after a similar 
> change made to the m41t00 driver. The changes work, but I am also 
> unfamiliar with workqueues, so my patch may not be any better.

I'm no expert in workqueues either; however, after reading
http://lwn.net/Articles/23634/, I believe that its unnecessary for an
rtc driver to have its own workqueue since rtc writes aren't particularly
time-critical.  If I am correct, then Randy's patch uses the proper wq calls.  

Agree?

Mark

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

[Mark A. Greer]

On Thu, Mar 23, 2006 at 03:52:16PM -0600, Kumar Gala wrote:
> 
> On Mar 23, 2006, at 3:40 PM, Mark A. Greer wrote:
> >I'm no expert in workqueues either; however, after reading
> >http://lwn.net/Articles/23634/, I believe that its unnecessary for an
> >rtc driver to have its own workqueue since rtc writes aren't  
> >particularly
> >time-critical.  If I am correct, then Randy's patch uses the proper  
> >wq calls.
> >
> >Agree?
> 
> How does this change with the RTC subsystem that's about to be added?  
> (could be irrelevant, just wanted to put that out there)

Dunno, but its broken where it is now so we need to fix it ASAP.
Once that's done, we can do whatever needs to be done to get it into
drivers/rtc.

Mark

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

[Jean Delvare]

Hi Mark,

> > I've attached a similar patch that has been tested using the DS1374 on the 
> > Freescale MPC8349MDS reference system. It is patterned after a similar 
> > change made to the m41t00 driver. The changes work, but I am also 
> > unfamiliar with workqueues, so my patch may not be any better.
> 
> I'm no expert in workqueues either; however, after reading
> http://lwn.net/Articles/23634/, I believe that its unnecessary for an
> rtc driver to have its own workqueue since rtc writes aren't particularly
> time-critical.  If I am correct, then Randy's patch uses the proper wq calls.  
> 
> Agree?

I'm not sure. My first try was mostly similar to Randy's, using the
shared workqueue. However, LDD3 (and, for that matter, the article you
pointed to) says to be cautious when using the shared workqueue, not
only because of by what others can do to you, but also because of what
your can do to others.

ds1374_set_tlet triggers many i2c transfers, which may delay or sleep
depending on the underlying i2c implementation, and definitely will
take some time (at least 224 I2C clock cycles if I'm counting properly,
that is 14 ms at 16 kHz.)

So I came to the conclusion that it wouldn't be fair to other users if
ds1374 was using the shared workqueue. Now, I really don't know for
sure, so I'll let workqueue experts decide what should be done here.

Thanks,
-- 
Jean Delvare

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

[Mark A. Greer]

On Fri, Mar 24, 2006 at 09:53:11PM +0100, Jean Delvare wrote:
> Hi Mark,
> 
> > > I've attached a similar patch that has been tested using the DS1374 on the 
> > > Freescale MPC8349MDS reference system. It is patterned after a similar 
> > > change made to the m41t00 driver. The changes work, but I am also 
> > > unfamiliar with workqueues, so my patch may not be any better.
> > 
> > I'm no expert in workqueues either; however, after reading
> > http://lwn.net/Articles/23634/, I believe that its unnecessary for an
> > rtc driver to have its own workqueue since rtc writes aren't particularly
> > time-critical.  If I am correct, then Randy's patch uses the proper wq calls.  
> > 
> > Agree?
> 
> I'm not sure. My first try was mostly similar to Randy's, using the
> shared workqueue. However, LDD3 (and, for that matter, the article you
> pointed to) says to be cautious when using the shared workqueue, not
> only because of by what others can do to you, but also because of what
> your can do to others.
> 
> ds1374_set_tlet triggers many i2c transfers, which may delay or sleep
> depending on the underlying i2c implementation, and definitely will
> take some time (at least 224 I2C clock cycles if I'm counting properly,
> that is 14 ms at 16 kHz.)
> 
> So I came to the conclusion that it wouldn't be fair to other users if
> ds1374 was using the shared workqueue. Now, I really don't know for
> sure, so I'll let workqueue experts decide what should be done here.

Hmm, you raise a good point, Jean.  I just talked to Randy and we agreed
to agree with you.  :)  Randy will make a patch for the ds1374 and I'll
rework the patches for the m41t00.  Stay tuned...

Mark

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

[Jean Delvare]

Hi Mark,

> > ds1374_set_tlet triggers many i2c transfers, which may delay or sleep
> > depending on the underlying i2c implementation, and definitely will
> > take some time (at least 224 I2C clock cycles if I'm counting properly,
> > that is 14 ms at 16 kHz.)
> > 
> > So I came to the conclusion that it wouldn't be fair to other users if
> > ds1374 was using the shared workqueue. Now, I really don't know for
> > sure, so I'll let workqueue experts decide what should be done here.
> 
> Hmm, you raise a good point, Jean.  I just talked to Randy and we agreed
> to agree with you.  :)  Randy will make a patch for the ds1374 and I'll
> rework the patches for the m41t00.  Stay tuned...

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.

Thanks,
-- 
Jean Delvare

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

[Randy Vinson]

[-- 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;
 }