char/cyclades: replace

Any comments would be appreciated.

Description: Use msleep_interruptible() instead of schedule_timeout() to guarantee the task delays as expected.

Signed-off-by Nishanth Aravamudan <[email protected]>

The original patch mistakenly used the msecs_to_jiffies() macro when it should have used the jiffies_to_msecs() macro. Please find the corrected patch below.

drivers/char/cyclades.c +2 -4

@@ -2717,8 +2717,7 @@ cy_wait_until_sent(struct tty_struct *tt
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
 	    printk("Not clean (jiff=%lu)...", jiffies);
 #endif
-	    current->state = TASK_INTERRUPTIBLE;
-	    schedule_timeout(char_time);
+	    msleep_interruptible(jiffies_to_msecs(char_time));
 	    if (signal_pending(current))
 		break;
 	    if (timeout && time_after(jiffies, orig_jiffies + timeout))

@@ -2729,8 +2728,7 @@ cy_wait_until_sent(struct tty_struct *tt
 	// Nothing to do!
     }
     /* Run one more char cycle */
-    current->state = TASK_INTERRUPTIBLE;
-    schedule_timeout(char_time * 5);
+    msleep_interruptible(jiffies_to_msecs(char_time * 5));
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
     printk("Clean (jiff=%lu)...done\n", jiffies);
 #endif

Quoted 4 lines · Marcelo Tosatti

Well have to convert from jiffies (char_time) to msecs, just to convert it back again to jiffies in msleep_interruptible().

Is this patch actually fixing any problem?

So there are a few things here:

1) I think the overhead of the conversions is very small in comparison with the eventual call to schedule() and such, so I don't think it's that big of a deal -- *please* correct me if I am wrong, though.

2) I am currently trying to determine if it might be possible for the timeout value (char_time, and I guess by extension, timeout) to be expressed in milliseconds instead of jiffies. If you have any input on that, Marcelo (or anyone from the list), I would really appreciate it.

Well have to convert from jiffies (char_time) to msecs, just to convert it back again to jiffies in msleep_interruptible().

Is this patch actually fixing any problem?

Quoted 6 lines · Nishanth Aravamudan

Quoted 4 lines · Marcelo Tosatti

Well have to convert from jiffies (char_time) to msecs, just to convert it back again to jiffies in msleep_interruptible().

Is this patch actually fixing any problem?

Quoted 5 lines

So there are a few things here:

1) I think the overhead of the conversions is very small in comparison with the eventual call to schedule() and such, so I don't think it's that big of a deal -- *please* correct me if I am wrong, though.

No you are right the overhead is very small - I'm just trying to find a good reason to apply the change.

Quoted 4 lines

2) I am currently trying to determine if it might be possible for the timeout value (char_time, and I guess by extension, timeout) to be expressed in milliseconds instead of jiffies. If you have any input on that, Marcelo (or anyone from the list), I would really appreciate it.

I do not like the idea very much.

I mean, what is the problem with that piece of code right now?

ie please educate me about msleep_interruptible()

Quoted 7 lines · Marcelo Tosatti

Quoted 6 lines · Nishanth Aravamudan

Quoted 4 lines · Marcelo Tosatti

Well have to convert from jiffies (char_time) to msecs, just to convert it back again to jiffies in msleep_interruptible().

Is this patch actually fixing any problem?

Quoted 8 lines

Quoted 5 lines

So there are a few things here:

1) I think the overhead of the conversions is very small in comparison with the eventual call to schedule() and such, so I don't think it's that big of a deal -- *please* correct me if I am wrong, though.

No you are right the overhead is very small - I'm just trying to find a good reason to apply the change.

So I would say there are some good reasons to apply this patch. Presuming this delay is somewhat long (it's a little hard to determine this from the code, so your input here would again be appreciated), having most drivers delay for long times using msleep*() goes a long way towards consistency IMO. Since your timeout values are variables, though, this becomes a little more difficult to argue :)

Then again, if we could figure out a way to use seconds (or some other human time-unit) to express the delays, I think using msleep*() becomes pretty obvious.
Or even if it doesn't happen now for whatever reason, down the road this makes the change that much easier

Quoted 5 lines

Quoted 4 lines

2) I am currently trying to determine if it might be possible for the timeout value (char_time, and I guess by extension, timeout) to be expressed in milliseconds instead of jiffies. If you have any input on that, Marcelo (or anyone from the list), I would really appreciate it.

Quoted 1 line

I do not like the idea very much.

Out of curiousity, why? Would it be particularly hard to do? If that is the case, that's fine, I'm just wondering...

Quoted 3 lines

I mean, what is the problem with that piece of code right now?

ie please educate me about msleep_interruptible()

To be honest, I think the arguments for using msleep() in general are far stronger. msleep_interruptible() was recently introduced to have the same effect except allow for a TASK_INTERRUPTIBLE based sleep. Since you are checking for a signal after returning from schedule_timeout(), msleep() isn't an option here ... What can be done here, actually, I just realized is to remove the signal_pending() check, as msleep_interruptible() does this as well.

There is another thing my patch didn't do, though - the

	current->state = TASK_RUNNING;

call is unnecessary, because schedule_timeout() / msleep_interruptible() takes care of this.

I have made these changes in the following patch -- please revert the previous cyclades patch and apply this one. Let me know if this is more satisfactory (it definitely simplifies the while-loop a bit).

Description: Uses msleep_interruptible() instead of schedule_timeout(). This allows the removal of one conditional, as msleep_interruptible() should only return non-zero if signal_pending(current) is true. current->state also does not need to be set, as msleep_interruptible() (actually, schedule_timeout) is guaranteed to return in TASK_RUNNING.

Signed-off-by Nishanth Aravamudan <[email protected]>

drivers/char/cyclades.c +2 -6

@@ -2717,20 +2717,16 @@ cy_wait_until_sent(struct tty_struct *tt
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
 	    printk("Not clean (jiff=%lu)...", jiffies);
 #endif
-	    current->state = TASK_INTERRUPTIBLE;
-	    schedule_timeout(char_time);
-	    if (signal_pending(current))
+	    if (msleep_interruptible(jiffies_to_msecs(char_time)))
 		break;
 	    if (timeout && time_after(jiffies, orig_jiffies + timeout))
 		break;
 	}
-	current->state = TASK_RUNNING;
     } else {
 	// Nothing to do!
     }
     /* Run one more char cycle */
-    current->state = TASK_INTERRUPTIBLE;
-    schedule_timeout(char_time * 5);
+    msleep_interruptible(jiffies_to_msecs(char_time * 5));
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
     printk("Clean (jiff=%lu)...done\n", jiffies);
 #endif